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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 23 Jul 2023 07:09:11 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> Hi Jonathan,
> 
> On 7/20/23 21:13, Jonathan Cameron wrote:
> >> +static const char *const mcp4728_vref_modes[] = {
> >> +	"vdd_ext",
> >> +	"internal",
> >> +};
> >> +
> >> +static int mcp4728_get_vref_mode(struct iio_dev *indio_dev,
> >> +				 const struct iio_chan_spec *chan)
> >> +{
> >> +	struct mcp4728_data *data = iio_priv(indio_dev);
> >> +
> >> +	return data->channel_data[chan->channel].ref_mode;
> >> +}
> >> +
> >> +static int mcp4728_set_vref_mode(struct iio_dev *indio_dev,
> >> +				 const struct iio_chan_spec *chan,
> >> +				 unsigned int mode)
> >> +{
> >> +	struct mcp4728_data *data = iio_priv(indio_dev);
> >> +	int ret;
> >> +
> >> +	data->channel_data[chan->channel].ref_mode = mode;
> >> +
> >> +	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.

> 
> - Finally using the same approach of https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/mcp4725.c#L462 where after having  synced current parameters stored in EEPROM they are updated with the ones specified in dts.
> 
From a quick look those are not coming from DTS and are not in the dt-binding for that
device. The parameters only come from legacy platform data (or the wild west of early IIO).

Only the presence of vref and whether to buffer it are in DT binding
https://elixir.bootlin.com/linux/latest/source/drivers/iio/dac/mcp4725.c#L373

We should probably rip the remaining platform data based probe paths out of drivers
sometime soon. No one uses them any more (there are better paths even if people are
using non DT based board files) and they can mislead because the rules weren't
as tight as they are for what goes in a DT binding.

Jonathan


> 
> Best regards
> 
>                        Andrea
> 
>