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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 27 Dec 2025 18:05:43 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> 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>

Sure. Done with the second option.

Jonathan

> 
> > > +};  
>