Re: [PATCH v4 2/9] iio: dac: mcp47feb02: correct gain bitshift at probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] System suspend and resume forcefully overwrite and lose the user-configured powerdown state.
- [High] System resume corrupts the VREF (voltage reference) configuration by overwriting it with the powerdown mode.
- [High] Sysfs store handler returns 0 on false state, potentially causing userspace infinite loops.
--

--- Patch [2]: [PATCH v4 2/9] iio: dac: mcp47feb02: correct gain bitshift at probe ---
commit f1be9d71af346cda91f297b65aff2d8b9c5d0198
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: dac: mcp47feb02: correct gain bitshift at probe

This commit corrects the formula for initializing the gain struct field
for each channel. It ensures the correct bits are extracted from the
Gain Register.

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index 7502959d98eab..bf78618ac2c89 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -1017,7 +1017,6 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  	if (ret)
>  		return ret;
>  
> -	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, dac_val;
> @@ -1028,7 +1027,7 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  		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;
> +		data->chdata[i].use_2x_gain = (gain_ch & DAC_GAIN_MASK(i)) ? 1 : 0;
>  
>  		/*
>  		 * Inform the user that the current voltage reference read from the volatile

While reviewing this patch, I noticed a few pre-existing issues in this file:

drivers/iio/dac/mcp47feb02.c:mcp47feb02_suspend() {
    ...
	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
		u8 pd_mode;

		data->chdata[ch].powerdown = true;
    ...
}

drivers/iio/dac/mcp47feb02.c:mcp47feb02_resume() {
    ...
	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
		u8 pd_mode;
		int ret;

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

[Severity: High]
This isn't a bug introduced by this patch, but does system suspend and
resume forcefully overwrite and lose the user-configured powerdown state?

The state is blindly overwritten with true during suspend and false during
resume. The hardware is then unconditionally awakened, ignoring any prior
user configuration to keep the channel powered down.


drivers/iio/dac/mcp47feb02.c:mcp47feb02_resume() {
    ...
		pd_mode = data->chdata[ch].powerdown_mode + 1;

		ret = regmap_write(data->regmap, REG_ADDR(ch), data->chdata[ch].dac_data);
		if (ret)
			return ret;

		ret = regmap_update_bits(data->regmap, MCP47FEB02_VREF_REG_ADDR,
					 DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));
    ...
}

[Severity: High]
This is a pre-existing issue, but does system resume corrupt the VREF
configuration by overwriting it with the powerdown mode?

The pd_mode variable appears to be mistakenly passed as the value for the
MCP47FEB02_VREF_REG_ADDR register instead of the correct reference mode.


drivers/iio/dac/mcp47feb02.c:store_eeprom_store() {
    ...
	ret = kstrtobool(buf, &state);
	if (ret)
		return ret;

	if (!state)
		return 0;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to return 0
here?

Returning 0 from a sysfs store handler instead of the length of the
written buffer can trick userspace tools into an infinite loop as they
repeatedly try to write the remaining bytes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-mcp47feb02_refactor-v4-0-beb2aa3bfd09@microchip.com?part=2