* [PATCH v2 0/2] iio: potentiometer: Add driver support for AD5110 @ 2021-08-09 7:57 Mugilraj Dhavachelvan 2021-08-09 7:57 ` [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices Mugilraj Dhavachelvan 2021-08-09 7:57 ` [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 0 siblings, 2 replies; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-09 7:57 UTC (permalink / raw) To: Dragos.Bogdan, Darius.Berghe, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Mugilraj Dhavachelvan, Guenter Roeck, Chris Packham, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio Add dt-bindings and driver support for AD5110, a Nonvolatile Digital Potentiometer. Changes since v1: - Drop 'shared_by_type' since it's a single channel - Add error check for i2c read/write data bytes - Simplified calculation for tolerance - Add shift for eeprom wiper pos read - Change new custom ABI to existing ABI - Allow top-scale mode by writting max_pos to val Mugilraj Dhavachelvan (2): dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices iio: potentiometer: Add driver support for AD5110 .../devicetree/bindings/trivial-devices.yaml | 2 + MAINTAINERS | 6 + drivers/iio/potentiometer/Kconfig | 10 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ad5110.c | 339 ++++++++++++++++++ 5 files changed, 358 insertions(+) create mode 100644 drivers/iio/potentiometer/ad5110.c -- 2.25.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices 2021-08-09 7:57 [PATCH v2 0/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan @ 2021-08-09 7:57 ` Mugilraj Dhavachelvan 2021-08-13 20:57 ` Rob Herring 2021-08-09 7:57 ` [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 1 sibling, 1 reply; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-09 7:57 UTC (permalink / raw) To: Dragos.Bogdan, Darius.Berghe, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Mugilraj Dhavachelvan, Michael Hennerich, Guenter Roeck, Chris Packham, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio Add AD5110, a Nonvolatile Digital Potentiometer into trivial-devices.yaml. Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com> --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index 8341e9d23c1e..55c8a6ce5057 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -33,6 +33,8 @@ properties: - ad,ad7414 # ADM9240: Complete System Hardware Monitor for uProcessor-Based Systems - ad,adm9240 + # AD5110 - Nonvolatile Digital Potentiometer + - adi,ad5110 # Analog Devices ADP5585 Keypad Decoder and I/O Expansion - adi,adp5585 # Analog Devices ADP5585 Keypad Decoder and I/O Expansion with support for Row5 -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices 2021-08-09 7:57 ` [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices Mugilraj Dhavachelvan @ 2021-08-13 20:57 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-08-13 20:57 UTC (permalink / raw) To: Mugilraj Dhavachelvan Cc: linux-iio, Dragos.Bogdan, Rob Herring, devicetree, linux-kernel, Lars-Peter Clausen, Krzysztof Kozlowski, Guenter Roeck, Alexandru Ardelean, Chris Packham, Jonathan Cameron, Alexandre Belloni, Darius.Berghe, Michael Hennerich On Mon, 09 Aug 2021 13:27:19 +0530, Mugilraj Dhavachelvan wrote: > Add AD5110, a Nonvolatile Digital Potentiometer into > trivial-devices.yaml. > > Signed-off-by: Mugilraj Dhavachelvan <dmugil2000@gmail.com> > --- > Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-09 7:57 [PATCH v2 0/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 2021-08-09 7:57 ` [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices Mugilraj Dhavachelvan @ 2021-08-09 7:57 ` Mugilraj Dhavachelvan 2021-08-09 20:05 ` Jonathan Cameron 2021-08-10 12:49 ` Andy Shevchenko 1 sibling, 2 replies; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-09 7:57 UTC (permalink / raw) To: Dragos.Bogdan, Darius.Berghe, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Mugilraj Dhavachelvan, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio 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> --- MAINTAINERS | 6 + drivers/iio/potentiometer/Kconfig | 10 + drivers/iio/potentiometer/Makefile | 1 + drivers/iio/potentiometer/ad5110.c | 339 +++++++++++++++++++++++++++++ 4 files changed, 356 insertions(+) create mode 100644 drivers/iio/potentiometer/ad5110.c diff --git a/MAINTAINERS b/MAINTAINERS index bd7aff0c120f..cf6e73b87b6e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -452,6 +452,12 @@ S: Maintained W: https://parisc.wiki.kernel.org/index.php/AD1889 F: sound/pci/ad1889.* +AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER +M: Mugilraj Dhavachelvan <dmugil2000@gmail.com> +L: linux-iio@vger.kernel.org +S: Supported +F: drivers/iio/potentiometer/ad5110.c + AD525X ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER M: Michael Hennerich <michael.hennerich@analog.com> S: Supported diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig index 4cac0173db8b..832df8da2bc6 100644 --- a/drivers/iio/potentiometer/Kconfig +++ b/drivers/iio/potentiometer/Kconfig @@ -6,6 +6,16 @@ menu "Digital potentiometers" +config AD5110 + tristate "Analog Devices AD5110 and similar Digital Potentiometer driver" + depends on I2C + help + Say yes here to build support for the Analog Devices AD5110, AD5112 + and AD5114 digital potentiometer chip. + + To compile this driver as a module, choose M here: the + module will be called ad5110. + config AD5272 tristate "Analog Devices AD5272 and similar Digital Potentiometer driver" depends on I2C diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile index 091adf3cdd0b..5ebb8e3bbd76 100644 --- a/drivers/iio/potentiometer/Makefile +++ b/drivers/iio/potentiometer/Makefile @@ -4,6 +4,7 @@ # # When adding new entries keep the list in alphabetical order +obj-$(CONFIG_AD5110) += ad5110.o obj-$(CONFIG_AD5272) += ad5272.o obj-$(CONFIG_DS1803) += ds1803.o obj-$(CONFIG_MAX5432) += max5432.o diff --git a/drivers/iio/potentiometer/ad5110.c b/drivers/iio/potentiometer/ad5110.c new file mode 100644 index 000000000000..4ba76e968edb --- /dev/null +++ b/drivers/iio/potentiometer/ad5110.c @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Analog Devices AD5110 digital potentiometer driver + * + * Copyright (C) 2021 Mugilraj Dhavachelvan <dmugil2000@gmail.com> + * + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf + * + */ + +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/module.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +/* AD5110 commands */ +#define AD5110_EEPROM_WR 1 +#define AD5110_RDAC_WR 2 +#define AD5110_SHUTDOWN 3 +#define AD5110_RESET 4 +#define AD5110_RDAC_RD 5 +#define AD5110_EEPROM_RD 6 + +/* AD5110_EEPROM_RD data */ +#define AD5110_WIPER_POS 0 +#define AD5110_RESISTOR_TOL 1 + +#define WIPER_RESISTANCE 70 + +struct ad5110_cfg { + int max_pos; + int kohms; + int shift; +}; + +enum ad5110_type { + AD5110_10, + AD5110_80, + AD5112_05, + AD5112_10, + AD5112_80, + AD5114_10, + AD5114_80, +}; + +static const struct ad5110_cfg ad5110_cfg[] = { + [AD5110_10] = { .max_pos = 128, .kohms = 10 }, + [AD5110_80] = { .max_pos = 128, .kohms = 80 }, + [AD5112_05] = { .max_pos = 64, .kohms = 5, .shift = 1 }, + [AD5112_10] = { .max_pos = 64, .kohms = 10, .shift = 1 }, + [AD5112_80] = { .max_pos = 64, .kohms = 80, .shift = 1 }, + [AD5114_10] = { .max_pos = 32, .kohms = 10, .shift = 2 }, + [AD5114_80] = { .max_pos = 32, .kohms = 80, .shift = 2 }, +}; + +struct ad5110_data { + struct i2c_client *client; + s16 tol; /* resistor tolerance */ + bool enable; + struct mutex lock; + const struct ad5110_cfg *cfg; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + u8 buf[2] ____cacheline_aligned; +}; + +static const struct iio_chan_spec ad5110_channels[] = { + { + .type = IIO_RESISTANCE, + .output = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), + } +}; + +static int ad5110_read(struct ad5110_data *data, u8 cmd, int *val) +{ + int ret; + + mutex_lock(&data->lock); + data->buf[0] = cmd; + data->buf[1] = *val; + + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); + if (ret != sizeof(data->buf)) { + ret = -EIO; + goto error; + } + + ret = i2c_master_recv_dmasafe(data->client, data->buf, 1); + if (ret != 1) { + ret = -EIO; + goto error; + } + + *val = data->buf[0]; + ret = 0; + +error: + mutex_unlock(&data->lock); + return ret; +} + +static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val) +{ + int ret; + + mutex_lock(&data->lock); + data->buf[0] = cmd; + data->buf[1] = val; + + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); + if (ret != sizeof(data->buf)) + ret = -EIO; + + mutex_unlock(&data->lock); + + return ret < 0 ? ret : 0; +} + +static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val) +{ + int ret; + + ret = ad5110_read(data, cmd, &val); + if (ret) + return ret; + + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; + if (!(val & BIT(7))) + data->tol *= -1; + + return 0; +} + +static ssize_t ad5110_eeprom_read(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct ad5110_data *data = iio_priv(indio_dev); + int val = AD5110_WIPER_POS; + int ret; + + ret = ad5110_read(data, AD5110_EEPROM_RD, &val); + if (ret) + return ret; + + val = val >> data->cfg->shift; + return iio_format_value(buf, IIO_VAL_INT, 1, &val); +} + +static ssize_t ad5110_eeprom_write(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct ad5110_data *data = iio_priv(indio_dev); + int ret; + + ret = ad5110_write(data, AD5110_EEPROM_WR, 0); + if (ret) { + dev_err(&data->client->dev, "RDAC to EEPROM write failed\n"); + return ret; + } + msleep(20); + + return len; +} + +static IIO_DEVICE_ATTR(store_eeprom, 0644, + ad5110_eeprom_read, + ad5110_eeprom_write, 0); + +static struct attribute *ad5110_attributes[] = { + &iio_dev_attr_store_eeprom.dev_attr.attr, + NULL, +}; + +static const struct attribute_group ad5110_attribute_group = { + .attrs = ad5110_attributes, +}; + +static int ad5110_read_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: + ret = ad5110_read(data, AD5110_RDAC_RD, val); + if (ret) + return ret; + + *val = *val >> data->cfg->shift; + return IIO_VAL_INT; + case IIO_CHAN_INFO_OFFSET: + *val = WIPER_RESISTANCE * data->cfg->max_pos; + *val2 = 1000 * data->cfg->kohms + data->tol; + return IIO_VAL_FRACTIONAL; + case IIO_CHAN_INFO_SCALE: + *val = 1000 * data->cfg->kohms + data->tol; + *val2 = data->cfg->max_pos; + return IIO_VAL_FRACTIONAL; + case IIO_CHAN_INFO_ENABLE: + *val = data->enable; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +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) + 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 ? 0 : 1); + if (ret) + return ret; + data->enable = val; + return 0; + default: + return -EINVAL; + } +} + +static const struct iio_info ad5110_info = { + .read_raw = ad5110_read_raw, + .write_raw = ad5110_write_raw, + .attrs = &ad5110_attribute_group, +}; + +#define AD5110_COMPATIBLE(of_compatible, cfg) { \ + .compatible = of_compatible, \ + .data = &ad5110_cfg[cfg], \ +} + +static const struct of_device_id ad5110_of_match[] = { + AD5110_COMPATIBLE("adi,ad5110-10", AD5110_10), + AD5110_COMPATIBLE("adi,ad5110-80", AD5110_80), + AD5110_COMPATIBLE("adi,ad5112-05", AD5112_05), + AD5110_COMPATIBLE("adi,ad5112-10", AD5112_10), + AD5110_COMPATIBLE("adi,ad5112-80", AD5112_80), + AD5110_COMPATIBLE("adi,ad5114-10", AD5114_10), + AD5110_COMPATIBLE("adi,ad5114-80", AD5114_80), + { } +}; +MODULE_DEVICE_TABLE(of, ad5110_of_match); + +static const struct i2c_device_id ad5110_id[] = { + { "ad5110-10", AD5110_10 }, + { "ad5110-80", AD5110_80 }, + { "ad5112-05", AD5112_05 }, + { "ad5112-10", AD5112_10 }, + { "ad5112-80", AD5112_80 }, + { "ad5114-10", AD5114_10 }, + { "ad5114-80", AD5114_80 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, ad5110_id); + +static int ad5110_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct iio_dev *indio_dev; + struct ad5110_data *data; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + data->client = client; + mutex_init(&data->lock); + data->enable = 1; + data->cfg = device_get_match_data(dev); + if (!data->cfg) + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; + + /* refresh RDAC register with EEPROM */ + ret = ad5110_write(data, AD5110_RESET, 0); + if (ret) { + dev_err(dev, "Refresh RDAC with EEPROM failed\n"); + return ret; + } + + ret = ad5110_resistor_tol(data, AD5110_EEPROM_RD, AD5110_RESISTOR_TOL); + if (ret) { + dev_err(dev, "Read resistor tolerance failed\n"); + return ret; + } + + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &ad5110_info; + indio_dev->channels = ad5110_channels; + indio_dev->num_channels = ARRAY_SIZE(ad5110_channels); + indio_dev->name = client->name; + + return devm_iio_device_register(dev, indio_dev); +} + +static struct i2c_driver ad5110_driver = { + .driver = { + .name = "ad5110", + .of_match_table = ad5110_of_match, + }, + .probe_new = ad5110_probe, + .id_table = ad5110_id, +}; + +module_i2c_driver(ad5110_driver); + +MODULE_AUTHOR("Mugilraj Dhavachelvan <dmugil2000@gmail.com>"); +MODULE_DESCRIPTION("AD5110 digital potentiometer"); +MODULE_LICENSE("GPL v2"); + -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-09 7:57 ` [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan @ 2021-08-09 20:05 ` Jonathan Cameron 2021-08-10 12:49 ` Andy Shevchenko 1 sibling, 0 replies; 14+ messages in thread From: Jonathan Cameron @ 2021-08-09 20:05 UTC (permalink / raw) To: Mugilraj Dhavachelvan Cc: Dragos.Bogdan, Darius.Berghe, Rob Herring, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio On Mon, 9 Aug 2021 13:27:20 +0530 Mugilraj Dhavachelvan <dmugil2000@gmail.com> 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> Looks good to me, but I'll leave it on the list for a little while to give time for others to take another look. Thanks, Jonathan > --- > MAINTAINERS | 6 + > drivers/iio/potentiometer/Kconfig | 10 + > drivers/iio/potentiometer/Makefile | 1 + > drivers/iio/potentiometer/ad5110.c | 339 +++++++++++++++++++++++++++++ > 4 files changed, 356 insertions(+) > create mode 100644 drivers/iio/potentiometer/ad5110.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bd7aff0c120f..cf6e73b87b6e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -452,6 +452,12 @@ S: Maintained > W: https://parisc.wiki.kernel.org/index.php/AD1889 > F: sound/pci/ad1889.* > > +AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER > +M: Mugilraj Dhavachelvan <dmugil2000@gmail.com> > +L: linux-iio@vger.kernel.org > +S: Supported > +F: drivers/iio/potentiometer/ad5110.c > + > AD525X ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER > M: Michael Hennerich <michael.hennerich@analog.com> > S: Supported > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig > index 4cac0173db8b..832df8da2bc6 100644 > --- a/drivers/iio/potentiometer/Kconfig > +++ b/drivers/iio/potentiometer/Kconfig > @@ -6,6 +6,16 @@ > > menu "Digital potentiometers" > > +config AD5110 > + tristate "Analog Devices AD5110 and similar Digital Potentiometer driver" > + depends on I2C > + help > + Say yes here to build support for the Analog Devices AD5110, AD5112 > + and AD5114 digital potentiometer chip. > + > + To compile this driver as a module, choose M here: the > + module will be called ad5110. > + > config AD5272 > tristate "Analog Devices AD5272 and similar Digital Potentiometer driver" > depends on I2C > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile > index 091adf3cdd0b..5ebb8e3bbd76 100644 > --- a/drivers/iio/potentiometer/Makefile > +++ b/drivers/iio/potentiometer/Makefile > @@ -4,6 +4,7 @@ > # > > # When adding new entries keep the list in alphabetical order > +obj-$(CONFIG_AD5110) += ad5110.o > obj-$(CONFIG_AD5272) += ad5272.o > obj-$(CONFIG_DS1803) += ds1803.o > obj-$(CONFIG_MAX5432) += max5432.o > diff --git a/drivers/iio/potentiometer/ad5110.c b/drivers/iio/potentiometer/ad5110.c > new file mode 100644 > index 000000000000..4ba76e968edb > --- /dev/null > +++ b/drivers/iio/potentiometer/ad5110.c > @@ -0,0 +1,339 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Analog Devices AD5110 digital potentiometer driver > + * > + * Copyright (C) 2021 Mugilraj Dhavachelvan <dmugil2000@gmail.com> > + * > + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf > + * > + */ > + > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > +/* AD5110 commands */ > +#define AD5110_EEPROM_WR 1 > +#define AD5110_RDAC_WR 2 > +#define AD5110_SHUTDOWN 3 > +#define AD5110_RESET 4 > +#define AD5110_RDAC_RD 5 > +#define AD5110_EEPROM_RD 6 > + > +/* AD5110_EEPROM_RD data */ > +#define AD5110_WIPER_POS 0 > +#define AD5110_RESISTOR_TOL 1 > + > +#define WIPER_RESISTANCE 70 > + > +struct ad5110_cfg { > + int max_pos; > + int kohms; > + int shift; > +}; > + > +enum ad5110_type { > + AD5110_10, > + AD5110_80, > + AD5112_05, > + AD5112_10, > + AD5112_80, > + AD5114_10, > + AD5114_80, > +}; > + > +static const struct ad5110_cfg ad5110_cfg[] = { > + [AD5110_10] = { .max_pos = 128, .kohms = 10 }, > + [AD5110_80] = { .max_pos = 128, .kohms = 80 }, > + [AD5112_05] = { .max_pos = 64, .kohms = 5, .shift = 1 }, > + [AD5112_10] = { .max_pos = 64, .kohms = 10, .shift = 1 }, > + [AD5112_80] = { .max_pos = 64, .kohms = 80, .shift = 1 }, > + [AD5114_10] = { .max_pos = 32, .kohms = 10, .shift = 2 }, > + [AD5114_80] = { .max_pos = 32, .kohms = 80, .shift = 2 }, > +}; > + > +struct ad5110_data { > + struct i2c_client *client; > + s16 tol; /* resistor tolerance */ > + bool enable; > + struct mutex lock; > + const struct ad5110_cfg *cfg; > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ > + u8 buf[2] ____cacheline_aligned; > +}; > + > +static const struct iio_chan_spec ad5110_channels[] = { > + { > + .type = IIO_RESISTANCE, > + .output = 1, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > + } > +}; > + > +static int ad5110_read(struct ad5110_data *data, u8 cmd, int *val) > +{ > + int ret; > + > + mutex_lock(&data->lock); > + data->buf[0] = cmd; > + data->buf[1] = *val; > + > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > + if (ret != sizeof(data->buf)) { > + ret = -EIO; > + goto error; > + } > + > + ret = i2c_master_recv_dmasafe(data->client, data->buf, 1); > + if (ret != 1) { > + ret = -EIO; > + goto error; > + } > + > + *val = data->buf[0]; > + ret = 0; > + > +error: > + mutex_unlock(&data->lock); > + return ret; > +} > + > +static int ad5110_write(struct ad5110_data *data, u8 cmd, u8 val) > +{ > + int ret; > + > + mutex_lock(&data->lock); > + data->buf[0] = cmd; > + data->buf[1] = val; > + > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > + if (ret != sizeof(data->buf)) > + ret = -EIO; > + > + mutex_unlock(&data->lock); > + > + return ret < 0 ? ret : 0; > +} > + > +static int ad5110_resistor_tol(struct ad5110_data *data, u8 cmd, int val) > +{ > + int ret; > + > + ret = ad5110_read(data, cmd, &val); > + if (ret) > + return ret; > + > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > + if (!(val & BIT(7))) > + data->tol *= -1; > + > + return 0; > +} > + > +static ssize_t ad5110_eeprom_read(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct ad5110_data *data = iio_priv(indio_dev); > + int val = AD5110_WIPER_POS; > + int ret; > + > + ret = ad5110_read(data, AD5110_EEPROM_RD, &val); > + if (ret) > + return ret; > + > + val = val >> data->cfg->shift; > + return iio_format_value(buf, IIO_VAL_INT, 1, &val); > +} > + > +static ssize_t ad5110_eeprom_write(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct ad5110_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = ad5110_write(data, AD5110_EEPROM_WR, 0); > + if (ret) { > + dev_err(&data->client->dev, "RDAC to EEPROM write failed\n"); > + return ret; > + } > + msleep(20); > + > + return len; > +} > + > +static IIO_DEVICE_ATTR(store_eeprom, 0644, > + ad5110_eeprom_read, > + ad5110_eeprom_write, 0); > + > +static struct attribute *ad5110_attributes[] = { > + &iio_dev_attr_store_eeprom.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group ad5110_attribute_group = { > + .attrs = ad5110_attributes, > +}; > + > +static int ad5110_read_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: > + ret = ad5110_read(data, AD5110_RDAC_RD, val); > + if (ret) > + return ret; > + > + *val = *val >> data->cfg->shift; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_OFFSET: > + *val = WIPER_RESISTANCE * data->cfg->max_pos; > + *val2 = 1000 * data->cfg->kohms + data->tol; > + return IIO_VAL_FRACTIONAL; > + case IIO_CHAN_INFO_SCALE: > + *val = 1000 * data->cfg->kohms + data->tol; > + *val2 = data->cfg->max_pos; > + return IIO_VAL_FRACTIONAL; > + case IIO_CHAN_INFO_ENABLE: > + *val = data->enable; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +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) > + 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 ? 0 : 1); > + if (ret) > + return ret; > + data->enable = val; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_info ad5110_info = { > + .read_raw = ad5110_read_raw, > + .write_raw = ad5110_write_raw, > + .attrs = &ad5110_attribute_group, > +}; > + > +#define AD5110_COMPATIBLE(of_compatible, cfg) { \ > + .compatible = of_compatible, \ > + .data = &ad5110_cfg[cfg], \ > +} > + > +static const struct of_device_id ad5110_of_match[] = { > + AD5110_COMPATIBLE("adi,ad5110-10", AD5110_10), > + AD5110_COMPATIBLE("adi,ad5110-80", AD5110_80), > + AD5110_COMPATIBLE("adi,ad5112-05", AD5112_05), > + AD5110_COMPATIBLE("adi,ad5112-10", AD5112_10), > + AD5110_COMPATIBLE("adi,ad5112-80", AD5112_80), > + AD5110_COMPATIBLE("adi,ad5114-10", AD5114_10), > + AD5110_COMPATIBLE("adi,ad5114-80", AD5114_80), > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad5110_of_match); > + > +static const struct i2c_device_id ad5110_id[] = { > + { "ad5110-10", AD5110_10 }, > + { "ad5110-80", AD5110_80 }, > + { "ad5112-05", AD5112_05 }, > + { "ad5112-10", AD5112_10 }, > + { "ad5112-80", AD5112_80 }, > + { "ad5114-10", AD5114_10 }, > + { "ad5114-80", AD5114_80 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, ad5110_id); > + > +static int ad5110_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct iio_dev *indio_dev; > + struct ad5110_data *data; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + data->client = client; > + mutex_init(&data->lock); > + data->enable = 1; > + data->cfg = device_get_match_data(dev); > + if (!data->cfg) > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; > + > + /* refresh RDAC register with EEPROM */ > + ret = ad5110_write(data, AD5110_RESET, 0); > + if (ret) { > + dev_err(dev, "Refresh RDAC with EEPROM failed\n"); > + return ret; > + } > + > + ret = ad5110_resistor_tol(data, AD5110_EEPROM_RD, AD5110_RESISTOR_TOL); > + if (ret) { > + dev_err(dev, "Read resistor tolerance failed\n"); > + return ret; > + } > + > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &ad5110_info; > + indio_dev->channels = ad5110_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad5110_channels); > + indio_dev->name = client->name; > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static struct i2c_driver ad5110_driver = { > + .driver = { > + .name = "ad5110", > + .of_match_table = ad5110_of_match, > + }, > + .probe_new = ad5110_probe, > + .id_table = ad5110_id, > +}; > + > +module_i2c_driver(ad5110_driver); > + > +MODULE_AUTHOR("Mugilraj Dhavachelvan <dmugil2000@gmail.com>"); > +MODULE_DESCRIPTION("AD5110 digital potentiometer"); > +MODULE_LICENSE("GPL v2"); > + ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-09 7:57 ` [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 2021-08-09 20:05 ` Jonathan Cameron @ 2021-08-10 12:49 ` Andy Shevchenko 2021-08-11 5:45 ` Mugilraj Dhavachelvan 2021-08-11 8:15 ` Lars-Peter Clausen 1 sibling, 2 replies; 14+ messages in thread From: Andy Shevchenko @ 2021-08-10 12:49 UTC (permalink / raw) To: Mugilraj Dhavachelvan Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan <dmugil2000@gmail.com> 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. ... > +/* > + * Analog Devices AD5110 digital potentiometer driver > + * > + * Copyright (C) 2021 Mugilraj Dhavachelvan <dmugil2000@gmail.com> > + * > + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf > + * Redundant blank line. > + */ ... > +#define WIPER_RESISTANCE 70 Missed prefix? ... > +static const struct iio_chan_spec ad5110_channels[] = { > + { > + .type = IIO_RESISTANCE, > + .output = 1, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > + } + comma. > +}; ... > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > + if (ret != sizeof(data->buf)) { > + ret = -EIO; Shadowed error code when ret < 0. > + goto error; > + } ... > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > + if (ret != sizeof(data->buf)) > + ret = -EIO; Ditto. > + mutex_unlock(&data->lock); > + > + return ret < 0 ? ret : 0; > +} ... > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > + if (!(val & BIT(7))) > + data->tol *= -1; Shouldn't you simple use corresponding sign_extend*()? ... > + ret = ad5110_write(data, AD5110_EEPROM_WR, 0); > + if (ret) { > + dev_err(&data->client->dev, "RDAC to EEPROM write failed\n"); > + return ret; > + } > + msleep(20); Each long sleeps like this must be explained. ... > +static IIO_DEVICE_ATTR(store_eeprom, 0644, > + ad5110_eeprom_read, > + ad5110_eeprom_write, 0); IIO_DEVICE_ATTR_RW() ? ... > +static struct attribute *ad5110_attributes[] = { > + &iio_dev_attr_store_eeprom.dev_attr.attr, > + NULL, No comma for a terminator line. > +}; ... > + data->cfg = device_get_match_data(dev); > + if (!data->cfg) > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; Not sure this is not a dead code since you are using ->probe_new(). ... > +static struct i2c_driver ad5110_driver = { > + .driver = { > + .name = "ad5110", > + .of_match_table = ad5110_of_match, > + }, > + .probe_new = ad5110_probe, > + .id_table = ad5110_id, > +}; > + Redundant blank line. > +module_i2c_driver(ad5110_driver); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-10 12:49 ` Andy Shevchenko @ 2021-08-11 5:45 ` Mugilraj Dhavachelvan 2021-08-11 7:03 ` Andy Shevchenko 2021-08-11 8:15 ` Lars-Peter Clausen 1 sibling, 1 reply; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-11 5:45 UTC (permalink / raw) To: Andy Shevchenko Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, devicetree, linux-kernel, linux-iio On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote: > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan > <dmugil2000@gmail.com> 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. > > ... > > > +/* > > + * Analog Devices AD5110 digital potentiometer driver > > + * > > + * Copyright (C) 2021 Mugilraj Dhavachelvan <dmugil2000@gmail.com> > > + * > > + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf > > > + * > > Redundant blank line. > fixed in v3 > > + */ > > ... > > > +#define WIPER_RESISTANCE 70 > > Missed prefix? > fixed in v3 > ... > > > +static const struct iio_chan_spec ad5110_channels[] = { > > + { > > + .type = IIO_RESISTANCE, > > + .output = 1, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) | > > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE), > > + } > > + comma. > fixed in v3 > > +}; > > ... > > > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > > + if (ret != sizeof(data->buf)) { > > > + ret = -EIO; > > Shadowed error code when ret < 0. > fixed in v3 > > + goto error; > > + } > > ... > > > + ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf)); > > + if (ret != sizeof(data->buf)) > > + ret = -EIO; > > Ditto. > fixed in v3 > > + mutex_unlock(&data->lock); > > + > > + return ret < 0 ? ret : 0; > > +} > > ... > > > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > + if (!(val & BIT(7))) > > + data->tol *= -1; > > Shouldn't you simple use corresponding sign_extend*()? > I'm not able see any sign_extend for 16 bit. Is there any other way? > ... > > > + ret = ad5110_write(data, AD5110_EEPROM_WR, 0); > > + if (ret) { > > + dev_err(&data->client->dev, "RDAC to EEPROM write failed\n"); > > + return ret; > > + } > > > > + msleep(20); > > Each long sleeps like this must be explained. > fixed in v3 > ... > > > +static IIO_DEVICE_ATTR(store_eeprom, 0644, > > + ad5110_eeprom_read, > > + ad5110_eeprom_write, 0); > > IIO_DEVICE_ATTR_RW() ? > fixed in v3 > ... > > > +static struct attribute *ad5110_attributes[] = { > > + &iio_dev_attr_store_eeprom.dev_attr.attr, > > + NULL, > > No comma for a terminator line. > fixed in v3 > > +}; > > ... > > > + data->cfg = device_get_match_data(dev); > > > + if (!data->cfg) > > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; > > Not sure this is not a dead code since you are using ->probe_new(). > Even I'm suspecting that and also removing id_table. But I'm not sure of it so just left as it is. > ... > > > +static struct i2c_driver ad5110_driver = { > > + .driver = { > > + .name = "ad5110", > > + .of_match_table = ad5110_of_match, > > + }, > > + .probe_new = ad5110_probe, > > + .id_table = ad5110_id, > > +}; > > > + > > Redundant blank line fixed in v3. > > > +module_i2c_driver(ad5110_driver); > Thanks for the feedback :) > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-11 5:45 ` Mugilraj Dhavachelvan @ 2021-08-11 7:03 ` Andy Shevchenko 2021-08-11 7:05 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Andy Shevchenko @ 2021-08-11 7:03 UTC (permalink / raw) To: Mugilraj Dhavachelvan Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, devicetree, Linux Kernel Mailing List, linux-iio On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan <dmugil2000@gmail.com> wrote: > On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan > > <dmugil2000@gmail.com> wrote: ... > > > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > > + if (!(val & BIT(7))) > > > + data->tol *= -1; > > > > Shouldn't you simple use corresponding sign_extend*()? > > > I'm not able see any sign_extend for 16 bit. Is there any other way? So, then add it in bitops.h the same way it's done for s32 and s64. ... > > > + if (!data->cfg) > > > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; > > > > Not sure this is not a dead code since you are using ->probe_new(). > > > Even I'm suspecting that and also removing id_table. But I'm not sure of > it so just left as it is. I²C ID table is good to have without direct use, but ->probe_new() is called if and only if there is a compatible string or ACPI ID match. In such case data->cfg may be NULL if and only if the corresponding table missed it, but this will be a bug anyway, so the above code will rather hide the bug. Hence, please remove it. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-11 7:03 ` Andy Shevchenko @ 2021-08-11 7:05 ` Andy Shevchenko 2021-08-11 8:11 ` Mugilraj Dhavachelvan 0 siblings, 1 reply; 14+ messages in thread From: Andy Shevchenko @ 2021-08-11 7:05 UTC (permalink / raw) To: Mugilraj Dhavachelvan Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, devicetree, Linux Kernel Mailing List, linux-iio On Wed, Aug 11, 2021 at 10:03 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan > <dmugil2000@gmail.com> wrote: > > On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan > > > <dmugil2000@gmail.com> wrote: > > ... > > > > > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > > > + if (!(val & BIT(7))) > > > > + data->tol *= -1; > > > > > > Shouldn't you simple use corresponding sign_extend*()? > > > > > I'm not able see any sign_extend for 16 bit. Is there any other way? > > So, then add it in bitops.h the same way it's done for s32 and s64. In case you are afraid that this will delay patches and you are in a hurry, you may define it locally (but in the very same way, so somebody can make it generic). Nevertheless, I think this one can be added directly to the generic header, it will definitely have more than one user. > ... > > > > > + if (!data->cfg) > > > > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; > > > > > > Not sure this is not a dead code since you are using ->probe_new(). > > > > > Even I'm suspecting that and also removing id_table. But I'm not sure of > > it so just left as it is. > > I²C ID table is good to have without direct use, but ->probe_new() is > called if and only if there is a compatible string or ACPI ID match. > In such case data->cfg may be NULL if and only if the corresponding > table missed it, but this will be a bug anyway, so the above code will > rather hide the bug. Hence, please remove it. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-11 7:05 ` Andy Shevchenko @ 2021-08-11 8:11 ` Mugilraj Dhavachelvan 0 siblings, 0 replies; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-11 8:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Lars-Peter Clausen, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, devicetree, Linux Kernel Mailing List, linux-iio On Wed, Aug 11, 2021 at 10:05:41AM +0300, Andy Shevchenko wrote: > On Wed, Aug 11, 2021 at 10:03 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Aug 11, 2021 at 8:46 AM Mugilraj Dhavachelvan > > <dmugil2000@gmail.com> wrote: > > > On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote: > > > > On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan > > > > <dmugil2000@gmail.com> wrote: > > > > ... > > > > > > > + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > > > > + if (!(val & BIT(7))) > > > > > + data->tol *= -1; > > > > > > > > Shouldn't you simple use corresponding sign_extend*()? > > > > > > > I'm not able see any sign_extend for 16 bit. Is there any other way? > > > > So, then add it in bitops.h the same way it's done for s32 and s64. > > In case you are afraid that this will delay patches and you are in a > hurry, you may define it locally (but in the very same way, so > somebody can make it generic). Nevertheless, I think this one can be > added directly to the generic header, it will definitely have more > than one user. > It seems like we can use sign_extend32() for 16 and 8 bit as well. > > ... > > > > > > > + if (!data->cfg) > > > > > + data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data]; > > > > > > > > Not sure this is not a dead code since you are using ->probe_new(). > > > > > > > Even I'm suspecting that and also removing id_table. But I'm not sure of > > > it so just left as it is. > > > > I²C ID table is good to have without direct use, but ->probe_new() is > > called if and only if there is a compatible string or ACPI ID match. > > In such case data->cfg may be NULL if and only if the corresponding > > table missed it, but this will be a bug anyway, so the above code will > > rather hide the bug. Hence, please remove it. > > > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-10 12:49 ` Andy Shevchenko 2021-08-11 5:45 ` Mugilraj Dhavachelvan @ 2021-08-11 8:15 ` Lars-Peter Clausen 2021-08-11 16:06 ` Andy Shevchenko 1 sibling, 1 reply; 14+ messages in thread From: Lars-Peter Clausen @ 2021-08-11 8:15 UTC (permalink / raw) To: Andy Shevchenko, Mugilraj Dhavachelvan Cc: Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, linux-kernel, linux-iio On 8/10/21 2:49 PM, Andy Shevchenko wrote: > >> + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; >> + if (!(val & BIT(7))) >> + data->tol *= -1; > Shouldn't you simple use corresponding sign_extend*()? The data is encoded a sign-magnitude. sign_extend() works for two's complement numbers. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-11 8:15 ` Lars-Peter Clausen @ 2021-08-11 16:06 ` Andy Shevchenko 2021-08-12 16:53 ` Mugilraj Dhavachelvan 0 siblings, 1 reply; 14+ messages in thread From: Andy Shevchenko @ 2021-08-11 16:06 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Mugilraj Dhavachelvan, Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, Linux Kernel Mailing List, linux-iio On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > On 8/10/21 2:49 PM, Andy Shevchenko wrote: > > > >> + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > >> + if (!(val & BIT(7))) > >> + data->tol *= -1; > > Shouldn't you simple use corresponding sign_extend*()? > The data is encoded a sign-magnitude. sign_extend() works for two's > complement numbers. Good catch! I'm wondering if it's a good idea to have a sign_magnitude_to_int() helper or so? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-11 16:06 ` Andy Shevchenko @ 2021-08-12 16:53 ` Mugilraj Dhavachelvan 2021-08-14 8:18 ` Mugilraj Dhavachelvan 0 siblings, 1 reply; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-12 16:53 UTC (permalink / raw) To: Andy Shevchenko Cc: Lars-Peter Clausen, Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, Linux Kernel Mailing List, linux-iio On Wed, Aug 11, 2021 at 07:06:43PM +0300, Andy Shevchenko wrote: > On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 8/10/21 2:49 PM, Andy Shevchenko wrote: > > > > > >> + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > >> + if (!(val & BIT(7))) > > >> + data->tol *= -1; > > > Shouldn't you simple use corresponding sign_extend*()? > > The data is encoded a sign-magnitude. sign_extend() works for two's > > complement numbers. > > Good catch! > I'm wondering if it's a good idea to have a sign_magnitude_to_int() > helper or so? > So, What should I do now? > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 2021-08-12 16:53 ` Mugilraj Dhavachelvan @ 2021-08-14 8:18 ` Mugilraj Dhavachelvan 0 siblings, 0 replies; 14+ messages in thread From: Mugilraj Dhavachelvan @ 2021-08-14 8:18 UTC (permalink / raw) To: Andy Shevchenko Cc: Lars-Peter Clausen, Bogdan, Dragos, Darius, Rob Herring, Jonathan Cameron, Michael Hennerich, Guenter Roeck, Chris Packham, Slawomir Stepien, Krzysztof Kozlowski, Alexandre Belloni, Alexandru Ardelean, devicetree, Linux Kernel Mailing List, linux-iio On Thu, Aug 12, 2021 at 10:23:41PM +0530, Mugilraj Dhavachelvan wrote: > On Wed, Aug 11, 2021 at 07:06:43PM +0300, Andy Shevchenko wrote: > > On Wed, Aug 11, 2021 at 11:15 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > > > On 8/10/21 2:49 PM, Andy Shevchenko wrote: > > > > > > > >> + data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8; > > > >> + if (!(val & BIT(7))) > > > >> + data->tol *= -1; > > > > Shouldn't you simple use corresponding sign_extend*()? > > > The data is encoded a sign-magnitude. sign_extend() works for two's > > > complement numbers. > > > > Good catch! > > I'm wondering if it's a good idea to have a sign_magnitude_to_int() > > helper or so? > > > So, What should I do now? > Will send v3 with other changes and add this change in v4. Hope it's okay. > > -- > > With Best Regards, > > Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-08-14 8:18 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-09 7:57 [PATCH v2 0/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 2021-08-09 7:57 ` [PATCH v2 1/2] dt-bindings: iio: potentiometer: Add AD5110 in trivial-devices Mugilraj Dhavachelvan 2021-08-13 20:57 ` Rob Herring 2021-08-09 7:57 ` [PATCH v2 2/2] iio: potentiometer: Add driver support for AD5110 Mugilraj Dhavachelvan 2021-08-09 20:05 ` Jonathan Cameron 2021-08-10 12:49 ` Andy Shevchenko 2021-08-11 5:45 ` Mugilraj Dhavachelvan 2021-08-11 7:03 ` Andy Shevchenko 2021-08-11 7:05 ` Andy Shevchenko 2021-08-11 8:11 ` Mugilraj Dhavachelvan 2021-08-11 8:15 ` Lars-Peter Clausen 2021-08-11 16:06 ` Andy Shevchenko 2021-08-12 16:53 ` Mugilraj Dhavachelvan 2021-08-14 8:18 ` Mugilraj Dhavachelvan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).