Re: [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

[sashiko-bot@kernel.org]

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

New issues:
- [Low] I2C Subsystem Guideline Violation: Positional Initializers

Pre-existing issues:
- [High] NULL Pointer Dereference in power management callbacks due to missing driver data
- [High] Hardware register corruption in `mcp47feb02_resume` due to a copy-paste error updating VREF
- [High] Resume path unconditionally overrides user's manual power-down configuration
--

--- Patch [1]: [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules ---
commit bb66896c21fa2d6229e950c6190b3d83baedb5ad
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

This commit refactors the MCP47FEB02 driver into a core module and an
I2C-specific module to prepare for future SPI support. It separates the
bus-specific code from the common DAC functionality.

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02-core.c
> similarity index 64%
> rename from drivers/iio/dac/mcp47feb02.c
> rename to drivers/iio/dac/mcp47feb02-core.c
> index a823c2a673a26..6c86fa40e6eb2 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02-core.c

[ ... ]

> @@ -666,7 +357,7 @@ static ssize_t mcp47feb02_write_powerdown(struct iio_dev *indio_dev, uintptr_t p
>  	return len;
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(mcp47feb02_pm_ops, mcp47feb02_suspend, mcp47feb02_resume);
> +EXPORT_SIMPLE_DEV_PM_OPS(mcp47feb02_pm_ops, mcp47feb02_suspend, mcp47feb02_resume);

[Severity: High]
This is a pre-existing issue, but does mcp47feb02_resume() write pd_mode
to the VREF register instead of the expected reference mode?

Looking at mcp47feb02_resume() in the core module:

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

This writes the power-down mode to the voltage reference register, leading
to incorrect analog output voltages upon system resume.

[Severity: High]
This isn't a bug introduced by this patch, but does mcp47feb02_resume()
also unconditionally clear the powerdown state and force normal operation?

Looking at mcp47feb02_resume() in the core module:

    data->chdata[ch].powerdown = false;

This completely wipes out any explicit channel power-down state previously
configured by the user via sysfs.

[ ... ]

> @@ -1175,75 +857,7 @@ static int mcp47feb02_probe(struct i2c_client *client)
>  
>  	return devm_iio_device_register(dev, indio_dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does mcp47feb02_common_probe()
miss a call to dev_set_drvdata() before returning?

Without setting the driver data, dev_get_drvdata(dev) in
mcp47feb02_suspend() and mcp47feb02_resume() returns NULL:

    struct iio_dev *indio_dev = dev_get_drvdata(dev);
    struct mcp47feb02_data *data = iio_priv(indio_dev);

This causes a NULL pointer dereference when passed to iio_priv() during
system power state transitions.

> diff --git a/drivers/iio/dac/mcp47feb02-i2c.c b/drivers/iio/dac/mcp47feb02-i2c.c
> new file mode 100644
> index 0000000000000..808c51d0afdf5
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02-i2c.c

[ ... ]

> +static const struct i2c_device_id mcp47feb02_i2c_id[] = {
> +	{ "mcp47feb01", (kernel_ulong_t)&mcp47feb01_chip_features },
> +	{ "mcp47feb02", (kernel_ulong_t)&mcp47feb02_chip_features },

[Severity: Low]
Does this array need to use named initializers to align with I2C subsystem
guidelines?

The refactored mcp47feb02_i2c_id array uses positional initializers,
dropping the named initializers present in the original code.

> +	{ "mcp47feb04", (kernel_ulong_t)&mcp47feb04_chip_features },

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-mcp47feb02_refactor-v1-0-ee59e63672bc@microchip.com?part=1