* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-10 6:17 ` [PATCH 2/2] iio: ad5272: Add support " Phil Reid
@ 2018-01-11 9:00 ` Phil Reid
2018-01-14 13:41 ` Jonathan Cameron
2018-01-12 10:42 ` alexandru.ardelean
2018-01-14 13:39 ` Jonathan Cameron
2 siblings, 1 reply; 11+ messages in thread
From: Phil Reid @ 2018-01-11 9:00 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
Michael.Hennerich, linux-iio, devicetree
On 10/01/2018 14:17, Phil Reid wrote:
> Add implementation for Analog Devices AD5272 and AD5274 digital
> potentiometer devices.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/iio/potentiometer/Kconfig | 10 ++
> drivers/iio/potentiometer/Makefile | 1 +
> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
> create mode 100644 drivers/iio/potentiometer/ad5272.c
>
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 8bf2825..0b04063 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -5,6 +5,16 @@
>
> menu "Digital potentiometers"
>
> +config AD5272
> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
> + depends on I2C
> + help
> + Say yes here to build support for the Analog Devices AD5272
> + digital potentiometer chip.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5272.
> +
> config DS1803
> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 2260d40..498bbb8 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -3,6 +3,7 @@
> #
>
> # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AD5272) += ad5272.o
> obj-$(CONFIG_DS1803) += ds1803.o
> obj-$(CONFIG_MAX5481) += max5481.o
> obj-$(CONFIG_MAX5487) += max5487.o
> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> new file mode 100644
> index 0000000..b9208c2
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ad5272.c
> @@ -0,0 +1,219 @@
> +/*
> + * Analog Devices AD5372 digital potentiometer driver
> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
> + *
> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
> + *
> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
> + * ad5272 1 1024 20, 50, 100 01011xx
> + * ad5274 1 256 20, 100 01011xx
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +
> +#define AD5272_RDAC_WR 1
> +#define AD5272_RDAC_RD 2
> +#define AD5272_CTL 7
> +
> +#define AD5272_RDAC_WR_EN BIT(1)
> +
> +struct ad5272_cfg {
> + int max_pos;
> + int kohms;
> + int shift;
> +};
> +
> +enum ad5272_type {
> + AD5272_020,
> + AD5272_050,
> + AD5272_100,
> + AD5274_020,
> + AD5274_100,
> +};
> +
> +static const struct ad5272_cfg ad5272_cfg[] = {
> + /* on-semiconductor parts */
> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
> +};
> +
> +struct ad5272_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + const struct ad5272_cfg *cfg;
> +};
> +
> +static const struct iio_chan_spec ad5272_channel = {
> + .type = IIO_RESISTANCE,
> + .output = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> +{
> + u8 buf[2];
> + int ret;
> +
> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
> + buf[1] = (u8)val;
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
> + mutex_unlock(&data->lock);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> +{
> + u8 buf[2];
> + int ret;
> +
> + buf[0] = (reg & 0xf) << 2;
> + buf[1] = 0;
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + ret = i2c_master_recv(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + *val = ((buf[0] & 0x3) << 8) | buf[1];
> + ret = 0;
> +error:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static int ad5272_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
> + *val = *val >> data->cfg->shift;
> + return ret ? ret : IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000 * data->cfg->kohms;
> + *val2 = data->cfg->max_pos;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad5272_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + if (val >= data->cfg->max_pos || val < 0)
> + return -EINVAL;
> +
> + val = val << data->cfg->shift;
> +
> + return ad5272_write(data, AD5272_RDAC_WR, val);
> +}
> +
> +static const struct iio_info ad5272_info = {
> + .read_raw = ad5272_read_raw,
> + .write_raw = ad5272_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int ad5272_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct gpio_desc *reset_gpio;
> + struct iio_dev *indio_dev;
> + struct ad5272_data *data;
> + int ret;
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_LOW);
Reset line to the chip is active low, so I guess I should really be setting it to GPIOD_OUT_HIGH.
Instead of having the DT define it as active-low?
I'm not sure what makes the most sense.
> + if (IS_ERR(reset_gpio))
> + return PTR_ERR(reset_gpio);
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, indio_dev);
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + mutex_init(&data->lock);
> + data->cfg = &ad5272_cfg[id->driver_data];
> +
> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
> + if (ret < 0)
> + return -ENODEV;
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &ad5272_info;
> + indio_dev->channels = &ad5272_channel;
> + indio_dev->num_channels = 1;
> + indio_dev->name = client->name;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id ad5272_dt_ids[] = {
> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id ad5272_id[] = {
> + { "ad5272-020", AD5272_020 },
> + { "ad5272-050", AD5272_050 },
> + { "ad5272-100", AD5272_100 },
> + { "ad5274-020", AD5274_020 },
> + { "ad5274-100", AD5274_100 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
> +
> +static struct i2c_driver ad5272_driver = {
> + .driver = {
> + .name = "ad5272",
> + .of_match_table = of_match_ptr(ad5272_dt_ids),
> + },
> + .probe = ad5272_probe,
> + .id_table = ad5272_id,
> +};
> +
> +module_i2c_driver(ad5272_driver);
> +
> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
> +MODULE_LICENSE("GPL v2");
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-11 9:00 ` Phil Reid
@ 2018-01-14 13:41 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2018-01-14 13:41 UTC (permalink / raw)
To: Phil Reid
Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, Michael.Hennerich,
linux-iio, devicetree
On Thu, 11 Jan 2018 17:00:31 +0800
Phil Reid <preid@electromag.com.au> wrote:
...
> > +static int ad5272_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct gpio_desc *reset_gpio;
> > + struct iio_dev *indio_dev;
> > + struct ad5272_data *data;
> > + int ret;
> > +
> > + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > + GPIOD_OUT_LOW);
> Reset line to the chip is active low, so I guess I should really be setting it to GPIOD_OUT_HIGH.
> Instead of having the DT define it as active-low?
> I'm not sure what makes the most sense.
As long as you are consistent it doesn't really matter. This flexibility is
more relevant for generic code where we might need to make things work across
both options.
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-10 6:17 ` [PATCH 2/2] iio: ad5272: Add support " Phil Reid
2018-01-11 9:00 ` Phil Reid
@ 2018-01-12 10:42 ` alexandru.ardelean
2018-01-14 13:02 ` Jonathan Cameron
2018-01-15 8:37 ` Phil Reid
2018-01-14 13:39 ` Jonathan Cameron
2 siblings, 2 replies; 11+ messages in thread
From: alexandru.ardelean @ 2018-01-12 10:42 UTC (permalink / raw)
To: linux-iio; +Cc: Alexandru Ardelean
From: Alexandru Ardelean <alexandru.ardelean@analog.com>
On 10/01/2018 14:17, Phil Reid wrote:
> Add implementation for Analog Devices AD5272 and AD5274 digital
> potentiometer devices.
>
Hey,
Thanks for the patch.
Replies inline
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/iio/potentiometer/Kconfig | 10 ++
> drivers/iio/potentiometer/Makefile | 1 +
> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
> create mode 100644 drivers/iio/potentiometer/ad5272.c
>
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 8bf2825..0b04063 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -5,6 +5,16 @@
>
> menu "Digital potentiometers"
>
> +config AD5272
> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
> + depends on I2C
> + help
> + Say yes here to build support for the Analog Devices AD5272
> + digital potentiometer chip.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5272.
> +
> config DS1803
> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 2260d40..498bbb8 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -3,6 +3,7 @@
> #
>
> # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AD5272) += ad5272.o
> obj-$(CONFIG_DS1803) += ds1803.o
> obj-$(CONFIG_MAX5481) += max5481.o
> obj-$(CONFIG_MAX5487) += max5487.o
> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> new file mode 100644
> index 0000000..b9208c2
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ad5272.c
> @@ -0,0 +1,219 @@
> +/*
> + * Analog Devices AD5372 digital potentiometer driver
> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
> + *
> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
> + *
> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
> + * ad5272 1 1024 20, 50, 100 01011xx
> + * ad5274 1 256 20, 100 01011xx
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +
> +#define AD5272_RDAC_WR 1
> +#define AD5272_RDAC_RD 2
> +#define AD5272_CTL 7
> +
> +#define AD5272_RDAC_WR_EN BIT(1)
> +
> +struct ad5272_cfg {
> + int max_pos;
> + int kohms;
> + int shift;
> +};
> +
> +enum ad5272_type {
> + AD5272_020,
> + AD5272_050,
> + AD5272_100,
> + AD5274_020,
> + AD5274_100,
> +};
> +
> +static const struct ad5272_cfg ad5272_cfg[] = {
> + /* on-semiconductor parts */
> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
> +};
> +
> +struct ad5272_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + const struct ad5272_cfg *cfg;
> +};
> +
> +static const struct iio_chan_spec ad5272_channel = {
> + .type = IIO_RESISTANCE,
> + .output = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> +{
> + u8 buf[2];
> + int ret;
> +
> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
> + buf[1] = (u8)val;
Maybe this should be masked with 0xff, to avoid any potential
side-effects of downcasting.
So, buf[1] = (u8)(0xff & val).
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
More of a preference from me.
But, I'd use sizeof(buf) instead of value '2'.
Same comment for other similar cases.
> + mutex_unlock(&data->lock);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> +{
> + u8 buf[2];
> + int ret;
> +
> + buf[0] = (reg & 0xf) << 2;
> + buf[1] = 0;
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + ret = i2c_master_recv(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + *val = ((buf[0] & 0x3) << 8) | buf[1];
> + ret = 0;
> +error:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static int ad5272_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
> + *val = *val >> data->cfg->shift;
> + return ret ? ret : IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000 * data->cfg->kohms;
> + *val2 = data->cfg->max_pos;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad5272_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + if (val >= data->cfg->max_pos || val < 0)
> + return -EINVAL;
> +
> + val = val << data->cfg->shift;
> +
> + return ad5272_write(data, AD5272_RDAC_WR, val);
> +}
> +
> +static const struct iio_info ad5272_info = {
> + .read_raw = ad5272_read_raw,
> + .write_raw = ad5272_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int ad5272_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct gpio_desc *reset_gpio;
> + struct iio_dev *indio_dev;
> + struct ad5272_data *data;
> + int ret;
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(reset_gpio))
> + return PTR_ERR(reset_gpio);
I think this may also need some more logic to perform a reset.
Maybe setting the value with `gpiod_set_value(reset_gpio, 1)`
Also, if `reset_gpio` is NULL, you could issue a software reset
with command 4.
Something similar is done in `ad5592r-base.c`
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, indio_dev);
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + mutex_init(&data->lock);
> + data->cfg = &ad5272_cfg[id->driver_data];
> +
> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
> + if (ret < 0)
> + return -ENODEV;
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &ad5272_info;
> + indio_dev->channels = &ad5272_channel;
> + indio_dev->num_channels = 1;
> + indio_dev->name = client->name;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id ad5272_dt_ids[] = {
> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id ad5272_id[] = {
> + { "ad5272-020", AD5272_020 },
> + { "ad5272-050", AD5272_050 },
> + { "ad5272-100", AD5272_100 },
> + { "ad5274-020", AD5274_020 },
> + { "ad5274-100", AD5274_100 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
> +
> +static struct i2c_driver ad5272_driver = {
> + .driver = {
> + .name = "ad5272",
> + .of_match_table = of_match_ptr(ad5272_dt_ids),
> + },
> + .probe = ad5272_probe,
> + .id_table = ad5272_id,
> +};
> +
> +module_i2c_driver(ad5272_driver);
> +
> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-12 10:42 ` alexandru.ardelean
@ 2018-01-14 13:02 ` Jonathan Cameron
2018-01-15 8:37 ` Phil Reid
1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2018-01-14 13:02 UTC (permalink / raw)
To: alexandru.ardelean; +Cc: linux-iio
On Fri, 12 Jan 2018 12:42:14 +0200
<alexandru.ardelean@analog.com> wrote:
> From: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> On 10/01/2018 14:17, Phil Reid wrote:
> > Add implementation for Analog Devices AD5272 and AD5274 digital
> > potentiometer devices.
> >
>
> Hey,
>
> Thanks for the patch.
> Replies inline
>
> > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > ---
> > drivers/iio/potentiometer/Kconfig | 10 ++
> > drivers/iio/potentiometer/Makefile | 1 +
> > drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 230 insertions(+)
> > create mode 100644 drivers/iio/potentiometer/ad5272.c
> >
> > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> > index 8bf2825..0b04063 100644
> > --- a/drivers/iio/potentiometer/Kconfig
> > +++ b/drivers/iio/potentiometer/Kconfig
> > @@ -5,6 +5,16 @@
> >
> > menu "Digital potentiometers"
> >
> > +config AD5272
> > + tristate "Analog Devices AD5272 Digital Potentiometer driver"
> > + depends on I2C
> > + help
> > + Say yes here to build support for the Analog Devices AD5272
> > + digital potentiometer chip.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ad5272.
> > +
> > config DS1803
> > tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> > depends on I2C
> > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> > index 2260d40..498bbb8 100644
> > --- a/drivers/iio/potentiometer/Makefile
> > +++ b/drivers/iio/potentiometer/Makefile
> > @@ -3,6 +3,7 @@
> > #
> >
> > # When adding new entries keep the list in alphabetical order
> > +obj-$(CONFIG_AD5272) += ad5272.o
> > obj-$(CONFIG_DS1803) += ds1803.o
> > obj-$(CONFIG_MAX5481) += max5481.o
> > obj-$(CONFIG_MAX5487) += max5487.o
> > diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> > new file mode 100644
> > index 0000000..b9208c2
> > --- /dev/null
> > +++ b/drivers/iio/potentiometer/ad5272.c
> > @@ -0,0 +1,219 @@
> > +/*
> > + * Analog Devices AD5372 digital potentiometer driver
> > + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
> > + *
> > + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
> > + *
> > + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
> > + * ad5272 1 1024 20, 50, 100 01011xx
> > + * ad5274 1 256 20, 100 01011xx
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +
> > +#define AD5272_RDAC_WR 1
> > +#define AD5272_RDAC_RD 2
> > +#define AD5272_CTL 7
> > +
> > +#define AD5272_RDAC_WR_EN BIT(1)
> > +
> > +struct ad5272_cfg {
> > + int max_pos;
> > + int kohms;
> > + int shift;
> > +};
> > +
> > +enum ad5272_type {
> > + AD5272_020,
> > + AD5272_050,
> > + AD5272_100,
> > + AD5274_020,
> > + AD5274_100,
> > +};
> > +
> > +static const struct ad5272_cfg ad5272_cfg[] = {
> > + /* on-semiconductor parts */
> > + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
> > + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
> > + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
> > + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
> > + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
> > +};
> > +
> > +struct ad5272_data {
> > + struct i2c_client *client;
> > + struct mutex lock;
> > + const struct ad5272_cfg *cfg;
> > +};
> > +
> > +static const struct iio_chan_spec ad5272_channel = {
> > + .type = IIO_RESISTANCE,
> > + .output = 1,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> > +};
> > +
> > +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> > +{
> > + u8 buf[2];
> > + int ret;
> > +
> > + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
> > + buf[1] = (u8)val;
>
> Maybe this should be masked with 0xff, to avoid any potential
> side-effects of downcasting.
> So, buf[1] = (u8)(0xff & val).
There are no sideeffects in down casting - it simply drops the bits.
c99 spec gives me:
6.3.1.3 Signed and unsigned integers
1 When a value with integer type is converted to another integer type other than _Bool, if
the value can be represented by the new type, it is unchanged.
2 Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or
subtracting one more than the maximum value that can be represented in the new type
until the value is in the range of the new type.49)
3 Otherwise, the new type is signed and the value cannot be represented in it; either the
result is implementation-defined or an implementation-defined signal is raised.
So in this case we are down casting we basically end up subtracting all the higher
bits (same as just cutting them off or masking as you did.
Jonathan
>
> > +
> > + mutex_lock(&data->lock);
> > + ret = i2c_master_send(data->client, buf, 2);
>
> More of a preference from me.
> But, I'd use sizeof(buf) instead of value '2'.
> Same comment for other similar cases.
>
> > + mutex_unlock(&data->lock);
> > + return ret < 0 ? ret : 0;
> > +}
> > +
> > +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> > +{
> > + u8 buf[2];
> > + int ret;
> > +
> > + buf[0] = (reg & 0xf) << 2;
> > + buf[1] = 0;
> > +
> > + mutex_lock(&data->lock);
> > + ret = i2c_master_send(data->client, buf, 2);
> > + if (ret < 0)
> > + goto error;
> > +
> > + ret = i2c_master_recv(data->client, buf, 2);
> > + if (ret < 0)
> > + goto error;
> > +
> > + *val = ((buf[0] & 0x3) << 8) | buf[1];
> > + ret = 0;
> > +error:
> > + mutex_unlock(&data->lock);
> > + return ret;
> > +}
> > +
> > +static int ad5272_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct ad5272_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW: {
> > + ret = ad5272_read(data, AD5272_RDAC_RD, val);
> > + *val = *val >> data->cfg->shift;
> > + return ret ? ret : IIO_VAL_INT;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = 1000 * data->cfg->kohms;
> > + *val2 = data->cfg->max_pos;
> > + return IIO_VAL_FRACTIONAL;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ad5272_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ad5272_data *data = iio_priv(indio_dev);
> > +
> > + if (mask != IIO_CHAN_INFO_RAW)
> > + return -EINVAL;
> > +
> > + if (val >= data->cfg->max_pos || val < 0)
> > + return -EINVAL;
> > +
> > + val = val << data->cfg->shift;
> > +
> > + return ad5272_write(data, AD5272_RDAC_WR, val);
> > +}
> > +
> > +static const struct iio_info ad5272_info = {
> > + .read_raw = ad5272_read_raw,
> > + .write_raw = ad5272_write_raw,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int ad5272_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct gpio_desc *reset_gpio;
> > + struct iio_dev *indio_dev;
> > + struct ad5272_data *data;
> > + int ret;
> > +
> > + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(reset_gpio))
> > + return PTR_ERR(reset_gpio);
>
> I think this may also need some more logic to perform a reset.
> Maybe setting the value with `gpiod_set_value(reset_gpio, 1)`
> Also, if `reset_gpio` is NULL, you could issue a software reset
> with command 4.
>
> Something similar is done in `ad5592r-base.c`
>
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + data = iio_priv(indio_dev);
> > + data->client = client;
> > + mutex_init(&data->lock);
> > + data->cfg = &ad5272_cfg[id->driver_data];
> > +
> > + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
> > + if (ret < 0)
> > + return -ENODEV;
> > +
> > + indio_dev->dev.parent = dev;
> > + indio_dev->info = &ad5272_info;
> > + indio_dev->channels = &ad5272_channel;
> > + indio_dev->num_channels = 1;
> > + indio_dev->name = client->name;
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id ad5272_dt_ids[] = {
> > + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
> > + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
> > + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
> > + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
> > + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
> > +#endif /* CONFIG_OF */
> > +
> > +static const struct i2c_device_id ad5272_id[] = {
> > + { "ad5272-020", AD5272_020 },
> > + { "ad5272-050", AD5272_050 },
> > + { "ad5272-100", AD5272_100 },
> > + { "ad5274-020", AD5274_020 },
> > + { "ad5274-100", AD5274_100 },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ad5272_id);
> > +
> > +static struct i2c_driver ad5272_driver = {
> > + .driver = {
> > + .name = "ad5272",
> > + .of_match_table = of_match_ptr(ad5272_dt_ids),
> > + },
> > + .probe = ad5272_probe,
> > + .id_table = ad5272_id,
> > +};
> > +
> > +module_i2c_driver(ad5272_driver);
> > +
> > +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
> > +MODULE_DESCRIPTION("AD5272 digital potentiometer");
> > +MODULE_LICENSE("GPL v2");
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-12 10:42 ` alexandru.ardelean
2018-01-14 13:02 ` Jonathan Cameron
@ 2018-01-15 8:37 ` Phil Reid
1 sibling, 0 replies; 11+ messages in thread
From: Phil Reid @ 2018-01-15 8:37 UTC (permalink / raw)
To: alexandru.ardelean, linux-iio
On 12/01/2018 18:42, alexandru.ardelean@analog.com wrote:
> From: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> On 10/01/2018 14:17, Phil Reid wrote:
>> Add implementation for Analog Devices AD5272 and AD5274 digital
>> potentiometer devices.
>>
>
> Hey,
>
> Thanks for the patch.
> Replies inline
Thanks for reviewing.
>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>> drivers/iio/potentiometer/Kconfig | 10 ++
>> drivers/iio/potentiometer/Makefile | 1 +
>> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 230 insertions(+)
>> create mode 100644 drivers/iio/potentiometer/ad5272.c
>>
>> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
>> index 8bf2825..0b04063 100644
>> --- a/drivers/iio/potentiometer/Kconfig
>> +++ b/drivers/iio/potentiometer/Kconfig
>> @@ -5,6 +5,16 @@
>>
>> menu "Digital potentiometers"
>>
>> +config AD5272
>> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
>> + depends on I2C
>> + help
>> + Say yes here to build support for the Analog Devices AD5272
>> + digital potentiometer chip.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ad5272.
>> +
>> config DS1803
>> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
>> depends on I2C
>> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
>> index 2260d40..498bbb8 100644
>> --- a/drivers/iio/potentiometer/Makefile
>> +++ b/drivers/iio/potentiometer/Makefile
>> @@ -3,6 +3,7 @@
>> #
>>
>> # When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_AD5272) += ad5272.o
>> obj-$(CONFIG_DS1803) += ds1803.o
>> obj-$(CONFIG_MAX5481) += max5481.o
>> obj-$(CONFIG_MAX5487) += max5487.o
>> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
>> new file mode 100644
>> index 0000000..b9208c2
>> --- /dev/null
>> +++ b/drivers/iio/potentiometer/ad5272.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Analog Devices AD5372 digital potentiometer driver
>> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
>> + *
>> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
>> + *
>> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
>> + * ad5272 1 1024 20, 50, 100 01011xx
>> + * ad5274 1 256 20, 100 01011xx
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +
>> +#define AD5272_RDAC_WR 1
>> +#define AD5272_RDAC_RD 2
>> +#define AD5272_CTL 7
>> +
>> +#define AD5272_RDAC_WR_EN BIT(1)
>> +
>> +struct ad5272_cfg {
>> + int max_pos;
>> + int kohms;
>> + int shift;
>> +};
>> +
>> +enum ad5272_type {
>> + AD5272_020,
>> + AD5272_050,
>> + AD5272_100,
>> + AD5274_020,
>> + AD5274_100,
>> +};
>> +
>> +static const struct ad5272_cfg ad5272_cfg[] = {
>> + /* on-semiconductor parts */
>> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
>> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
>> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
>> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
>> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
>> +};
>> +
>> +struct ad5272_data {
>> + struct i2c_client *client;
>> + struct mutex lock;
>> + const struct ad5272_cfg *cfg;
>> +};
>> +
>> +static const struct iio_chan_spec ad5272_channel = {
>> + .type = IIO_RESISTANCE,
>> + .output = 1,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +};
>> +
>> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
>> +{
>> + u8 buf[2];
>> + int ret;
>> +
>> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
>> + buf[1] = (u8)val;
>
> Maybe this should be masked with 0xff, to avoid any potential
> side-effects of downcasting.
> So, buf[1] = (u8)(0xff & val).
>
>> +
>> + mutex_lock(&data->lock);
>> + ret = i2c_master_send(data->client, buf, 2);
>
> More of a preference from me.
> But, I'd use sizeof(buf) instead of value '2'.
> Same comment for other similar cases.
Ok , Jonathan also like that way.
>
>> + mutex_unlock(&data->lock);
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
>> +{
>> + u8 buf[2];
>> + int ret;
>> +
>> + buf[0] = (reg & 0xf) << 2;
>> + buf[1] = 0;
>> +
>> + mutex_lock(&data->lock);
>> + ret = i2c_master_send(data->client, buf, 2);
>> + if (ret < 0)
>> + goto error;
>> +
>> + ret = i2c_master_recv(data->client, buf, 2);
>> + if (ret < 0)
>> + goto error;
>> +
>> + *val = ((buf[0] & 0x3) << 8) | buf[1];
>> + ret = 0;
>> +error:
>> + mutex_unlock(&data->lock);
>> + return ret;
>> +}
>> +
>> +static int ad5272_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct ad5272_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW: {
>> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
>> + *val = *val >> data->cfg->shift;
>> + return ret ? ret : IIO_VAL_INT;
>> + }
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = 1000 * data->cfg->kohms;
>> + *val2 = data->cfg->max_pos;
>> + return IIO_VAL_FRACTIONAL;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int ad5272_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + struct ad5272_data *data = iio_priv(indio_dev);
>> +
>> + if (mask != IIO_CHAN_INFO_RAW)
>> + return -EINVAL;
>> +
>> + if (val >= data->cfg->max_pos || val < 0)
>> + return -EINVAL;
>> +
>> + val = val << data->cfg->shift;
>> +
>> + return ad5272_write(data, AD5272_RDAC_WR, val);
>> +}
>> +
>> +static const struct iio_info ad5272_info = {
>> + .read_raw = ad5272_read_raw,
>> + .write_raw = ad5272_write_raw,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int ad5272_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct gpio_desc *reset_gpio;
>> + struct iio_dev *indio_dev;
>> + struct ad5272_data *data;
>> + int ret;
>> +
>> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(reset_gpio))
>> + return PTR_ERR(reset_gpio);
>
> I think this may also need some more logic to perform a reset.
> Maybe setting the value with `gpiod_set_value(reset_gpio, 1)`
> Also, if `reset_gpio` is NULL, you could issue a software reset
> with command 4.
>
> Something similar is done in `ad5592r-base.c`
Ok, that looks like a good approach.
>
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, indio_dev);
>> +
>> + data = iio_priv(indio_dev);
>> + data->client = client;
>> + mutex_init(&data->lock);
>> + data->cfg = &ad5272_cfg[id->driver_data];
>> +
>> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
>> + if (ret < 0)
>> + return -ENODEV;
>> +
>> + indio_dev->dev.parent = dev;
>> + indio_dev->info = &ad5272_info;
>> + indio_dev->channels = &ad5272_channel;
>> + indio_dev->num_channels = 1;
>> + indio_dev->name = client->name;
>> +
>> + return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id ad5272_dt_ids[] = {
>> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
>> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
>> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
>> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
>> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
>> +#endif /* CONFIG_OF */
>> +
>> +static const struct i2c_device_id ad5272_id[] = {
>> + { "ad5272-020", AD5272_020 },
>> + { "ad5272-050", AD5272_050 },
>> + { "ad5272-100", AD5272_100 },
>> + { "ad5274-020", AD5274_020 },
>> + { "ad5274-100", AD5274_100 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
>> +
>> +static struct i2c_driver ad5272_driver = {
>> + .driver = {
>> + .name = "ad5272",
>> + .of_match_table = of_match_ptr(ad5272_dt_ids),
>> + },
>> + .probe = ad5272_probe,
>> + .id_table = ad5272_id,
>> +};
>> +
>> +module_i2c_driver(ad5272_driver);
>> +
>> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
>> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
>> +MODULE_LICENSE("GPL v2");
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-10 6:17 ` [PATCH 2/2] iio: ad5272: Add support " Phil Reid
2018-01-11 9:00 ` Phil Reid
2018-01-12 10:42 ` alexandru.ardelean
@ 2018-01-14 13:39 ` Jonathan Cameron
2018-01-15 8:47 ` Phil Reid
2 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2018-01-14 13:39 UTC (permalink / raw)
To: Phil Reid
Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, Michael.Hennerich,
linux-iio, devicetree
On Wed, 10 Jan 2018 14:17:31 +0800
Phil Reid <preid@electromag.com.au> wrote:
> Add implementation for Analog Devices AD5272 and AD5274 digital
> potentiometer devices.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
Hi Phil,
A few minor comments inline, but looking pretty good.
Jonathan
> ---
> drivers/iio/potentiometer/Kconfig | 10 ++
> drivers/iio/potentiometer/Makefile | 1 +
> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
> create mode 100644 drivers/iio/potentiometer/ad5272.c
>
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 8bf2825..0b04063 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -5,6 +5,16 @@
>
> menu "Digital potentiometers"
>
> +config AD5272
> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
> + depends on I2C
> + help
> + Say yes here to build support for the Analog Devices AD5272
> + digital potentiometer chip.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5272.
> +
> config DS1803
> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> depends on I2C
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 2260d40..498bbb8 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -3,6 +3,7 @@
> #
>
> # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AD5272) += ad5272.o
> obj-$(CONFIG_DS1803) += ds1803.o
> obj-$(CONFIG_MAX5481) += max5481.o
> obj-$(CONFIG_MAX5487) += max5487.o
> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> new file mode 100644
> index 0000000..b9208c2
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ad5272.c
> @@ -0,0 +1,219 @@
> +/*
> + * Analog Devices AD5372 digital potentiometer driver
> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
> + *
> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
> + *
> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
> + * ad5272 1 1024 20, 50, 100 01011xx
> + * ad5274 1 256 20, 100 01011xx
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
Could go all SPDX with the gpl part of this. Personally I don't mind
much but we'll probably end up converting it over soon anyway.
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +
> +#define AD5272_RDAC_WR 1
> +#define AD5272_RDAC_RD 2
> +#define AD5272_CTL 7
> +
> +#define AD5272_RDAC_WR_EN BIT(1)
> +
> +struct ad5272_cfg {
> + int max_pos;
> + int kohms;
> + int shift;
> +};
> +
> +enum ad5272_type {
> + AD5272_020,
> + AD5272_050,
> + AD5272_100,
> + AD5274_020,
> + AD5274_100,
> +};
> +
> +static const struct ad5272_cfg ad5272_cfg[] = {
> + /* on-semiconductor parts */
> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
> +};
> +
> +struct ad5272_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + const struct ad5272_cfg *cfg;
> +};
> +
> +static const struct iio_chan_spec ad5272_channel = {
> + .type = IIO_RESISTANCE,
> + .output = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> +{
> + u8 buf[2];
> + int ret;
> +
> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
> + buf[1] = (u8)val;
again, could do as buf[2] = {...};
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
sizeof(buf) preferred.
> + mutex_unlock(&data->lock);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> +{
> + u8 buf[2];
Could assign value here for compactness.
u8 buf[2] = {(reg & 0xf) << 2, 0);
It's a little paranoid to mask reg given you are controlling it entirely
in the driver and anything of more than 4 bits is a bug anyway.
> + int ret;
> +
> + buf[0] = (reg & 0xf) << 2;
> + buf[1] = 0;
> +
> + mutex_lock(&data->lock);
> + ret = i2c_master_send(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + ret = i2c_master_recv(data->client, buf, 2);
> + if (ret < 0)
> + goto error;
> +
> + *val = ((buf[0] & 0x3) << 8) | buf[1];
> + ret = 0;
> +error:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static int ad5272_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
> + *val = *val >> data->cfg->shift;
> + return ret ? ret : IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000 * data->cfg->kohms;
> + *val2 = data->cfg->max_pos;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad5272_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ad5272_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + if (val >= data->cfg->max_pos || val < 0)
> + return -EINVAL;
Also verify that val2 = 0 as otherwise the classic
1.999999 evaluates to 1 rather surprises users.
> +
> + val = val << data->cfg->shift;
I would role this into the next line.
return ad5272_write(data, AD5272_RDAC_WR, val << data->cfg->shift);
> +
> + return ad5272_write(data, AD5272_RDAC_WR, val);
> +}
> +
> +static const struct iio_info ad5272_info = {
> + .read_raw = ad5272_read_raw,
> + .write_raw = ad5272_write_raw,
> + .driver_module = THIS_MODULE,
.driver_module no longer exists so just drop this.
We do the equivalent with some macro tricks to avoid having
to have this same line in every driver.
> +};
> +
> +static int ad5272_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct gpio_desc *reset_gpio;
> + struct iio_dev *indio_dev;
> + struct ad5272_data *data;
> + int ret;
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(reset_gpio))
> + return PTR_ERR(reset_gpio);
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, indio_dev);
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + mutex_init(&data->lock);
> + data->cfg = &ad5272_cfg[id->driver_data];
Calling it cfg kind of implies config to me whereas this is
chip type specific data.
> +
> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
> + if (ret < 0)
> + return -ENODEV;
> +
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &ad5272_info;
> + indio_dev->channels = &ad5272_channel;
> + indio_dev->num_channels = 1;
> + indio_dev->name = client->name;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +#if defined(CONFIG_OF)
> +static const struct of_device_id ad5272_dt_ids[] = {
> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
Why do this rather than stash the index in the data parameter and look up later
(more consistent with the i2c_device_id table below)?
> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id ad5272_id[] = {
> + { "ad5272-020", AD5272_020 },
> + { "ad5272-050", AD5272_050 },
> + { "ad5272-100", AD5272_100 },
> + { "ad5274-020", AD5274_020 },
> + { "ad5274-100", AD5274_100 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
> +
> +static struct i2c_driver ad5272_driver = {
> + .driver = {
> + .name = "ad5272",
> + .of_match_table = of_match_ptr(ad5272_dt_ids),
> + },
> + .probe = ad5272_probe,
> + .id_table = ad5272_id,
> +};
> +
> +module_i2c_driver(ad5272_driver);
> +
> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-14 13:39 ` Jonathan Cameron
@ 2018-01-15 8:47 ` Phil Reid
2018-01-20 15:25 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Phil Reid @ 2018-01-15 8:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, Michael.Hennerich,
linux-iio, devicetree
G'day Jonathan,
On 14/01/2018 21:39, Jonathan Cameron wrote:
> On Wed, 10 Jan 2018 14:17:31 +0800
> Phil Reid <preid@electromag.com.au> wrote:
>
>> Add implementation for Analog Devices AD5272 and AD5274 digital
>> potentiometer devices.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> Hi Phil,
>
> A few minor comments inline, but looking pretty good.
Thanks for the review.
>
> Jonathan
>
>> ---
>> drivers/iio/potentiometer/Kconfig | 10 ++
>> drivers/iio/potentiometer/Makefile | 1 +
>> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 230 insertions(+)
>> create mode 100644 drivers/iio/potentiometer/ad5272.c
>>
>> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
>> index 8bf2825..0b04063 100644
>> --- a/drivers/iio/potentiometer/Kconfig
>> +++ b/drivers/iio/potentiometer/Kconfig
>> @@ -5,6 +5,16 @@
>>
>> menu "Digital potentiometers"
>>
>> +config AD5272
>> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
>> + depends on I2C
>> + help
>> + Say yes here to build support for the Analog Devices AD5272
>> + digital potentiometer chip.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ad5272.
>> +
>> config DS1803
>> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
>> depends on I2C
>> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
>> index 2260d40..498bbb8 100644
>> --- a/drivers/iio/potentiometer/Makefile
>> +++ b/drivers/iio/potentiometer/Makefile
>> @@ -3,6 +3,7 @@
>> #
>>
>> # When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_AD5272) += ad5272.o
>> obj-$(CONFIG_DS1803) += ds1803.o
>> obj-$(CONFIG_MAX5481) += max5481.o
>> obj-$(CONFIG_MAX5487) += max5487.o
>> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
>> new file mode 100644
>> index 0000000..b9208c2
>> --- /dev/null
>> +++ b/drivers/iio/potentiometer/ad5272.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Analog Devices AD5372 digital potentiometer driver
>> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
>> + *
>> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
>> + *
>> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
>> + * ad5272 1 1024 20, 50, 100 01011xx
>> + * ad5274 1 256 20, 100 01011xx
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>
> Could go all SPDX with the gpl part of this. Personally I don't mind
> much but we'll probably end up converting it over soon anyway.
>
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +
>> +#define AD5272_RDAC_WR 1
>> +#define AD5272_RDAC_RD 2
>> +#define AD5272_CTL 7
>> +
>> +#define AD5272_RDAC_WR_EN BIT(1)
>> +
>> +struct ad5272_cfg {
>> + int max_pos;
>> + int kohms;
>> + int shift;
>> +};
>> +
>> +enum ad5272_type {
>> + AD5272_020,
>> + AD5272_050,
>> + AD5272_100,
>> + AD5274_020,
>> + AD5274_100,
>> +};
>> +
>> +static const struct ad5272_cfg ad5272_cfg[] = {
>> + /* on-semiconductor parts */
>> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
>> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
>> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
>> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
>> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
>> +};
>> +
>> +struct ad5272_data {
>> + struct i2c_client *client;
>> + struct mutex lock;
>> + const struct ad5272_cfg *cfg;
>> +};
>> +
>> +static const struct iio_chan_spec ad5272_channel = {
>> + .type = IIO_RESISTANCE,
>> + .output = 1,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +};
>> +
>> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
>> +{
>> + u8 buf[2];
>> + int ret;
>> +
>> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
>> + buf[1] = (u8)val;
>
> again, could do as buf[2] = {...};
ok
>
>> +
>> + mutex_lock(&data->lock);
>> + ret = i2c_master_send(data->client, buf, 2);
>
> sizeof(buf) preferred.
ok
>
>> + mutex_unlock(&data->lock);
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
>> +{
>> + u8 buf[2];
>
> Could assign value here for compactness.
> u8 buf[2] = {(reg & 0xf) << 2, 0) >
> It's a little paranoid to mask reg given you are controlling it entirely
> in the driver and anything of more than 4 bits is a bug anyway.
ok
>> + int ret;
>> +
>> + buf[0] = (reg & 0xf) << 2;
>> + buf[1] = 0;
>> +
>> + mutex_lock(&data->lock);
>> + ret = i2c_master_send(data->client, buf, 2);
>> + if (ret < 0)
>> + goto error;
>> +
>> + ret = i2c_master_recv(data->client, buf, 2);
>> + if (ret < 0)
>> + goto error;
>> +
>> + *val = ((buf[0] & 0x3) << 8) | buf[1];
>> + ret = 0;
>> +error:
>> + mutex_unlock(&data->lock);
>> + return ret;
>> +}
>> +
>> +static int ad5272_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct ad5272_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW: {
>> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
>> + *val = *val >> data->cfg->shift;
>> + return ret ? ret : IIO_VAL_INT;
>> + }
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = 1000 * data->cfg->kohms;
>> + *val2 = data->cfg->max_pos;
>> + return IIO_VAL_FRACTIONAL;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int ad5272_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2, long mask)
>> +{
>> + struct ad5272_data *data = iio_priv(indio_dev);
>> +
>> + if (mask != IIO_CHAN_INFO_RAW)
>> + return -EINVAL;
>> +
>> + if (val >= data->cfg->max_pos || val < 0)
>> + return -EINVAL;
>
> Also verify that val2 = 0 as otherwise the classic
> 1.999999 evaluates to 1 rather surprises users.
yeap, that'd surprise me too.
>
>> +
>> + val = val << data->cfg->shift;
>
> I would role this into the next line.
ok
>
> return ad5272_write(data, AD5272_RDAC_WR, val << data->cfg->shift);
>> +
>> + return ad5272_write(data, AD5272_RDAC_WR, val);
>> +}
>> +
>> +static const struct iio_info ad5272_info = {
>> + .read_raw = ad5272_read_raw,
>> + .write_raw = ad5272_write_raw,
>> + .driver_module = THIS_MODULE,
>
> .driver_module no longer exists so just drop this.
> We do the equivalent with some macro tricks to avoid having
> to have this same line in every driver.
ok
>
>> +};
>> +
>> +static int ad5272_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct gpio_desc *reset_gpio;
>> + struct iio_dev *indio_dev;
>> + struct ad5272_data *data;
>> + int ret;
>> +
>> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
>> + GPIOD_OUT_LOW);
>> + if (IS_ERR(reset_gpio))
>> + return PTR_ERR(reset_gpio);
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, indio_dev);
>> +
>> + data = iio_priv(indio_dev);
>> + data->client = client;
>> + mutex_init(&data->lock);
>> + data->cfg = &ad5272_cfg[id->driver_data];
>
> Calling it cfg kind of implies config to me whereas this is
> chip type specific data.
I just followed what seemed to be the existing convention for the iio/potentmeters.
ds1803, mcp4531, max5481
Happy to change.
>
>> +
>> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
>> + if (ret < 0)
>> + return -ENODEV;
>> +
>> + indio_dev->dev.parent = dev;
>> + indio_dev->info = &ad5272_info;
>> + indio_dev->channels = &ad5272_channel;
>> + indio_dev->num_channels = 1;
>> + indio_dev->name = client->name;
>> +
>> + return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id ad5272_dt_ids[] = {
>> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
>
> Why do this rather than stash the index in the data parameter and look up later
> (more consistent with the i2c_device_id table below)?
Yeah I was a bit confused by this.
I was following how the ds1803.c driver was doing things.
To be honest I don't have a good grasp of how .data works here or how it gets referenced or if it's
even used...
>
>> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
>> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
>> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
>> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
>> +#endif /* CONFIG_OF */
>> +
>> +static const struct i2c_device_id ad5272_id[] = {
>> + { "ad5272-020", AD5272_020 },
>> + { "ad5272-050", AD5272_050 },
>> + { "ad5272-100", AD5272_100 },
>> + { "ad5274-020", AD5274_020 },
>> + { "ad5274-100", AD5274_100 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
>> +
>> +static struct i2c_driver ad5272_driver = {
>> + .driver = {
>> + .name = "ad5272",
>> + .of_match_table = of_match_ptr(ad5272_dt_ids),
>> + },
>> + .probe = ad5272_probe,
>> + .id_table = ad5272_id,
>> +};
>> +
>> +module_i2c_driver(ad5272_driver);
>> +
>> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
>> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
>> +MODULE_LICENSE("GPL v2");
>
>
>
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers
2018-01-15 8:47 ` Phil Reid
@ 2018-01-20 15:25 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2018-01-20 15:25 UTC (permalink / raw)
To: Phil Reid
Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, Michael.Hennerich,
linux-iio, devicetree
On Mon, 15 Jan 2018 16:47:35 +0800
Phil Reid <preid@electromag.com.au> wrote:
> G'day Jonathan,
>
> On 14/01/2018 21:39, Jonathan Cameron wrote:
> > On Wed, 10 Jan 2018 14:17:31 +0800
> > Phil Reid <preid@electromag.com.au> wrote:
> >
> >> Add implementation for Analog Devices AD5272 and AD5274 digital
> >> potentiometer devices.
> >>
> >> Signed-off-by: Phil Reid <preid@electromag.com.au>
> >
> > Hi Phil,
> >
> > A few minor comments inline, but looking pretty good.
>
> Thanks for the review.
>
> >
> > Jonathan
> >
> >> ---
> >> drivers/iio/potentiometer/Kconfig | 10 ++
> >> drivers/iio/potentiometer/Makefile | 1 +
> >> drivers/iio/potentiometer/ad5272.c | 219 +++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 230 insertions(+)
> >> create mode 100644 drivers/iio/potentiometer/ad5272.c
> >>
> >> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> >> index 8bf2825..0b04063 100644
> >> --- a/drivers/iio/potentiometer/Kconfig
> >> +++ b/drivers/iio/potentiometer/Kconfig
> >> @@ -5,6 +5,16 @@
> >>
> >> menu "Digital potentiometers"
> >>
> >> +config AD5272
> >> + tristate "Analog Devices AD5272 Digital Potentiometer driver"
> >> + depends on I2C
> >> + help
> >> + Say yes here to build support for the Analog Devices AD5272
> >> + digital potentiometer chip.
> >> +
> >> + To compile this driver as a module, choose M here: the
> >> + module will be called ad5272.
> >> +
> >> config DS1803
> >> tristate "Maxim Integrated DS1803 Digital Potentiometer driver"
> >> depends on I2C
> >> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> >> index 2260d40..498bbb8 100644
> >> --- a/drivers/iio/potentiometer/Makefile
> >> +++ b/drivers/iio/potentiometer/Makefile
> >> @@ -3,6 +3,7 @@
> >> #
> >>
> >> # When adding new entries keep the list in alphabetical order
> >> +obj-$(CONFIG_AD5272) += ad5272.o
> >> obj-$(CONFIG_DS1803) += ds1803.o
> >> obj-$(CONFIG_MAX5481) += max5481.o
> >> obj-$(CONFIG_MAX5487) += max5487.o
> >> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> >> new file mode 100644
> >> index 0000000..b9208c2
> >> --- /dev/null
> >> +++ b/drivers/iio/potentiometer/ad5272.c
> >> @@ -0,0 +1,219 @@
> >> +/*
> >> + * Analog Devices AD5372 digital potentiometer driver
> >> + * Copyright (C) 2018 Phil Reid <preid@electromag.com.au>
> >> + *
> >> + * Datasheet: http://www.analog.com/media/en/technical-documentation/data-sheets/AD5272_5274.pdf
> >> + *
> >> + * DEVID #Wipers #Positions Resistor Opts (kOhm) i2c address
> >> + * ad5272 1 1024 20, 50, 100 01011xx
> >> + * ad5274 1 256 20, 100 01011xx
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms of the GNU General Public License version 2 as published by
> >> + * the Free Software Foundation.
> >
> > Could go all SPDX with the gpl part of this. Personally I don't mind
> > much but we'll probably end up converting it over soon anyway.
> >
> >> + */
> >> +
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/module.h>
> >> +
> >> +#define AD5272_RDAC_WR 1
> >> +#define AD5272_RDAC_RD 2
> >> +#define AD5272_CTL 7
> >> +
> >> +#define AD5272_RDAC_WR_EN BIT(1)
> >> +
> >> +struct ad5272_cfg {
> >> + int max_pos;
> >> + int kohms;
> >> + int shift;
> >> +};
> >> +
> >> +enum ad5272_type {
> >> + AD5272_020,
> >> + AD5272_050,
> >> + AD5272_100,
> >> + AD5274_020,
> >> + AD5274_100,
> >> +};
> >> +
> >> +static const struct ad5272_cfg ad5272_cfg[] = {
> >> + /* on-semiconductor parts */
> >> + [AD5272_020] = { .max_pos = 1024, .kohms = 20 },
> >> + [AD5272_050] = { .max_pos = 1024, .kohms = 50 },
> >> + [AD5272_100] = { .max_pos = 1024, .kohms = 100 },
> >> + [AD5274_020] = { .max_pos = 256, .kohms = 20, .shift = 2 },
> >> + [AD5274_100] = { .max_pos = 256, .kohms = 100, .shift = 2 },
> >> +};
> >> +
> >> +struct ad5272_data {
> >> + struct i2c_client *client;
> >> + struct mutex lock;
> >> + const struct ad5272_cfg *cfg;
> >> +};
> >> +
> >> +static const struct iio_chan_spec ad5272_channel = {
> >> + .type = IIO_RESISTANCE,
> >> + .output = 1,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >> +};
> >> +
> >> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> >> +{
> >> + u8 buf[2];
> >> + int ret;
> >> +
> >> + buf[0] = ((reg & 0xf) << 2) | ((val >> 8) & 0x3);
> >> + buf[1] = (u8)val;
> >
> > again, could do as buf[2] = {...};
> ok
> >
> >> +
> >> + mutex_lock(&data->lock);
> >> + ret = i2c_master_send(data->client, buf, 2);
> >
> > sizeof(buf) preferred.
> ok
> >
> >> + mutex_unlock(&data->lock);
> >> + return ret < 0 ? ret : 0;
> >> +}
> >> +
> >> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> >> +{
> >> + u8 buf[2];
> >
> > Could assign value here for compactness.
> > u8 buf[2] = {(reg & 0xf) << 2, 0) >
> > It's a little paranoid to mask reg given you are controlling it entirely
> > in the driver and anything of more than 4 bits is a bug anyway.
> ok
>
> >> + int ret;
> >> +
> >> + buf[0] = (reg & 0xf) << 2;
> >> + buf[1] = 0;
> >> +
> >> + mutex_lock(&data->lock);
> >> + ret = i2c_master_send(data->client, buf, 2);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + ret = i2c_master_recv(data->client, buf, 2);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + *val = ((buf[0] & 0x3) << 8) | buf[1];
> >> + ret = 0;
> >> +error:
> >> + mutex_unlock(&data->lock);
> >> + return ret;
> >> +}
> >> +
> >> +static int ad5272_read_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan,
> >> + int *val, int *val2, long mask)
> >> +{
> >> + struct ad5272_data *data = iio_priv(indio_dev);
> >> + int ret;
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_RAW: {
> >> + ret = ad5272_read(data, AD5272_RDAC_RD, val);
> >> + *val = *val >> data->cfg->shift;
> >> + return ret ? ret : IIO_VAL_INT;
> >> + }
> >> + case IIO_CHAN_INFO_SCALE:
> >> + *val = 1000 * data->cfg->kohms;
> >> + *val2 = data->cfg->max_pos;
> >> + return IIO_VAL_FRACTIONAL;
> >> + }
> >> +
> >> + return -EINVAL;
> >> +}
> >> +
> >> +static int ad5272_write_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan,
> >> + int val, int val2, long mask)
> >> +{
> >> + struct ad5272_data *data = iio_priv(indio_dev);
> >> +
> >> + if (mask != IIO_CHAN_INFO_RAW)
> >> + return -EINVAL;
> >> +
> >> + if (val >= data->cfg->max_pos || val < 0)
> >> + return -EINVAL;
> >
> > Also verify that val2 = 0 as otherwise the classic
> > 1.999999 evaluates to 1 rather surprises users.
> yeap, that'd surprise me too.
>
> >
> >> +
> >> + val = val << data->cfg->shift;
> >
> > I would role this into the next line.
> ok
>
> >
> > return ad5272_write(data, AD5272_RDAC_WR, val << data->cfg->shift);
> >> +
> >> + return ad5272_write(data, AD5272_RDAC_WR, val);
> >> +}
> >> +
> >> +static const struct iio_info ad5272_info = {
> >> + .read_raw = ad5272_read_raw,
> >> + .write_raw = ad5272_write_raw,
> >> + .driver_module = THIS_MODULE,
> >
> > .driver_module no longer exists so just drop this.
> > We do the equivalent with some macro tricks to avoid having
> > to have this same line in every driver.
> ok
>
> >
> >> +};
> >> +
> >> +static int ad5272_probe(struct i2c_client *client,
> >> + const struct i2c_device_id *id)
> >> +{
> >> + struct device *dev = &client->dev;
> >> + struct gpio_desc *reset_gpio;
> >> + struct iio_dev *indio_dev;
> >> + struct ad5272_data *data;
> >> + int ret;
> >> +
> >> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> >> + GPIOD_OUT_LOW);
> >> + if (IS_ERR(reset_gpio))
> >> + return PTR_ERR(reset_gpio);
> >> +
> >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> >> + if (!indio_dev)
> >> + return -ENOMEM;
> >> +
> >> + i2c_set_clientdata(client, indio_dev);
> >> +
> >> + data = iio_priv(indio_dev);
> >> + data->client = client;
> >> + mutex_init(&data->lock);
> >> + data->cfg = &ad5272_cfg[id->driver_data];
> >
> > Calling it cfg kind of implies config to me whereas this is
> > chip type specific data.
> I just followed what seemed to be the existing convention for the iio/potentmeters.
> ds1803, mcp4531, max5481
>
> Happy to change.
>
> >
> >> +
> >> + ret = ad5272_write(data, AD5272_CTL, AD5272_RDAC_WR_EN);
> >> + if (ret < 0)
> >> + return -ENODEV;
> >> +
> >> + indio_dev->dev.parent = dev;
> >> + indio_dev->info = &ad5272_info;
> >> + indio_dev->channels = &ad5272_channel;
> >> + indio_dev->num_channels = 1;
> >> + indio_dev->name = client->name;
> >> +
> >> + return devm_iio_device_register(dev, indio_dev);
> >> +}
> >> +
> >> +#if defined(CONFIG_OF)
> >> +static const struct of_device_id ad5272_dt_ids[] = {
> >> + { .compatible = "adi,ad5272-020", .data = &ad5272_cfg[AD5272_020] },
> >
> > Why do this rather than stash the index in the data parameter and look up later
> > (more consistent with the i2c_device_id table below)?
>
> Yeah I was a bit confused by this.
> I was following how the ds1803.c driver was doing things.
> To be honest I don't have a good grasp of how .data works here or how it gets referenced or if it's
> even used...
Good point - you aren't currently using it.
This gets more interesting when i2c actually starts using the
of tables for devicetree rather than the older i2c_device_id tables.
The .data field is used for the same purpose as the indexes in
the other table, but you have to explicitly request it
with of_get_match_data
Jonathan
>
>
> >
> >> + { .compatible = "adi,ad5272-050", .data = &ad5272_cfg[AD5272_050] },
> >> + { .compatible = "adi,ad5272-100", .data = &ad5272_cfg[AD5272_100] },
> >> + { .compatible = "adi,ad5274-020", .data = &ad5272_cfg[AD5274_020] },
> >> + { .compatible = "adi,ad5274-100", .data = &ad5272_cfg[AD5274_100] },
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ad5272_dt_ids);
> >> +#endif /* CONFIG_OF */
> >> +
> >> +static const struct i2c_device_id ad5272_id[] = {
> >> + { "ad5272-020", AD5272_020 },
> >> + { "ad5272-050", AD5272_050 },
> >> + { "ad5272-100", AD5272_100 },
> >> + { "ad5274-020", AD5274_020 },
> >> + { "ad5274-100", AD5274_100 },
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, ad5272_id);
> >> +
> >> +static struct i2c_driver ad5272_driver = {
> >> + .driver = {
> >> + .name = "ad5272",
> >> + .of_match_table = of_match_ptr(ad5272_dt_ids),
> >> + },
> >> + .probe = ad5272_probe,
> >> + .id_table = ad5272_id,
> >> +};
> >> +
> >> +module_i2c_driver(ad5272_driver);
> >> +
> >> +MODULE_AUTHOR("Phil Reid <preid@eletromag.com.au>");
> >> +MODULE_DESCRIPTION("AD5272 digital potentiometer");
> >> +MODULE_LICENSE("GPL v2");
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread