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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 8 Aug 2021 17:22:57 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Mugilraj Dhavachelvan <dmugil2000@...il.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>, Dragos.Bogdan@...log.com,
        Darius.Berghe@...log.com, Rob Herring <robh+dt@...nel.org>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Chris Packham <chris.packham@...iedtelesis.co.nz>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: potentiometer: Add driver support for AD5110

On Sat, 7 Aug 2021 23:04:18 +0530
Mugilraj Dhavachelvan <dmugil2000@...il.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@...il.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!!
> >