Re: [PATCH v4 9/9] iio: dac: add support for Microchip MCP48FEB02

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The EEPROM write cycle polling timeout is too short, leading to sporadic -ETIMEDOUT errors and partial EEPROM updates.
--

--- Patch [9]: [PATCH v4 9/9] iio: dac: add support for Microchip MCP48FEB02 ---
commit f9a8ca3d3f3e9e1b7296c59e5fdc69c68349d1ae
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: dac: add support for Microchip MCP48FEB02

This patch adds SPI support for the Microchip MCP48FxBy1/2/4/8 series of DACs.
It introduces a new SPI driver module that registers devices using the common
core probe function.

[ ... ]

> diff --git a/drivers/iio/dac/mcp47feb02-spi.c b/drivers/iio/dac/mcp47feb02-spi.c
> new file mode 100644
> index 0000000000000..0c68d7c318cb2
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02-spi.c
> @@ -0,0 +1,145 @@
[ ... ]
> +static int mcp47feb02_spi_probe(struct spi_device *spi)
> +{
> +	const struct mcp47feb02_features *chip_features;
> +	struct device *dev = &spi->dev;
> +	struct regmap *regmap;
> +
> +	chip_features = spi_get_device_match_data(spi);
> +	if (!chip_features)
> +		return dev_err_probe(dev, -ENODEV, "No SPI device found\n");
> +
> +	if (chip_features->have_eeprom)
> +		regmap = devm_regmap_init_spi(spi, &mcp47feb02_regmap_config);
> +	else
> +		regmap = devm_regmap_init_spi(spi, &mcp47fvb02_regmap_config);
> +
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "Error initializing SPI regmap\n");
> +
> +	return mcp47feb02_common_probe(chip_features, regmap);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential timeout
problem in the core driver that might affect these newly supported SPI devices.

In drivers/iio/dac/mcp47feb02-core.c:mcp47feb02_write_to_eeprom(), the EEPROM
write cycle polling timeout is set to 5 ms:

	ret = regmap_read_poll_timeout(data->regmap, MCP47FEB02_GAIN_CTRL_STATUS_REG_ADDR,
				       eewa_val,
				       !(eewa_val & MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK),
				       USEC_PER_MSEC, USEC_PER_MSEC * 5);

Could this short timeout cause sporadic -ETIMEDOUT errors during EEPROM writes?

The EEPROM write cycle (t_WC) for the MCP47/48FEBxx series can take up to 10 ms.
When a user writes to the store_eeprom sysfs attribute, this function is called
in a loop. Could a timeout here cause the sequence to abort midway, leaving
the EEPROM in an inconsistent, partially updated state?

The same 5 ms timeout appears again in store_eeprom_store():

	ret = regmap_read_poll_timeout(data->regmap, MCP47FEB02_GAIN_CTRL_STATUS_REG_ADDR, eewa_val,
				       !(eewa_val & MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK),
				       USEC_PER_MSEC, USEC_PER_MSEC * 5);

Would it be better to increase the timeout to at least 10 ms to match the
hardware maximum?

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