Re: [PATCH v5 07/10] iio: dac: mcp47feb02: rename command mask define

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver fails to correctly parse the powerdown state during initialization, causing it to lose the distinction between 'Normal Operation' and '1 kOhm to GND' powerdown mode.
- [High] The driver's suspend/resume PM callbacks permanently clobber the user-configured logical powerdown state.
--

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

iio: dac: mcp47feb02: rename command mask define

This commit replaces the initial mask define with two new macros.
These new macros explicitly select the corresponding bits for each channel
in the VREF and Power Down registers.

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index c054711a55ca0..fb66bd2664a9e 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c

[ ... ]

> @@ -1069,7 +1070,7 @@ static int mcp47feb02_init_ctrl_regs(struct mcp47feb02_data *data)
>  			break;
>  		}
>  
> -		pd_tmp = (pd_ch >> (2 * i)) & MCP47FEB02_DAC_CTRL_MASK;
> +		pd_tmp = MCP47FEB02_GET_POWER_DOWN_MODE(pd_ch, i);
>  		data->chdata[i].powerdown_mode = pd_tmp ? (pd_tmp - 1) : pd_tmp;
>  		data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible this logic
incorrectly parses the powerdown state during initialization?

When pd_tmp is 1 (1 kOhm to GND), powerdown_mode is calculated as 1 - 1 = 0.
When pd_tmp is 0 (Normal Operation), powerdown_mode is also 0.

Since the driver then sets powerdown to !!powerdown_mode (which evaluates to
false in both cases), it seems the driver loses the hardware's true powerdown
state if it booted in the '1 kOhm to GND' mode, misreporting it as normal
operation.

>  	}


[Severity: High]
This is a pre-existing issue, but I wanted to ask about the suspend and
resume behavior in this driver.

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;
    ...
}

If a DAC channel is explicitly placed in powerdown by the user via sysfs,
would it unexpectedly wake up fully powered on after a system resume because
the user's requested powerdown configuration is permanently overwritten?

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