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