Extcon: Arizona: Add driver for Wolfson Arizona class devices
Quoted 11 lines
Most Wolfson Arizona class audio hub CODECs include a flexible ultra low power accessory detection subsystem. This driver exposes initial support for this subsystem via the Extcon framework, implementing support for ultra low power detection of headphone and headset with the ability to detect the polarity of a headset.
The functionality of the devices is much richer and more flexible than the current driver, future patches will extend the features of the driver to more fully exploit this.
Signed-off-by Mark Brown <[email protected]>
The driver looks good.
I only have some performance concerns that may be ignored if you don't care of it for this device.
Quoted 7 lines
--- drivers/extcon/Kconfig | 8 + drivers/extcon/Makefile | 1 + drivers/extcon/extcon-arizona.c | 491 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 500 insertions(+) create mode 100644 drivers/extcon/extcon-arizona.c
[]
Review
Quoted 11 lines
(quoted diff) +11
-0+ +#define ARIZONA_CABLE_MECHANICAL "Mechanical" +#define ARIZONA_CABLE_HEADPHONE "Headphone" +#define ARIZONA_CABLE_HEADSET "Headset" + +static const char *arizona_cable[] = { + ARIZONA_CABLE_MECHANICAL, + ARIZONA_CABLE_HEADSET, + ARIZONA_CABLE_HEADPHONE, + NULL, +};
For ARIZONA_CABLE_HEADPHONE and ARIZONA_CABLE_MECHANICAL, you can use extcon_cable_name[EXTCON_HEADPHONE_OUT] and extcon_cable_name[EXTCON_MECHANICAL].
It appears that I need to rephrase line 38-41 of extcon_class.c. Anyway, it is not recommended to import the whole list. However, it is strongly recommended to reuse the corresponding entries from the list.
Anyway, the HEADSET appears to be a pair of HEADPHONE and MIC. You may replace HEADSET with MIC in arizona_cable and remove exclusive[] and regard HEADPHONE | MIC as "HEADSET".
Quoted 25 lines
(quoted diff) +25
-0+ +static irqreturn_t arizona_micdet(int irq, void *data) +{ + struct arizona_extcon_info *info = data; + struct arizona *arizona = info->arizona; + unsigned int val; + int ret; + + mutex_lock(&info->lock); + + ret = regmap_read(arizona->regmap, ARIZONA_MIC_DETECT_3, &val); + if (ret != 0) { + dev_err(arizona->dev, "Failed to read MICDET: %d\n", ret); + return IRQ_NONE; + } + + dev_dbg(arizona->dev, "MICDET: %x\n", val); + + if (!(val & ARIZONA_MICD_VALID)) { + dev_warn(arizona->dev, "Microphone detection state invalid\n"); + mutex_unlock(&info->lock); + return IRQ_NONE; + } + + /* Due to jack detect this should never happen */
Review
Quoted 10 lines
(quoted diff) +10
-0+ if (!(val & ARIZONA_MICD_STS)) { + dev_warn(arizona->dev, "Detected open circuit\n"); + info->detecting = false; + goto handled; + } + + /* If we got a high impedence we should have a headset, report it. */ + if (info->detecting && (val & 0x400)) { + ret = extcon_set_cable_state(&info->edev, + ARIZONA_CABLE_HEADSET, true);
You may use extcon_set_cable_state_ for the performance as you already know the index of HEADSET. Or extcon_update_state();
Quoted 15 lines
(quoted diff) +15
-0+ + if (ret != 0) + dev_err(arizona->dev, "Headset report failed: %d\n", + ret); + + info->mic = true; + info->detecting = false; + goto handled; + } + + /* If we detected a lower impedence during initial startup + * then we probably have the wrong polarity, flip it. Don't + * do this for the lowest impedences to speed up detection of + * plain headphones. If both polarities report a low + * impedence then give up and report headphones.
Review
Quoted 10 lines
(quoted diff) +10
-0+ */ + if (info->detecting && (val & 0x3f8)) { + info->jack_flips++; + + if (info->jack_flips >= info->micd_num_modes) { + dev_dbg(arizona->dev, "Detected headphone\n"); + info->detecting = false; + ret = extcon_set_cable_state(&info->edev, + ARIZONA_CABLE_HEADPHONE, + true);
Same for here.
[]
Quoted 8 lines
(quoted diff) +8
-0+ + /* + * If we're still detecting and we detect a short then we've + * got a headphone. Otherwise it's a button press, the + * button reporting is stubbed out for now. + */ + if (val & 0x3fc) { + if (info->mic) {
Review
Quoted 10 lines
(quoted diff) +10
-0+ dev_dbg(arizona->dev, "Mic button detected\n"); + + } else if (info->detecting) { + dev_dbg(arizona->dev, "Headphone detected\n"); + info->detecting = false; + arizona_stop_mic(info); + + ret = extcon_set_cable_state(&info->edev, + ARIZONA_CABLE_HEADPHONE, + true);
Here, too.
[]
Quoted 15 lines
(quoted diff) +15
-0+ +static irqreturn_t arizona_jackdet(int irq, void *data) +{ + struct arizona_extcon_info *info = data; + struct arizona *arizona = info->arizona; + unsigned int val; + int ret; + + pm_runtime_get_sync(info->dev); + + mutex_lock(&info->lock); + + ret = regmap_read(arizona->regmap, ARIZONA_AOD_IRQ_RAW_STATUS, &val); + if (ret != 0) { + dev_err(arizona->dev, "Failed to read jackdet status: %d\n",
Review
Quoted 10 lines
(quoted diff) +10
-0+ ret); + mutex_unlock(&info->lock); + pm_runtime_put_autosuspend(info->dev); + return IRQ_NONE; + } + + if (val & ARIZONA_JD1_STS) { + dev_dbg(arizona->dev, "Detected jack\n"); + ret = extcon_set_cable_state(&info->edev, + ARIZONA_CABLE_MECHANICAL, true);
Here.
Cheers!
MyungJoo
�淸�{.n�+돴윯돪�+%듚�깁負쪐w��{.n�+돴쪐{깸G쳺��{ay� ���,j�f＂톒쉹��묎�z_溫(�쉸듶줷"얎�m����G쳺�◀?솳鈺�&ｘ�~뤳턨O뺞촾퇉v�^�m��� �띠�◀?뻀�
Quoted 2 lines · MyungJoo Ham
I only have some performance concerns that may be ignored if you don't care of it for this device.
To be honest I think if we ever care about performance with extcon we've got a serious problem - cable insertion shouldn't be happening too quickly and obviously the userspace API has all the same issues.
Quoted 15 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+#define ARIZONA_CABLE_MECHANICAL "Mechanical" +#define ARIZONA_CABLE_HEADPHONE "Headphone" +#define ARIZONA_CABLE_HEADSET "Headset" +static const char *arizona_cable[] = { + ARIZONA_CABLE_MECHANICAL, + ARIZONA_CABLE_HEADSET, + ARIZONA_CABLE_HEADPHONE, + NULL, +};For
ARIZONA_CABLE_HEADPHONEandARIZONA_CABLE_MECHANICAL, you can use extcon_cable_name[EXTCON_HEADPHONE_OUT] and extcon_cable_name[EXTCON_MECHANICAL].
Quoted 3 lines
It appears that I need to rephrase line 38-41 of
extcon_class.c. Anyway, it is not recommended to import the whole list. However, it is strongly recommended to reuse the corresponding entries from the list.
That's what I initially wanted to do but there's real usability problems fishing the values out of the array, the obvious method does things like this:
drivers/extcon/extcon-arizona.c:62: error: initializer element is not constant drivers/extcon/extcon-arizona.c:62: error: (near initialization for 'arizona_cable[0]')
for example and you don't want the driver to end up looking like line noise. Perhaps there's some simple way of doing it that didn't occur to me but there aren't any examples in tree.
Quoted 3 lines
Anyway, the HEADSET appears to be a pair of HEADPHONE and MIC. You may replace HEADSET with MIC in
arizona_cableand remove exclusive[] and regard HEADPHONE | MIC as "HEADSET".
This was done following the example of the Android switch API which defines these as separate cable types. Cable type is probably the wrong name here but it's a bit late now...
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* If we got a high impedence we should have a headset, report it. */ + if (info->detecting && (val & 0x400)) { + ret = extcon_set_cable_state(&info->edev, + ARIZONA_CABLE_HEADSET, true);You may use
extcon_set_cable_state_for the performance as you already know the index of HEADSET. Orextcon_update_state();
I didn't use set_cable_state_ as the _ makes it look like extcon_set_cable_state() is the intended call, obviously almost every driver will have the indexes known. If there's much preferenced here I'd expect the main function to take the numbers as argument and then have extcon_set_cable_state_by_name() or something.
extcon_update_state() is a bit annoying to use as you need defines for both indexes and bits or you need shifting so the code looks ugly.