Re: [PATCH] Extcon: Arizona: Add driver for Wolfson Arizona classdevices

[MyungJoo Ham]

> 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 <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>


The driver looks good.

I only have some performance concerns that may be ignored
if you don't care of it for this device.

> ---
>  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
> 

[]

> +
> +#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".

> +
> +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 */
> +	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();

> +
> +		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.
> +	 */
> +	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.

[]

> +
> +	/*
> +	 * 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) {
> +			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.

[]

> +
> +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",
> +			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

?移ťŽ&Ţ~ş&śŹ–+-깁負ĽŠwŽ왢쎉喝méb욎dz받–)í끾čw*jgП¨ś‰šŽ듶˘j/곴äz받–듺2듷솳鈺Ú&˘)傘Ťa뛴ŽGŤ앶hŽćj:+v돣Šwč녪Ľ>W슧違iŰaxPjŘmśŸ˙Ă-ť+껠dš_