Re: [PATCH v4 2/2] iio: dac: adding support for Microchip MCP47FEB02

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Dec 27, 2025 at 03:52:45PM +0000, Jonathan Cameron wrote:
> On Tue, 16 Dec 2025 14:05:51 +0200
> Ariana Lazar <ariana.lazar@microchip.com> wrote:

> > This is the iio driver for Microchip MCP47F(E/V)B(0/1/2)1,
> > MCP47F(E/V)B(0/1/2)2, MCP47F(E/V)B(0/1/2)4 and MCP47F(E/V)B(0/1/2)8 series
> > of buffered voltage output Digital-to-Analog Converters with nonvolatile or
> > volatile memory and an I2C Interface.
> > 
> > The families support up to 8 output channels.
> > 
> > The devices can be 8-bit, 10-bit and 12-bit.
> > 
> > Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> Hi Ariana,
> 
> One stale bit of documentation and I'd be surprised if the
> style of text used in Kconfig short help proves sustainable.
> We often end up over time moving to 'x and similar' to avoid
> very complex pattern matching as more and more parts end up supported
> by a given driver.
> 
> With those in mind. Applied to the togreg branch of iio.git which I'll initially
> push out as testing to let 0-day take a poke at it.
> 
> Thanks
> 
> Jonathan
> 
> > diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..c04f3b72b1b1fc303b1bde63c281aade8a67b2f9
> > --- /dev/null
> > +++ b/drivers/iio/dac/mcp47feb02.c
> 
> > +/**
> > + * struct mcp47feb02_data - chip configuration
> > + * @chdata: options configured for each channel on the device
> > + * @lock: prevents concurrent reads/writes to driver's state members
> > + * @chip_features: pointer to features struct
> > + * @scale_1: scales set on channels that are based on Vref1
> > + * @scale: scales set on channels that are based on Vref/Vref0
> > + * @active_channels_mask: enabled channels
> > + * @client: the i2c-client attached to the device
> 
> Not there. I'll tidy this up whilst applying if nothing else
> comes up.
> 
> > + * @regmap: regmap for directly accessing device register
> > + * @vref1_buffered: Vref1 buffer is enabled
> > + * @vref_buffered: Vref/Vref0 buffer is enabled
> > + * @phys_channels: physical channels on the device
> > + * @labels: table with channels labels
> > + * @use_vref1: vref1-supply is defined
> > + * @use_vref: vref-supply is defined
> > + */
> > +struct mcp47feb02_data {
> > +	struct mcp47feb02_channel_data chdata[MCP47FEB02_MAX_CH];
> > +	struct mutex lock; /* prevents concurrent reads/writes to driver's state members */
> > +	const struct mcp47feb02_features *chip_features;
> > +	int scale_1[2 * MCP47FEB02_MAX_SCALES_CH];
> > +	int scale[2 * MCP47FEB02_MAX_SCALES_CH];
> > +	unsigned long active_channels_mask;
> > +	struct regmap *regmap;
> > +	bool vref1_buffered;
> > +	bool vref_buffered;
> > +	u16 phys_channels;
> > +	const char *labels[MCP47FEB02_MAX_CH];
> > +	bool use_vref1;
> > +	bool use_vref;

I would group slightly different these:

	...
	const char *labels[MCP47FEB02_MAX_CH];
	bool vref1_buffered;
	bool vref_buffered;
	bool use_vref1;
	bool use_vref;
	u16 phys_channels;

With this the vref grouped together and not split. A possible variant:

	const char *labels[MCP47FEB02_MAX_CH];
	u16 phys_channels;
	bool vref1_buffered;
	bool vref_buffered;
	bool use_vref1;
	bool use_vref;

Jonathan, can you also tweak this?

And I think it's worth to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> > +};

-- 
With Best Regards,
Andy Shevchenko