Re: [PATCH v7 09/11] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

[Jonathan Cameron <jic23@kernel.org>]

> Prepare the driver for the bus-specific code by refactoring into separate
> files. The renamed file will contain the common DAC functionality shared by
> the MCP47FxBy1/2/4/8 I2C and MCP48FxBy1/2/4/8 SPI drivers. The MCP47FEB02
> driver was refactored into two modules: mcp47feb02-core.c and
> mcp47feb02-i2c.c in order to prepare the support for SPI MCP48FxBy1/2/4/8
> DAC family on top of the current implementation.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>

Sashiko makes a point about change of Kconfig symbol and make
oldconfig etc. Not ideal but we've done this before and I don't
think it has proved a signficant problem in practice.  We could
carry an extra symbol to avoid the problem but it is going to
be a long term bit of mess so I think what you have here is the
way to go.

Otherwise a few small things inline.


> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02-core.c
> similarity index 73%
> rename from drivers/iio/dac/mcp47feb02.c
> rename to drivers/iio/dac/mcp47feb02-core.c
> index 7312c2c96141..91fad42df216 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02-core.c

>  				      unsigned int val)
> @@ -440,8 +235,6 @@ static int mcp47feb02_write_to_eeprom(struct mcp47feb02_data *data, unsigned int
>  	 * Wait until the currently occurring EEPROM Write Cycle is completed.
>  	 * Only serial commands to the volatile memory are allowed.
>  	 */
> -	guard(mutex)(&data->lock);
> -

What is this change doing in this patch?  Looking at the code I think
is indeed a good thing to do, just shouldn't be hidden down in here.

Break this out as a separate precursor patch.

>  	ret = regmap_read_poll_timeout(data->regmap, MCP47FEB02_GAIN_CTRL_STATUS_REG_ADDR,
>  				       eewa_val,
>  				       !(eewa_val & MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK),
> @@ -467,6 +260,8 @@ static ssize_t store_eeprom_store(struct device *dev, struct device_attribute *a
>  	if (!state)
>  		return len;
>  
> +	guard(mutex)(&data->lock);
> +
>  	/*
>  	 * Verify DAC Wiper and DAC Configuration are unlocked. If both are disabled,
>  	 * writing to EEPROM is available.
> @@ -575,12 +370,10 @@ static int mcp47feb02_resume(struct device *dev)
>  	guard(mutex)(&data->lock);
>  
>  	for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> -		u8 pd_mode;
>  		u32 mask;
>  		int ret;
>  
>  		data->chdata[ch].powerdown = false;
> -		pd_mode = data->chdata[ch].powerdown_mode + 1;
>  
>  		ret = regmap_write(data->regmap, REG_ADDR(ch), data->chdata[ch].dac_data);
>  		if (ret)
> @@ -588,7 +381,7 @@ static int mcp47feb02_resume(struct device *dev)
>  
>  		mask =  MCP47FEB02_VREF_PD_MASK(ch);
>  		ret = regmap_update_bits(data->regmap, MCP47FEB02_VREF_REG_ADDR,
> -					 mask, field_prep(mask, pd_mode));
> +					 mask, field_prep(mask, data->chdata[ch].ref_mode));
>  		if (ret)
>  			return ret;
>  
> @@ -668,7 +461,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);

Sashiko:
[Severity: Low]
Does this macro export the PM ops symbol without the intended IIO_MCP47FEB02
namespace?

The driver establishes a dedicated namespace IIO_MCP47FEB02 for its core
exported symbols, but mcp47feb02_pm_ops is exported using
EXPORT_SIMPLE_DEV_PM_OPS, which places it into the global namespace.

Should this use EXPORT_NS_SIMPLE_DEV_PM_OPS(mcp47feb02_pm_ops,
mcp47feb02_suspend, mcp47feb02_resume, IIO_MCP47FEB02) instead to
maintain namespace encapsulation?
-
That is a good spot.  Yes, this should use the NS variant.


Thanks,

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>