Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 7 Aug 2021 23:04:18 +0530
Mugilraj Dhavachelvan <dmugil2000@gmail.com> wrote:

> Sorry some formatting issues happened in my previous mail.
> 
> On 07/08/21 10:56 pm, Mugilraj Dhavachelvan wrote:
> > 
> > 
> > On 07/08/21 5:41 pm, Lars-Peter Clausen wrote:  
> >> On 8/7/21 7:08 AM, Mugilraj Dhavachelvan wrote:  
> >>> The AD5110/AD5112/AD5114 provide a nonvolatile solution
> >>> for 128-/64-/32-position adjustment applications, offering
> >>> guaranteed low resistor tolerance errors of ±8% and up to
> >>> ±6 mA current density.
> >>>
> >>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf
> >>> Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com>  
> >>
> >> Thanks for the patch. This looks really good> 
> >>  
>

...

> >>> +
> >>> +static IIO_DEVICE_ATTR(wiper_pos_eeprom, 0644,
> >>> +               ad5110_eeprom_read,
> >>> +               ad5110_eeprom_write, 0);  
> >> This is new custom ABI and needs to be documented  

We have existing similar ABI in dac/mcp4725 which is simply
called store_eeprom

It's in the main docs
Documentation/ABI/testing/sysfs-bus-iio as storing
device configuration.  I'm guessing this device doesn't have
other configuration so that description will work?


> 
> > I'm not aware of this, fixed in v2.  
> >>> +static int ad5110_write_raw(struct iio_dev *indio_dev,
> >>> +                struct iio_chan_spec const *chan,
> >>> +                int val, int val2, long mask)
> >>> +{
> >>> +    struct ad5110_data *data = iio_priv(indio_dev);
> >>> +    int ret;
> >>> +
> >>> +    switch (mask) {
> >>> +    case IIO_CHAN_INFO_RAW:
> >>> +        if (val >= data->cfg->max_pos || val < 0)  
> >> val == data->cfg->max_pos is a valid setting. Writing max_pos puts it in top-scale mode which gives maximum resistance.  
> > Fixed in v2.  
> >>> +            return -EINVAL;
> >>> +
> >>> +        return ad5110_write(data, AD5110_RDAC_WR, val << data->cfg->shift);
> >>> +    case IIO_CHAN_INFO_ENABLE:
> >>> +        if (val < 0 || val > 1)
> >>> +            return -EINVAL;
> >>> +        if (data->enable == val)
> >>> +            return 0;
> >>> +        ret = ad5110_write(data, AD5110_SHUTDOWN, val);  
> >> Doesn't val have to be inverted to get the right behavior  
> 
> > I just replicated the datasheet operation. 
> > You mean,
> >  1 - shutdown off
> >  0 - shutdown on
> > If yes, then the user won't get confused with the datasheet and the behavior of the driver?
> > Or Is it work like this? But yeah even I like this change it's more convenient.  

ABI has to be consistent and writing an enable attribute with 1 has to mean enabling it whatever
approach the datasheet takes to describe things.  Most users don't read
datasheets so interface needs to be intuitive.

> >>> +        if (ret)
> >>> +            return ret;
> >>> +        data->enable = val;
> >>> +        return 0;
> >>> +    default:
> >>> +        return -EINVAL;
> >>> +    }
> >>> +}  
> >> [...]  
> > Thanks for feedback!!
> >