Re: [PATCH v5 5/7] thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing `regmap_write()` with `regmap_field_write()` implicitly changes a full register overwrite to a read-modify-write, potentially failing to clear other bits in the register.
--

commit 156e68047b2cd59d81061c97136f7825e128ca91
Author: Christian Marangi <ansuelsmth@gmail.com>

thermal/drivers: airoha: Generalize get_thermal_ADC and set_mux function

This commit introduces the use of the regmap field API for get_thermal_ADC
and set_thermal_mux in preparation for Airoha AN7583 support. It also
improves comments and moves a sleep call into the mux function.

> diff --git a/drivers/thermal/airoha_thermal.c b/drivers/thermal/airoha_thermal.c
> index ebb47ae5f2cee..249abbbd46bcd 100644
> --- a/drivers/thermal/airoha_thermal.c
> +++ b/drivers/thermal/airoha_thermal.c

[ ... ]

> @@ -219,25 +228,32 @@ static int airoha_get_thermal_ADC(struct airoha_thermal_priv *priv)
> -static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
> +static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
> +				   int tdac_idx)
>  {
> -	u32 adc_mux, pllrg;
> +	u32 pllrg;
>  
>  	/* Save PLLRG current value */
>  	regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
>  
> -	/* Give access to thermal regs */
> +	/* Give access to Thermal regs */
>  	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
>  		     priv->pllrg_protect);
> -	adc_mux = FIELD_PREP(EN7581_MUX_TADC, EN7581_SCU_THERMAL_MUX_DIODE1);
> -	regmap_write(priv->chip_scu, EN7581_PWD_TADC, adc_mux);
> +
> +	/* Configure Thermal ADC mux to tdac_idx */
> +	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
> +			   tdac_idx);

[Severity: High]
Does this unintentionally change a full register write into a
read-modify-write?

Before this patch, airoha_init_thermal_ADC_mode used regmap_write to
write the mux value directly to EN7581_PWD_TADC. Because adc_mux only
populated bits 1-3, this implicitly cleared all other bits to 0.

By switching to regmap_field_write, the operation becomes a
read-modify-write on bits 1-3. Given the register name EN7581_PWD_TADC, if
there is a power-down bit that defaults to 1 at reset, the new code will
preserve it as 1 instead of clearing it to 0.

Could this regression leave the ADC permanently powered down or in an
invalid state?

>  
>  	/* Restore PLLRG value on exit */
>  	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520155525.22239-1-ansuelsmth@gmail.com?part=5