Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Jonathan Cameron]

On Sun, 23 Jul 2023 14:15:10 +0200
Andrea Collamati <andrea.collamati@xxxxxxxxx> wrote:

> On 7/23/23 13:41, Jonathan Cameron wrote:
> >>>> +
> >>>> +	if (mode == MCP4728_VREF_EXTERNAL_VDD &&
> >>>> +	    data->channel_data[chan->channel].g_mode == MCP4728_GAIN_X2) {
> >>>> +		dev_warn(&data->client->dev,
> >>>> +			 "CH%d: Gain x2 not effective when vref is vdd, force to x1",
> >>>> +			 chan->channel);    
> >>> Even better if you don't present the option at all and wrap it up in the
> >>> standard ABI of _scale
> >>>    
> >> I think that the solution could be:
> >>
> >> - Removing custom ABI (vref/gain)
> >>
> >> - Initialize them at device tree level using two 4-elements arrays.  
> > If doing with device tree, they should reflect something that is a characteristic
> > of how the chips is wired up.  So you would need to explain why that is the case here.
> >
> > However, I'm still not understanding why _SCALE is not appropriate here.  We have
> > a small set of options with well defined scales.  
> 
> SCALE is appropriate. I didn't know that scale_available was a standard ABI.
> 
> I will follow  the implementation done n https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/ad5592r-base.c#L487

That's long out of date wrt to best practice.  Instead follow.
drivers/iio/dac/ad7293.c or any of the other drivers that set
.info_mask_shared_by_*_available = BIT(IIO_CHAN_INFO_SCALE)
and provide the read_avail() callback.

The advantage that brings is that consumer drivers can then
access this information much more simply.

One day we will hopefully finish converting old drivers over to
newer interfaces, but it's slow progress!

Jonathan