[PATCH v2 0/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[PATCH v2 0/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Phil Reid]

Add documentation and driver for the Analog Devices 5272.

Driver currently doesn't support programing the RDAC position to 
non-volatile memory. 

Changes from v1:
- Use sizeof for i2c buffer lens
- Condense code a little when setting up i2c buf
- check val2 in raw write
- Add reset function. Perform a software reset if
  gpio reset pin is not defined
- remove THIS_MODULE assignment
- change of data to use index as per i2c device table


Phil Reid (2):
  dt-bindings: ad5272: Add bindings for Analog Devices digital
    potentiometers
  iio: ad5272: Add support for Analog Devices digital potentiometers

 .../bindings/iio/potentiometer/ad5272.txt          |  27 +++
 drivers/iio/potentiometer/Kconfig                  |  10 +
 drivers/iio/potentiometer/Makefile                 |   1 +
 drivers/iio/potentiometer/ad5272.c                 | 227 +++++++++++++++++++++
 4 files changed, 265 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt
 create mode 100644 drivers/iio/potentiometer/ad5272.c

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] dt-bindings: ad5272: Add bindings for Analog Devices digital potentiometers

[Phil Reid]

Add binding documentation for Analog Devices AD5272 and AD5274 digital
potentiometer devices.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 .../bindings/iio/potentiometer/ad5272.txt          | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt
new file mode 100644
index 0000000..dde4c67
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt
@@ -0,0 +1,27 @@
+* Analog Devices AD5272 digital potentiometer driver
+
+The node for this driver must be a child node of a I2C controller, hence
+all mandatory properties for your controller must be specified. See directory:
+
+        Documentation/devicetree/bindings/i2c
+
+for more details.
+
+Required properties:
+	- compatible:  	Must be one of the following, depending on the model:
+			adi,ad5272-020
+			adi,ad5272-050
+			adi,ad5272-100
+			adi,ad5274-020
+			adi,ad5274-100
+
+Optional properties:
+ - reset-gpios: GPIO specification for the RESET input. This is an
+		active low signal to the PCA953x.
+
+Example:
+ad5272: ad5272@2f {
+	reg = <0x2F>;
+	compatible = "adi,ad5272-020";
+	reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+};
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Phil Reid]

Add implementation for Analog Devices AD5272 and AD5274 digital
potentiometer devices.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/iio/potentiometer/Kconfig  |  10 ++
 drivers/iio/potentiometer/Makefile |   1 +
 drivers/iio/potentiometer/ad5272.c | 227 +++++++++++++++++++++++++++++++++++++
 3 files changed, 238 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..a04b018
--- /dev/null
+++ b/drivers/iio/potentiometer/ad5272.c
@@ -0,0 +1,227 @@
+/*
+ * Analog Devices AD5372 digital potentiometer driver
+ * Copyright (C) 2018 Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
+ *
+ * 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
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/delay.h>
+#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_RESET    4
+#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] = { (reg << 2) | ((val >> 8) & 0x3), (u8)val };
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = i2c_master_send(data->client, buf, sizeof(buf));
+	mutex_unlock(&data->lock);
+	return ret < 0 ? ret : 0;
+}
+
+static int ad5272_read(struct ad5272_data *data, int reg, int *val)
+{
+	u8 buf[2] = {reg << 2, 0};
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = i2c_master_send(data->client, buf, sizeof(buf));
+	if (ret < 0)
+		goto error;
+
+	ret = i2c_master_recv(data->client, buf, sizeof(buf));
+	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 || val2)
+		return -EINVAL;
+
+	return ad5272_write(data, AD5272_RDAC_WR, val << data->cfg->shift);
+}
+
+static const struct iio_info ad5272_info = {
+	.read_raw = ad5272_read_raw,
+	.write_raw = ad5272_write_raw,
+};
+
+static int ad5272_reset(struct ad5272_data *data)
+{
+	struct gpio_desc *reset_gpio;
+
+	reset_gpio = devm_gpiod_get_optional(&data->client->dev, "reset",
+		GPIOD_OUT_LOW);
+	if (IS_ERR(reset_gpio))
+		return PTR_ERR(reset_gpio);
+
+	if (reset_gpio) {
+		udelay(1);
+		gpiod_set_value(reset_gpio, 1);
+	} else {
+		ad5272_write(data, AD5272_RESET, 0);
+	}
+	usleep_range(1000, 2000);
+	return 0;
+}
+
+static int ad5272_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct ad5272_data *data;
+	int ret;
+
+	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_reset(data);
+	if (ret)
+		return ret;
+
+	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 = (void *)AD5272_020 },
+	{ .compatible = "adi,ad5272-050", .data = (void *)AD5272_050 },
+	{ .compatible = "adi,ad5272-100", .data = (void *)AD5272_100 },
+	{ .compatible = "adi,ad5274-020", .data = (void *)AD5274_020 },
+	{ .compatible = "adi,ad5274-100", .data = (void *)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-TeWIRZsGUAnP+dxuUfoltA@public.gmane.org>");
+MODULE_DESCRIPTION("AD5272 digital potentiometer");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

Re: [PATCH v2 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Lars-Peter Clausen]

On 01/16/2018 10:49 AM, Phil Reid wrote:
> Add implementation for Analog Devices AD5272 and AD5274 digital
> potentiometer devices.
> 
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

Hi,

Thanks for the patch. Looks mostly good.

> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> new file mode 100644
> index 0000000..a04b018
> --- /dev/null
> +++ b/drivers/iio/potentiometer/ad5272.c
> @@ -0,0 +1,227 @@
> +/*
> + * Analog Devices AD5372 digital potentiometer driver
> + * Copyright (C) 2018 Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> + *
> + * 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
> + *
> + * SPDX-License-Identifier: GPL-2.0+

People will probably complain about this, there are some weired rules where
and how the SPDX identifier should be.

> + */
[...]
> +
> +static const struct ad5272_cfg ad5272_cfg[] = {
> +	/* on-semiconductor parts */

Copy&paste error?

> +	[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 },
> +};
[...]
> +
> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> +{
> +	u8 buf[2] = { (reg << 2) | ((val >> 8) & 0x3), (u8)val };
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
> +	mutex_unlock(&data->lock);
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> +{
> +	u8 buf[2] = {reg << 2, 0};

I believe the general recommendation is to not use stack based transfer
buffer to avoid potential issues in case the I2C master driver uses DMA.
There are many examples IIO example drivers that use DMA safe transfer
buffers. Look for ____cacheline_aligned.

> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = i2c_master_recv(data->client, buf, sizeof(buf));
> +	if (ret < 0)
> +		goto error;
> +

This should be one I2C transfer, otherwise some other driver might get its
own transfer between the send and recv, which could cause issues.

> +	*val = ((buf[0] & 0x3) << 8) | buf[1];
> +	ret = 0;
> +error:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}

Re: [PATCH v2 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Phil Reid]

G'day Lars-Peter,

On 17/01/2018 01:01, Lars-Peter Clausen wrote:
> On 01/16/2018 10:49 AM, Phil Reid wrote:
>> Add implementation for Analog Devices AD5272 and AD5274 digital
>> potentiometer devices.
>>
>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> 
> Hi,
> 
> Thanks for the patch. Looks mostly good.
> 
>> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
>> new file mode 100644
>> index 0000000..a04b018
>> --- /dev/null
>> +++ b/drivers/iio/potentiometer/ad5272.c
>> @@ -0,0 +1,227 @@
>> +/*
>> + * Analog Devices AD5372 digital potentiometer driver
>> + * Copyright (C) 2018 Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>> + *
>> + * 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
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
> 
> People will probably complain about this, there are some weired rules where
> and how the SPDX identifier should be.
> 
Jonathan suggested this in his review of v1. Forgot to mention that in the change list.


>> + */
> [...]
>> +
>> +static const struct ad5272_cfg ad5272_cfg[] = {
>> +	/* on-semiconductor parts */
> 
> Copy&paste error?

yeap.
> 
>> +	[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 },
>> +};
> [...]
>> +
>> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
>> +{
>> +	u8 buf[2] = { (reg << 2) | ((val >> 8) & 0x3), (u8)val };
>> +	int ret;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
>> +	mutex_unlock(&data->lock);
>> +	return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
>> +{
>> +	u8 buf[2] = {reg << 2, 0};
> 
> I believe the general recommendation is to not use stack based transfer
> buffer to avoid potential issues in case the I2C master driver uses DMA.
> There are many examples IIO example drivers that use DMA safe transfer
> buffers. Look for ____cacheline_aligned.
ok.


> 
>> +	int ret;
>> +
>> +	mutex_lock(&data->lock);
>> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	ret = i2c_master_recv(data->client, buf, sizeof(buf));
>> +	if (ret < 0)
>> +		goto error;
>> +
> 
> This should be one I2C transfer, otherwise some other driver might get its
> own transfer between the send and recv, which could cause issues.

Yeah I considered that.
But the datasheet for the device states that a stop must be issued after the write,
before the read.
Docs for i2c_transfer state that the stop is issued only at the end.



> 
>> +	*val = ((buf[0] & 0x3) << 8) | buf[1];
>> +	ret = 0;
>> +error:
>> +	mutex_unlock(&data->lock);
>> +	return ret;
>> +}
> 
> 


-- 
Regards
Phil Reid
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Jonathan Cameron]

On Wed, 17 Jan 2018 09:46:23 +0800
Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> G'day Lars-Peter,
> 
> On 17/01/2018 01:01, Lars-Peter Clausen wrote:
> > On 01/16/2018 10:49 AM, Phil Reid wrote:  
> >> Add implementation for Analog Devices AD5272 and AD5274 digital
> >> potentiometer devices.
> >>
> >> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>  
> > 
> > Hi,
> > 
> > Thanks for the patch. Looks mostly good.
> >   
> >> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
> >> new file mode 100644
> >> index 0000000..a04b018
> >> --- /dev/null
> >> +++ b/drivers/iio/potentiometer/ad5272.c
> >> @@ -0,0 +1,227 @@
> >> +/*
> >> + * Analog Devices AD5372 digital potentiometer driver
> >> + * Copyright (C) 2018 Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> >> + *
> >> + * 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
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0+  
> > 
> > People will probably complain about this, there are some weired rules where
> > and how the SPDX identifier should be.
> >   
> Jonathan suggested this in his review of v1. Forgot to mention that in the change list.
Lars is referring to the 'where' element of using SPDX tags.  They go
on as a c++ style comment right at the top of the file.

Good think he noticed as I missed this when reviewing v3!

Jonathan

> 
> 
> >> + */  
> > [...]  
> >> +
> >> +static const struct ad5272_cfg ad5272_cfg[] = {
> >> +	/* on-semiconductor parts */  
> > 
> > Copy&paste error?  
> 
> yeap.
> >   
> >> +	[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 },
> >> +};  
> > [...]  
> >> +
> >> +static int ad5272_write(struct ad5272_data *data, int reg, int val)
> >> +{
> >> +	u8 buf[2] = { (reg << 2) | ((val >> 8) & 0x3), (u8)val };
> >> +	int ret;
> >> +
> >> +	mutex_lock(&data->lock);
> >> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
> >> +	mutex_unlock(&data->lock);
> >> +	return ret < 0 ? ret : 0;
> >> +}
> >> +
> >> +static int ad5272_read(struct ad5272_data *data, int reg, int *val)
> >> +{
> >> +	u8 buf[2] = {reg << 2, 0};  
> > 
> > I believe the general recommendation is to not use stack based transfer
> > buffer to avoid potential issues in case the I2C master driver uses DMA.
> > There are many examples IIO example drivers that use DMA safe transfer
> > buffers. Look for ____cacheline_aligned.  
> ok.
> 
> 
> >   
> >> +	int ret;
> >> +
> >> +	mutex_lock(&data->lock);
> >> +	ret = i2c_master_send(data->client, buf, sizeof(buf));
> >> +	if (ret < 0)
> >> +		goto error;
> >> +
> >> +	ret = i2c_master_recv(data->client, buf, sizeof(buf));
> >> +	if (ret < 0)
> >> +		goto error;
> >> +  
> > 
> > This should be one I2C transfer, otherwise some other driver might get its
> > own transfer between the send and recv, which could cause issues.  
> 
> Yeah I considered that.
> But the datasheet for the device states that a stop must be issued after the write,
> before the read.
> Docs for i2c_transfer state that the stop is issued only at the end.
> 
> 
> 
> >   
> >> +	*val = ((buf[0] & 0x3) << 8) | buf[1];
> >> +	ret = 0;
> >> +error:
> >> +	mutex_unlock(&data->lock);
> >> +	return ret;
> >> +}  
> > 
> >   
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] iio: ad5272: Add support for Analog Devices digital potentiometers

[Phil Reid]

On 21/01/2018 20:11, Jonathan Cameron wrote:
> On Wed, 17 Jan 2018 09:46:23 +0800
> Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:
> 
>> G'day Lars-Peter,
>>
>> On 17/01/2018 01:01, Lars-Peter Clausen wrote:
>>> On 01/16/2018 10:49 AM, Phil Reid wrote:
>>>> Add implementation for Analog Devices AD5272 and AD5274 digital
>>>> potentiometer devices.
>>>>
>>>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>>>
>>> Hi,
>>>
>>> Thanks for the patch. Looks mostly good.
>>>    
>>>> diff --git a/drivers/iio/potentiometer/ad5272.c b/drivers/iio/potentiometer/ad5272.c
>>>> new file mode 100644
>>>> index 0000000..a04b018
>>>> --- /dev/null
>>>> +++ b/drivers/iio/potentiometer/ad5272.c
>>>> @@ -0,0 +1,227 @@
>>>> +/*
>>>> + * Analog Devices AD5372 digital potentiometer driver
>>>> + * Copyright (C) 2018 Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>>>> + *
>>>> + * 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
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0+
>>>
>>> People will probably complain about this, there are some weired rules where
>>> and how the SPDX identifier should be.
>>>    
>> Jonathan suggested this in his review of v1. Forgot to mention that in the change list.
> Lars is referring to the 'where' element of using SPDX tags.  They go
> on as a c++ style comment right at the top of the file.
> 
> Good think he noticed as I missed this when reviewing v3!
> 
fair enough.

I did grep the kernel and found a mixture of placements so just picked the wrong one.
I also didn't find anything in the Documentation folder about it.
But my tree isn't at the bleeding edge.

-- 
Regards
Phil Reid

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html