Re: [PATCH 2/2] ASoC: cs42l43: Update load detect procedure for cs42l43b

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: cs42l43: cs42l43_run_type_detect() treats invalid
  status as headset
- [Medium] ASoC: cs42l43: CS42L43B_LOAD_DET_SEL cached due to unread
  regmap_write()
--

commit 2f8a25cd3cc97db982b37853fb39365ad8d29795
Author: Charles Keepax <ckeepax@opensource.cirrus.com>

ASoC: cs42l43: Update load detect procedure for cs42l43b

This commit updates the headphone load detection procedure to pass the
detected jack type into load detection and configure CS42L43B_LOAD_DET_SEL
on newer chip variants.

> diff --git a/sound/soc/codecs/cs42l43-jack.c b/sound/soc/codecs/cs42l43-jack.c
> index a2555dd9229e3..b3f89c712ed93 100644
> --- a/sound/soc/codecs/cs42l43-jack.c
> +++ b/sound/soc/codecs/cs42l43-jack.c

[ ... ]

> @@ -526,14 +533,36 @@ static void cs42l43_start_load_detect(struct cs42l43_codec *priv, bool mic)
>  			dev_err(priv->dev, "Load detect HP power down timed out\n");
>  	}
>  
> -	if (mic)
> -		regmap_multi_reg_write_bypassed(cs42l43->regmap,
> -						cs42l43_4pole_patch,
> -						ARRAY_SIZE(cs42l43_4pole_patch));
> -	else
> -		regmap_multi_reg_write_bypassed(cs42l43->regmap,
> -						cs42l43_3pole_patch,
> -						ARRAY_SIZE(cs42l43_3pole_patch));
> +	switch (cs42l43->variant_id) {
> +	case CS42L43_DEVID_VAL:
> +		switch (type) {
> +		case CS42L43_TYPE_3POLE:
> +		case CS42L43_TYPE_OPEN:
> +			patch = cs42l43_3pole_patch;
> +			size = ARRAY_SIZE(cs42l43_3pole_patch);
> +			break;
> +		default:
> +			patch = cs42l43_4pole_patch;
> +			size = ARRAY_SIZE(cs42l43_4pole_patch);
> +			break;
> +		}
> +		break;
> +	default:
> +		patch = cs42l43_4pole_patch;
> +		size = ARRAY_SIZE(cs42l43_4pole_patch);
> +
> +		switch (type) {
> +		case CS42L43_TYPE_OMTP:
> +			regmap_write(cs42l43->regmap, CS42L43B_LOAD_DET_SEL, 0x89);
> +			break;
> +		default:
> +			regmap_write(cs42l43->regmap, CS42L43B_LOAD_DET_SEL, 0x98);
> +			break;
> +		}
> +		break;
> +	}

[Severity: Medium]
Does writing CS42L43B_LOAD_DET_SEL via regmap_write() here and in
cs42l43_stop_load_detect() cache the register in REGCACHE_MAPLE and cause
every subsequent regcache_sync() in cs42l43_runtime_resume() to re-write
it to hardware?

Commit "mfd: cs42l43: Add load detection register needed for cs42l43b"
added CS42L43B_LOAD_DET_SEL to cs42l43_precious_register() (which
cs42l43_volatile_register() also consults), but did not add it to
cs42l43_readable_register().  Because regmap_volatile() returns false for
non-readable registers when format_write is NULL, regmap treats
CS42L43B_LOAD_DET_SEL as cacheable, and with no entry in
cs42l43_reg_default[] it is synced back on every resume unless it is marked
readable or written with cache bypass.

[ ... ]

> @@ -629,23 +666,25 @@ static int cs42l43_run_load_detect(struct cs42l43_codec *priv, bool mic)
>  	if (!cs42l43_jack_present(priv))
>  		return -ENODEV;
>  
> -	if (mic) {
> +	switch (type) {
> +	case CS42L43_TYPE_3POLE:
> +	case CS42L43_TYPE_OPEN:
> +		switch (val & CS42L43_AMP3_RES_DET_MASK) {
> +		case 0x0: // < 22 Ohm impedance
> +		case 0x1: // < 150 Ohm impedance
> +		case 0x2: // < 1000 Ohm impedance
> +			return CS42L43_JACK_HEADPHONE;
> +		case 0x3: // > 1000 Ohm impedance
> +			return CS42L43_JACK_LINEOUT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
>  		cs42l43_start_hs_bias(priv, false);
>  		cs42l43_start_button_detect(priv);
>  
>  		return CS42L43_JACK_HEADSET;
>  	}
> -
> -	switch (val & CS42L43_AMP3_RES_DET_MASK) {
> -	case 0x0: // < 22 Ohm impedance
> -	case 0x1: // < 150 Ohm impedance
> -	case 0x2: // < 1000 Ohm impedance
> -		return CS42L43_JACK_HEADPHONE;
> -	case 0x3: // > 1000 Ohm impedance
> -		return CS42L43_JACK_LINEOUT;
> -	default:
> -		return -EINVAL;
> -	}
>  }
>  
>  static int cs42l43_run_type_detect(struct cs42l43_codec *priv)
> @@ -686,18 +725,12 @@ static int cs42l43_run_type_detect(struct cs42l43_codec *priv)
>  	if (!cs42l43_jack_present(priv))
>  		return -ENODEV;
>  
> -	switch (type & CS42L43_HSDET_TYPE_STS_MASK) {
> -	case 0x0: // CTIA
> -	case 0x1: // OMTP
> -	case 0x4:
> -		return cs42l43_run_load_detect(priv, true);
> -	case 0x2: // 3-pole
> -		return cs42l43_run_load_detect(priv, false);
> -	case 0x3: // Open-circuit
> +	type &= CS42L43_HSDET_TYPE_STS_MASK;
> +
> +	if (type == CS42L43_TYPE_OPEN)
>  		return CS42L43_JACK_EXTENSION;
> -	default:
> -		return -EINVAL;
> -	}
> +	else
> +		return cs42l43_run_load_detect(priv, type);
>  }

[Severity: Medium]
Does replacing the switch in cs42l43_run_type_detect() cause invalid
CS42L43_HS_STAT values (0x5..0x7) to be misclassified as a 4-pole headset
instead of returning -EINVAL?

In cs42l43_run_type_detect(), type is initialized to 0xff and
regmap_read(cs42l43->regmap, CS42L43_HS_STAT, &type) is not checked for
errors; if that read fails or hardware reports 0x5..0x7, type &
CS42L43_HSDET_TYPE_STS_MASK is passed to cs42l43_run_load_detect(), which
now hits the default branch, enables headset bias and button detection,
and reports CS42L43_JACK_HEADSET.  In addition, the CS42L43_TYPE_OPEN
cases in cs42l43_start_load_detect() and cs42l43_run_load_detect() are
unreachable because cs42l43_run_type_detect() returns
CS42L43_JACK_EXTENSION when type == CS42L43_TYPE_OPEN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111171032.1491717-1-ckeepax@opensource.cirrus.com?part=2