Re: [PATCH v5 01/10] iio: dac: mcp47feb02: initialize dac_data field in channel data struct at probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect powerdown state evaluation at probe time causes the driver to misinterpret hardware powerdown mode 1 as normal operation.
- [High] System resume unconditionally forces channels out of powerdown, losing user configuration.
--

commit f99608deaab75112b697e5e0e28a4014f58de232
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: dac: mcp47feb02: initialize dac_data field in channel data struct at probe

Prevent operations which use cache from overwriting default hardware values
with zeros by reading active DAC registers during probe to correctly
initialize the channel struct DAC values.

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index a823c2a673a26..7502959d98eab 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -1020,7 +1020,12 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  	gain_ch = gain_ch & MCP47FEB02_GAIN_BITS_MASK;
>  	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
>  		struct device *dev = regmap_get_device(data->regmap);
> -		unsigned int pd_tmp;
> +		unsigned int pd_tmp, dac_val;
> +
> +		ret = regmap_read(data->regmap, REG_ADDR(i), &dac_val);
> +		if (ret)
> +			return ret;
> +		data->chdata[i].dac_data = dac_val;
>  
>  		data->chdata[i].ref_mode = (vref_ch >> (2 * i)) & MCP47FEB02_DAC_CTRL_MASK;
>  		data->chdata[i].use_2x_gain = (gain_ch >> i)  & MCP47FEB02_GAIN_BIT_MASK;

[Severity: High]
This is a pre-existing issue, but does the probe logic misinterpret hardware
powerdown mode 1 as normal operation?

If the probe phase reads the powerdown state register and it returns 1, pd_tmp
is 1. This correctly maps powerdown_mode to index 0.

However, the boolean powerdown evaluation becomes !!0, which is false:

mcp47feb02_init_ctrl_regs() {
        [ ... ]
        pd_tmp = (pd_ch >> (2 * i)) & MCP47FEB02_DAC_CTRL_MASK;
        data->chdata[i].powerdown_mode = pd_tmp ? (pd_tmp - 1) : pd_tmp;
        data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);
        [ ... ]
}

Because of this, the channel is flagged as actively operating when the
hardware is physically in powerdown mode 1 (1kOhm to GND), creating a
mismatch between the software state and the physical hardware state.

[Severity: High]
This is another pre-existing issue, but do the suspend and resume routines
discard user configuration and unconditionally force channels out of powerdown?

During suspend, the driver overrides the software powerdown state to true:

mcp47feb02_suspend() {
        [ ... ]
        data->chdata[ch].powerdown = true;
        pd_mode = data->chdata[ch].powerdown_mode + 1;
        ret = regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
                                 DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));
        [ ... ]
}

During resume, it blindly forces all active channels to false and writes
MCP47FEB02_NORMAL_OPERATION to the hardware:

mcp47feb02_resume() {
        [ ... ]
        data->chdata[ch].powerdown = false;

        ret = regmap_write(data->regmap, REG_ADDR(ch), data->chdata[ch].dac_data);
        [ ... ]
        ret = regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
                                 DAC_CTRL_MASK(ch),
                                 DAC_CTRL_VAL(ch, MCP47FEB02_NORMAL_OPERATION));
        [ ... ]
}

If a user intentionally powered down a channel, it will unexpectedly wake up
and output voltage upon system resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-mcp47feb02_refactor-v5-0-8b67bcab93d1@microchip.com?part=1