All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 10:35 ` michael.hennerich
  0 siblings, 0 replies; 10+ messages in thread
From: michael.hennerich @ 2017-03-23 10:35 UTC (permalink / raw)
  To: jic23, lars, knaack.h, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, devicetree, linux-kernel, Michael Hennerich

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
 MAINTAINERS                                        |   1 +
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
 5 files changed, 289 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
 create mode 100644 drivers/iio/adc/ltc2497.c

diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
new file mode 100644
index 0000000..c2829c19
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
@@ -0,0 +1,13 @@
+* Linear Technology / Analog Devices LTC2497 ADC
+
+Required properties:
+ - compatible: Should be "lltc,ltc2497"
+ - reg: Should contain the ADC I2C address
+ - vref-supply: The regulator supply for ADC reference voltage
+
+Example:
+	ltc2497: adc@76 {
+		compatible = "lltc,ltc2497";
+		reg = <0x76>;
+		vref-supply = <&ltc2497_reg>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index a7d6f9a..173043c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
 W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/iio/*/ad*
+F:	drivers/iio/adc/ltc2497*
 X:	drivers/iio/*/adjd*
 F:	drivers/staging/iio/*/ad*
 F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6f..141cf4a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -326,6 +326,17 @@ config LTC2485
 	  To compile this driver as a module, choose M here: the module will be
 	  called ltc2485.
 
+config LTC2497
+	tristate "Linear Technology LTC2497 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Linear Technology LTC2497
+	  16-Bit 8-/16-Channel Delta Sigma ADC.
+	  Provides direct access via sysfs
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ltc2497.
+
 config MAX1027
 	tristate "Maxim max1027 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe39..9d626b5 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
+obj-$(CONFIG_LTC2497) += ltc2497.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
new file mode 100644
index 0000000..0452715
--- /dev/null
+++ b/drivers/iio/adc/ltc2497.c
@@ -0,0 +1,263 @@
+/*
+ * ltc2497.c - Driver for Linear Technology LTC2497 ADC
+ *
+ * Copyright (C) 2017 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ *
+ * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LTC2497_ENABLE			0xA0
+#define LTC2497_SGL			(1 << 4)
+#define LTC2497_DIFF			(0 << 4)
+#define LTC2497_SIGN			(1 << 3)
+#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
+#define LTC2497_CONVERSION_TIME_MS	150ULL
+
+struct ltc2497_st {
+	struct i2c_client *client;
+	struct regulator *ref;
+	ktime_t	time_prev;
+	u8 addr_prev;
+};
+
+static int ltc2497_wait_conv(struct ltc2497_st *st)
+{
+	s64 time_elapsed;
+
+	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
+
+	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
+		/* delay if conversion time not passed
+		 * since last read or write
+		 */
+		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
+		return 0;
+	}
+
+	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
+		/* We're in automatic mode -
+		 * so the last reading is stil not outdated
+		 */
+		return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
+{
+	struct i2c_client *client = st->client;
+	__be32 buf = 0;
+	int ret;
+
+	ret = ltc2497_wait_conv(st);
+	if (ret < 0 || st->addr_prev != address) {
+		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
+		if (ret < 0)
+			return ret;
+		st->addr_prev = address;
+		msleep(LTC2497_CONVERSION_TIME_MS);
+	}
+	ret = i2c_master_recv(client, (char *)&buf, 3);
+	if (ret < 0)  {
+		dev_err(&client->dev, "i2c_master_recv failed\n");
+		return ret;
+	}
+	st->time_prev = ktime_get();
+	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
+
+	return ret;
+}
+
+static int ltc2497_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ltc2497_st *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		ret = ltc2497_read(st, chan->address, val);
+		mutex_unlock(&indio_dev->mlock);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(st->ref);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
+		*val2 = 17;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+#define LTC2497_CHAN(_chan, _addr) { \
+	.type = IIO_VOLTAGE, \
+	.indexed = 1, \
+	.channel = (_chan), \
+	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+#define LTC2497_CHAN_DIFF(_chan, _addr) { \
+	.type = IIO_VOLTAGE, \
+	.indexed = 1, \
+	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
+	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
+	.address = (_addr | _chan), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+	.differential = 1, \
+}
+
+static const struct iio_chan_spec ltc2497_channel[] = {
+	LTC2497_CHAN(0, LTC2497_SGL),
+	LTC2497_CHAN(1, LTC2497_SGL),
+	LTC2497_CHAN(2, LTC2497_SGL),
+	LTC2497_CHAN(3, LTC2497_SGL),
+	LTC2497_CHAN(4, LTC2497_SGL),
+	LTC2497_CHAN(5, LTC2497_SGL),
+	LTC2497_CHAN(6, LTC2497_SGL),
+	LTC2497_CHAN(7, LTC2497_SGL),
+	LTC2497_CHAN(8, LTC2497_SGL),
+	LTC2497_CHAN(9, LTC2497_SGL),
+	LTC2497_CHAN(10, LTC2497_SGL),
+	LTC2497_CHAN(11, LTC2497_SGL),
+	LTC2497_CHAN(12, LTC2497_SGL),
+	LTC2497_CHAN(13, LTC2497_SGL),
+	LTC2497_CHAN(14, LTC2497_SGL),
+	LTC2497_CHAN(15, LTC2497_SGL),
+	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
+};
+
+static const struct iio_info ltc2497_info = {
+	.read_raw = ltc2497_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int ltc2497_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct ltc2497_st *st;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
+				     I2C_FUNC_SMBUS_WRITE_BYTE))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	st->client = client;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = id->name;
+	indio_dev->info = &ltc2497_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ltc2497_channel;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
+
+	st->ref = devm_regulator_get(&client->dev, "vref");
+	if (IS_ERR(st->ref))
+		return PTR_ERR(st->ref);
+
+	ret = regulator_enable(st->ref);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
+	if (ret < 0)
+		goto err_regulator_disable;
+
+	st->addr_prev = LTC2497_CONFIG_DEFAULT;
+	st->time_prev = ktime_get();
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0)
+		goto err_regulator_disable;
+
+	return 0;
+
+err_regulator_disable:
+	regulator_disable(st->ref);
+
+	return ret;
+}
+
+static int ltc2497_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ltc2497_st *st = iio_priv(indio_dev);
+
+	regulator_disable(st->ref);
+
+	return 0;
+}
+
+static const struct i2c_device_id ltc2497_id[] = {
+	{ "ltc2497", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ltc2497_id);
+
+static const struct of_device_id ltc2497_of_match[] = {
+	{ .compatible = "lltc,ltc2497", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ltc2497_of_match);
+
+static struct i2c_driver ltc2497_driver = {
+	.driver = {
+		.name = "ltc2497",
+		.of_match_table = of_match_ptr(ltc2497_of_match),
+	},
+	.probe = ltc2497_probe,
+	.remove = ltc2497_remove,
+	.id_table = ltc2497_id,
+};
+module_i2c_driver(ltc2497_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 10:35 ` michael.hennerich
  0 siblings, 0 replies; 10+ messages in thread
From: michael.hennerich @ 2017-03-23 10:35 UTC (permalink / raw)
  To: jic23, lars, knaack.h, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, devicetree, linux-kernel, Michael Hennerich

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
 MAINTAINERS                                        |   1 +
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
 5 files changed, 289 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
 create mode 100644 drivers/iio/adc/ltc2497.c

diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
new file mode 100644
index 0000000..c2829c19
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
@@ -0,0 +1,13 @@
+* Linear Technology / Analog Devices LTC2497 ADC
+
+Required properties:
+ - compatible: Should be "lltc,ltc2497"
+ - reg: Should contain the ADC I2C address
+ - vref-supply: The regulator supply for ADC reference voltage
+
+Example:
+	ltc2497: adc@76 {
+		compatible = "lltc,ltc2497";
+		reg = <0x76>;
+		vref-supply = <&ltc2497_reg>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index a7d6f9a..173043c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
 W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/iio/*/ad*
+F:	drivers/iio/adc/ltc2497*
 X:	drivers/iio/*/adjd*
 F:	drivers/staging/iio/*/ad*
 F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6f..141cf4a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -326,6 +326,17 @@ config LTC2485
 	  To compile this driver as a module, choose M here: the module will be
 	  called ltc2485.
 
+config LTC2497
+	tristate "Linear Technology LTC2497 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Linear Technology LTC2497
+	  16-Bit 8-/16-Channel Delta Sigma ADC.
+	  Provides direct access via sysfs
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ltc2497.
+
 config MAX1027
 	tristate "Maxim max1027 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe39..9d626b5 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
+obj-$(CONFIG_LTC2497) += ltc2497.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
new file mode 100644
index 0000000..0452715
--- /dev/null
+++ b/drivers/iio/adc/ltc2497.c
@@ -0,0 +1,263 @@
+/*
+ * ltc2497.c - Driver for Linear Technology LTC2497 ADC
+ *
+ * Copyright (C) 2017 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ *
+ * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LTC2497_ENABLE			0xA0
+#define LTC2497_SGL			(1 << 4)
+#define LTC2497_DIFF			(0 << 4)
+#define LTC2497_SIGN			(1 << 3)
+#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
+#define LTC2497_CONVERSION_TIME_MS	150ULL
+
+struct ltc2497_st {
+	struct i2c_client *client;
+	struct regulator *ref;
+	ktime_t	time_prev;
+	u8 addr_prev;
+};
+
+static int ltc2497_wait_conv(struct ltc2497_st *st)
+{
+	s64 time_elapsed;
+
+	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
+
+	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
+		/* delay if conversion time not passed
+		 * since last read or write
+		 */
+		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
+		return 0;
+	}
+
+	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
+		/* We're in automatic mode -
+		 * so the last reading is stil not outdated
+		 */
+		return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
+{
+	struct i2c_client *client = st->client;
+	__be32 buf = 0;
+	int ret;
+
+	ret = ltc2497_wait_conv(st);
+	if (ret < 0 || st->addr_prev != address) {
+		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
+		if (ret < 0)
+			return ret;
+		st->addr_prev = address;
+		msleep(LTC2497_CONVERSION_TIME_MS);
+	}
+	ret = i2c_master_recv(client, (char *)&buf, 3);
+	if (ret < 0)  {
+		dev_err(&client->dev, "i2c_master_recv failed\n");
+		return ret;
+	}
+	st->time_prev = ktime_get();
+	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
+
+	return ret;
+}
+
+static int ltc2497_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ltc2497_st *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		ret = ltc2497_read(st, chan->address, val);
+		mutex_unlock(&indio_dev->mlock);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(st->ref);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
+		*val2 = 17;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+#define LTC2497_CHAN(_chan, _addr) { \
+	.type = IIO_VOLTAGE, \
+	.indexed = 1, \
+	.channel = (_chan), \
+	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+#define LTC2497_CHAN_DIFF(_chan, _addr) { \
+	.type = IIO_VOLTAGE, \
+	.indexed = 1, \
+	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
+	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
+	.address = (_addr | _chan), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+	.differential = 1, \
+}
+
+static const struct iio_chan_spec ltc2497_channel[] = {
+	LTC2497_CHAN(0, LTC2497_SGL),
+	LTC2497_CHAN(1, LTC2497_SGL),
+	LTC2497_CHAN(2, LTC2497_SGL),
+	LTC2497_CHAN(3, LTC2497_SGL),
+	LTC2497_CHAN(4, LTC2497_SGL),
+	LTC2497_CHAN(5, LTC2497_SGL),
+	LTC2497_CHAN(6, LTC2497_SGL),
+	LTC2497_CHAN(7, LTC2497_SGL),
+	LTC2497_CHAN(8, LTC2497_SGL),
+	LTC2497_CHAN(9, LTC2497_SGL),
+	LTC2497_CHAN(10, LTC2497_SGL),
+	LTC2497_CHAN(11, LTC2497_SGL),
+	LTC2497_CHAN(12, LTC2497_SGL),
+	LTC2497_CHAN(13, LTC2497_SGL),
+	LTC2497_CHAN(14, LTC2497_SGL),
+	LTC2497_CHAN(15, LTC2497_SGL),
+	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
+	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
+	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
+};
+
+static const struct iio_info ltc2497_info = {
+	.read_raw = ltc2497_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int ltc2497_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct ltc2497_st *st;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
+				     I2C_FUNC_SMBUS_WRITE_BYTE))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	st->client = client;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = id->name;
+	indio_dev->info = &ltc2497_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ltc2497_channel;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
+
+	st->ref = devm_regulator_get(&client->dev, "vref");
+	if (IS_ERR(st->ref))
+		return PTR_ERR(st->ref);
+
+	ret = regulator_enable(st->ref);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
+	if (ret < 0)
+		goto err_regulator_disable;
+
+	st->addr_prev = LTC2497_CONFIG_DEFAULT;
+	st->time_prev = ktime_get();
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0)
+		goto err_regulator_disable;
+
+	return 0;
+
+err_regulator_disable:
+	regulator_disable(st->ref);
+
+	return ret;
+}
+
+static int ltc2497_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ltc2497_st *st = iio_priv(indio_dev);
+
+	regulator_disable(st->ref);
+
+	return 0;
+}
+
+static const struct i2c_device_id ltc2497_id[] = {
+	{ "ltc2497", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ltc2497_id);
+
+static const struct of_device_id ltc2497_of_match[] = {
+	{ .compatible = "lltc,ltc2497", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ltc2497_of_match);
+
+static struct i2c_driver ltc2497_driver = {
+	.driver = {
+		.name = "ltc2497",
+		.of_match_table = of_match_ptr(ltc2497_of_match),
+	},
+	.probe = ltc2497_probe,
+	.remove = ltc2497_remove,
+	.id_table = ltc2497_id,
+};
+module_i2c_driver(ltc2497_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 11:05   ` Lars-Peter Clausen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars-Peter Clausen @ 2017-03-23 11:05 UTC (permalink / raw)
  To: michael.hennerich, jic23, knaack.h, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, devicetree, linux-kernel

The subject should follow the standard subsystem subject scheme. E.g. in
this case "iio: adc: Add driver for ..."

On 03/23/2017 11:35 AM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>

Needs a commit message.

> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>  5 files changed, 289 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..c2829c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Should be "lltc,ltc2497"

Replace 'should' with 'must'. It won't work if it isn't.

> + - reg: Should contain the ADC I2C address

Same here.

> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
[...]
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..0452715
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,263 @@
> +/*
> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC

That's the "Analog Devices LTC2497 ADC" now ;)

> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of.h>

Sorting alphabetically is preferred.

> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			(1 << 4)

I'm sure 5 minutes after the patch has been applied somebody will send a
patch to replace this with BIT(4)

> +#define LTC2497_DIFF			(0 << 4)
> +#define LTC2497_SIGN			(1 << 3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
> +
> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);

Considering how long this sleeps msleep_interruptible() might be the better
choice.

> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	__be32 buf = 0;

transfer buffers must not be on the stack to avoid issues if the controller
should use DMA.

> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0 || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		msleep(LTC2497_CONVERSION_TIME_MS);
> +	}
> +	ret = i2c_master_recv(client, (char *)&buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
> +
> +	return ret;
> +}
[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 11:05   ` Lars-Peter Clausen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars-Peter Clausen @ 2017-03-23 11:05 UTC (permalink / raw)
  To: michael.hennerich-OyLXuOCK7orQT0dZR+AlfA,
	jic23-DgEjT+Ai2ygdnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
	pmeerw-jW+XmwGofnusTnJN9+BGXg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

The subject should follow the standard subsystem subject scheme. E.g. in
this case "iio: adc: Add driver for ..."

On 03/23/2017 11:35 AM, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

Needs a commit message.

> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>  5 files changed, 289 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..c2829c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Should be "lltc,ltc2497"

Replace 'should' with 'must'. It won't work if it isn't.

> + - reg: Should contain the ADC I2C address

Same here.

> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
[...]
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..0452715
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,263 @@
> +/*
> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC

That's the "Analog Devices LTC2497 ADC" now ;)

> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of.h>

Sorting alphabetically is preferred.

> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			(1 << 4)

I'm sure 5 minutes after the patch has been applied somebody will send a
patch to replace this with BIT(4)

> +#define LTC2497_DIFF			(0 << 4)
> +#define LTC2497_SIGN			(1 << 3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
> +
> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);

Considering how long this sleeps msleep_interruptible() might be the better
choice.

> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	__be32 buf = 0;

transfer buffers must not be on the stack to avoid issues if the controller
should use DMA.

> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0 || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		msleep(LTC2497_CONVERSION_TIME_MS);
> +	}
> +	ret = i2c_master_recv(client, (char *)&buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
> +
> +	return ret;
> +}
[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 11:28   ` Peter Meerwald-Stadler
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-23 11:28 UTC (permalink / raw)
  To: Michael Hennerich
  Cc: jic23, lars, knaack.h, robh+dt, mark.rutland, linux-iio,
	devicetree, linux-kernel

On Thu, 23 Mar 2017, michael.hennerich@analog.com wrote:

> From: Michael Hennerich <michael.hennerich@analog.com>

comments below
 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>  5 files changed, 289 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..c2829c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Should be "lltc,ltc2497"
> + - reg: Should contain the ADC I2C address
> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7d6f9a..173043c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>  W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/iio/*/ad*
> +F:	drivers/iio/adc/ltc2497*
>  X:	drivers/iio/*/adjd*
>  F:	drivers/staging/iio/*/ad*
>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6f..141cf4a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -326,6 +326,17 @@ config LTC2485
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2485.
>  
> +config LTC2497
> +	tristate "Linear Technology LTC2497 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Linear Technology LTC2497
> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
> +	  Provides direct access via sysfs

the line "Provides..." should be removed I think, it confuses me at least

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ltc2497.
> +
>  config MAX1027
>  	tristate "Maxim max1027 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe39..9d626b5 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
> +obj-$(CONFIG_LTC2497) += ltc2497.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..0452715
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,263 @@
> +/*
> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC
> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			(1 << 4)
> +#define LTC2497_DIFF			(0 << 4)
> +#define LTC2497_SIGN			(1 << 3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
 
is the ULL really needed?

> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	__be32 buf = 0;
> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0 || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);

use LTC2497_ENABLE instead of 0xa0

> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		msleep(LTC2497_CONVERSION_TIME_MS);
> +	}
> +	ret = i2c_master_recv(client, (char *)&buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);

what is the purpose of - (1 << 17)? maybe add a comment?
I'd rather do it with bit operations that arithmetic

> +
> +	return ret;
> +}
> +
> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		ret = ltc2497_read(st, chan->address, val);
> +		mutex_unlock(&indio_dev->mlock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(st->ref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = 17;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define LTC2497_CHAN(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan), \
> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
> +	.address = (_addr | _chan), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.differential = 1, \
> +}
> +
> +static const struct iio_chan_spec ltc2497_channel[] = {
> +	LTC2497_CHAN(0, LTC2497_SGL),
> +	LTC2497_CHAN(1, LTC2497_SGL),
> +	LTC2497_CHAN(2, LTC2497_SGL),
> +	LTC2497_CHAN(3, LTC2497_SGL),
> +	LTC2497_CHAN(4, LTC2497_SGL),
> +	LTC2497_CHAN(5, LTC2497_SGL),
> +	LTC2497_CHAN(6, LTC2497_SGL),
> +	LTC2497_CHAN(7, LTC2497_SGL),
> +	LTC2497_CHAN(8, LTC2497_SGL),
> +	LTC2497_CHAN(9, LTC2497_SGL),
> +	LTC2497_CHAN(10, LTC2497_SGL),
> +	LTC2497_CHAN(11, LTC2497_SGL),
> +	LTC2497_CHAN(12, LTC2497_SGL),
> +	LTC2497_CHAN(13, LTC2497_SGL),
> +	LTC2497_CHAN(14, LTC2497_SGL),
> +	LTC2497_CHAN(15, LTC2497_SGL),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
> +};
> +
> +static const struct iio_info ltc2497_info = {
> +	.read_raw = ltc2497_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int ltc2497_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2497_st *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	st->client = client;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->info = &ltc2497_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2497_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
> +
> +	st->ref = devm_regulator_get(&client->dev, "vref");
> +	if (IS_ERR(st->ref))
> +		return PTR_ERR(st->ref);
> +
> +	ret = regulator_enable(st->ref);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
> +	st->time_prev = ktime_get();
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);

no devm_ since we have a non-empty _remove()?

> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	return 0;
> +
> +err_regulator_disable:
> +	regulator_disable(st->ref);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +
> +	regulator_disable(st->ref);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ltc2497_id[] = {
> +	{ "ltc2497", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
> +
> +static const struct of_device_id ltc2497_of_match[] = {
> +	{ .compatible = "lltc,ltc2497", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> +
> +static struct i2c_driver ltc2497_driver = {
> +	.driver = {
> +		.name = "ltc2497",
> +		.of_match_table = of_match_ptr(ltc2497_of_match),
> +	},
> +	.probe = ltc2497_probe,
> +	.remove = ltc2497_remove,
> +	.id_table = ltc2497_id,
> +};
> +module_i2c_driver(ltc2497_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 11:28   ` Peter Meerwald-Stadler
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Meerwald-Stadler @ 2017-03-23 11:28 UTC (permalink / raw)
  To: Michael Hennerich
  Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, lars-Qo5EllUWu/uELgA04lAiVw,
	knaack.h-Mmb7MZpHnFY, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Thu, 23 Mar 2017, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:

> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

comments below
 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>  5 files changed, 289 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..c2829c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Should be "lltc,ltc2497"
> + - reg: Should contain the ADC I2C address
> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7d6f9a..173043c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>  W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/iio/*/ad*
> +F:	drivers/iio/adc/ltc2497*
>  X:	drivers/iio/*/adjd*
>  F:	drivers/staging/iio/*/ad*
>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6f..141cf4a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -326,6 +326,17 @@ config LTC2485
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2485.
>  
> +config LTC2497
> +	tristate "Linear Technology LTC2497 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Linear Technology LTC2497
> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
> +	  Provides direct access via sysfs

the line "Provides..." should be removed I think, it confuses me at least

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ltc2497.
> +
>  config MAX1027
>  	tristate "Maxim max1027 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe39..9d626b5 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
> +obj-$(CONFIG_LTC2497) += ltc2497.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..0452715
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,263 @@
> +/*
> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC
> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			(1 << 4)
> +#define LTC2497_DIFF			(0 << 4)
> +#define LTC2497_SIGN			(1 << 3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
 
is the ULL really needed?

> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	__be32 buf = 0;
> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0 || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);

use LTC2497_ENABLE instead of 0xa0

> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		msleep(LTC2497_CONVERSION_TIME_MS);
> +	}
> +	ret = i2c_master_recv(client, (char *)&buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);

what is the purpose of - (1 << 17)? maybe add a comment?
I'd rather do it with bit operations that arithmetic

> +
> +	return ret;
> +}
> +
> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		ret = ltc2497_read(st, chan->address, val);
> +		mutex_unlock(&indio_dev->mlock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(st->ref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = 17;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define LTC2497_CHAN(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan), \
> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
> +	.address = (_addr | _chan), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.differential = 1, \
> +}
> +
> +static const struct iio_chan_spec ltc2497_channel[] = {
> +	LTC2497_CHAN(0, LTC2497_SGL),
> +	LTC2497_CHAN(1, LTC2497_SGL),
> +	LTC2497_CHAN(2, LTC2497_SGL),
> +	LTC2497_CHAN(3, LTC2497_SGL),
> +	LTC2497_CHAN(4, LTC2497_SGL),
> +	LTC2497_CHAN(5, LTC2497_SGL),
> +	LTC2497_CHAN(6, LTC2497_SGL),
> +	LTC2497_CHAN(7, LTC2497_SGL),
> +	LTC2497_CHAN(8, LTC2497_SGL),
> +	LTC2497_CHAN(9, LTC2497_SGL),
> +	LTC2497_CHAN(10, LTC2497_SGL),
> +	LTC2497_CHAN(11, LTC2497_SGL),
> +	LTC2497_CHAN(12, LTC2497_SGL),
> +	LTC2497_CHAN(13, LTC2497_SGL),
> +	LTC2497_CHAN(14, LTC2497_SGL),
> +	LTC2497_CHAN(15, LTC2497_SGL),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
> +};
> +
> +static const struct iio_info ltc2497_info = {
> +	.read_raw = ltc2497_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int ltc2497_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2497_st *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	st->client = client;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->info = &ltc2497_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2497_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
> +
> +	st->ref = devm_regulator_get(&client->dev, "vref");
> +	if (IS_ERR(st->ref))
> +		return PTR_ERR(st->ref);
> +
> +	ret = regulator_enable(st->ref);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
> +	st->time_prev = ktime_get();
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);

no devm_ since we have a non-empty _remove()?

> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	return 0;
> +
> +err_regulator_disable:
> +	regulator_disable(st->ref);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +
> +	regulator_disable(st->ref);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ltc2497_id[] = {
> +	{ "ltc2497", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
> +
> +static const struct of_device_id ltc2497_of_match[] = {
> +	{ .compatible = "lltc,ltc2497", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> +
> +static struct i2c_driver ltc2497_driver = {
> +	.driver = {
> +		.name = "ltc2497",
> +		.of_match_table = of_match_ptr(ltc2497_of_match),
> +	},
> +	.probe = ltc2497_probe,
> +	.remove = ltc2497_remove,
> +	.id_table = ltc2497_id,
> +};
> +module_i2c_driver(ltc2497_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
--
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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
  2017-03-23 11:28   ` Peter Meerwald-Stadler
@ 2017-03-23 14:45     ` Michael Hennerich
  -1 siblings, 0 replies; 10+ messages in thread
From: Michael Hennerich @ 2017-03-23 14:45 UTC (permalink / raw)
  To: Peter Meerwald-Stadler
  Cc: jic23, lars, knaack.h, robh+dt, mark.rutland, linux-iio,
	devicetree, linux-kernel

On 23.03.2017 12:28, Peter Meerwald-Stadler wrote:
> On Thu, 23 Mar 2017, michael.hennerich@analog.com wrote:
>
>> From: Michael Hennerich <michael.hennerich@analog.com>
>
> comments below

Hi Peter,

Thanks for the review -
comments inline.

>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> ---
>>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>>  MAINTAINERS                                        |   1 +
>>  drivers/iio/adc/Kconfig                            |  11 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>>  5 files changed, 289 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>>  create mode 100644 drivers/iio/adc/ltc2497.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>> new file mode 100644
>> index 0000000..c2829c19
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>> @@ -0,0 +1,13 @@
>> +* Linear Technology / Analog Devices LTC2497 ADC
>> +
>> +Required properties:
>> + - compatible: Should be "lltc,ltc2497"
>> + - reg: Should contain the ADC I2C address
>> + - vref-supply: The regulator supply for ADC reference voltage
>> +
>> +Example:
>> +	ltc2497: adc@76 {
>> +		compatible = "lltc,ltc2497";
>> +		reg = <0x76>;
>> +		vref-supply = <&ltc2497_reg>;
>> +	};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a7d6f9a..173043c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>>  W:	http://ez.analog.com/community/linux-device-drivers
>>  S:	Supported
>>  F:	drivers/iio/*/ad*
>> +F:	drivers/iio/adc/ltc2497*
>>  X:	drivers/iio/*/adjd*
>>  F:	drivers/staging/iio/*/ad*
>>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 2268a6f..141cf4a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -326,6 +326,17 @@ config LTC2485
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called ltc2485.
>>
>> +config LTC2497
>> +	tristate "Linear Technology LTC2497 ADC driver"
>> +	depends on I2C
>> +	help
>> +	  Say yes here to build support for Linear Technology LTC2497
>> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
>> +	  Provides direct access via sysfs
>
> the line "Provides..." should be removed I think, it confuses me at least

Sure I can remove - originally added to make checkpatch happy.

>
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called ltc2497.
>> +
>>  config MAX1027
>>  	tristate "Maxim max1027 ADC driver"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 73dbe39..9d626b5 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>> +obj-$(CONFIG_LTC2497) += ltc2497.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>>  obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
>> new file mode 100644
>> index 0000000..0452715
>> --- /dev/null
>> +++ b/drivers/iio/adc/ltc2497.c
>> @@ -0,0 +1,263 @@
>> +/*
>> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC
>> + *
>> + * Copyright (C) 2017 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2.
>> + *
>> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define LTC2497_ENABLE			0xA0
>> +#define LTC2497_SGL			(1 << 4)
>> +#define LTC2497_DIFF			(0 << 4)
>> +#define LTC2497_SIGN			(1 << 3)
>> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
>> +#define LTC2497_CONVERSION_TIME_MS	150ULL
>
> is the ULL really needed?

Needed? - maybe not - but probably not wrong ktime_ms_delta() returns 
64bit and we later do some comparisons...

>
>> +struct ltc2497_st {
>> +	struct i2c_client *client;
>> +	struct regulator *ref;
>> +	ktime_t	time_prev;
>> +	u8 addr_prev;
>> +};
>> +
>> +static int ltc2497_wait_conv(struct ltc2497_st *st)
>> +{
>> +	s64 time_elapsed;
>> +
>> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
>> +
>> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
>> +		/* delay if conversion time not passed
>> +		 * since last read or write
>> +		 */
>> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
>> +		return 0;
>> +	}
>> +
>> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
>> +		/* We're in automatic mode -
>> +		 * so the last reading is stil not outdated
>> +		 */
>> +		return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
>> +{
>> +	struct i2c_client *client = st->client;
>> +	__be32 buf = 0;
>> +	int ret;
>> +
>> +	ret = ltc2497_wait_conv(st);
>> +	if (ret < 0 || st->addr_prev != address) {
>> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
>
> use LTC2497_ENABLE instead of 0xa0

good catch.

>
>> +		if (ret < 0)
>> +			return ret;
>> +		st->addr_prev = address;
>> +		msleep(LTC2497_CONVERSION_TIME_MS);
>> +	}
>> +	ret = i2c_master_recv(client, (char *)&buf, 3);
>> +	if (ret < 0)  {
>> +		dev_err(&client->dev, "i2c_master_recv failed\n");
>> +		return ret;
>> +	}
>> +	st->time_prev = ktime_get();
>> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
>
> what is the purpose of - (1 << 17)? maybe add a comment?
> I'd rather do it with bit operations that arithmetic

Well - this is for offset binary to signed conversion.
I'll add a comment.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct ltc2497_st *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +		ret = ltc2497_read(st, chan->address, val);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		ret = regulator_get_voltage(st->ref);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*val = ret / 1000;
>> +		*val2 = 17;
>> +
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +#define LTC2497_CHAN(_chan, _addr) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.indexed = 1, \
>> +	.channel = (_chan), \
>> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>> +}
>> +
>> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.indexed = 1, \
>> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
>> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
>> +	.address = (_addr | _chan), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>> +	.differential = 1, \
>> +}
>> +
>> +static const struct iio_chan_spec ltc2497_channel[] = {
>> +	LTC2497_CHAN(0, LTC2497_SGL),
>> +	LTC2497_CHAN(1, LTC2497_SGL),
>> +	LTC2497_CHAN(2, LTC2497_SGL),
>> +	LTC2497_CHAN(3, LTC2497_SGL),
>> +	LTC2497_CHAN(4, LTC2497_SGL),
>> +	LTC2497_CHAN(5, LTC2497_SGL),
>> +	LTC2497_CHAN(6, LTC2497_SGL),
>> +	LTC2497_CHAN(7, LTC2497_SGL),
>> +	LTC2497_CHAN(8, LTC2497_SGL),
>> +	LTC2497_CHAN(9, LTC2497_SGL),
>> +	LTC2497_CHAN(10, LTC2497_SGL),
>> +	LTC2497_CHAN(11, LTC2497_SGL),
>> +	LTC2497_CHAN(12, LTC2497_SGL),
>> +	LTC2497_CHAN(13, LTC2497_SGL),
>> +	LTC2497_CHAN(14, LTC2497_SGL),
>> +	LTC2497_CHAN(15, LTC2497_SGL),
>> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
>> +};
>> +
>> +static const struct iio_info ltc2497_info = {
>> +	.read_raw = ltc2497_read_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static int ltc2497_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct ltc2497_st *st;
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
>> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
>> +		return -EOPNOTSUPP;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
>> +	st->client = client;
>> +
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->name = id->name;
>> +	indio_dev->info = &ltc2497_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ltc2497_channel;
>> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
>> +
>> +	st->ref = devm_regulator_get(&client->dev, "vref");
>> +	if (IS_ERR(st->ref))
>> +		return PTR_ERR(st->ref);
>> +
>> +	ret = regulator_enable(st->ref);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
>> +	if (ret < 0)
>> +		goto err_regulator_disable;
>> +
>> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
>> +	st->time_prev = ktime_get();
>> +
>> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>
> no devm_ since we have a non-empty _remove()?

ok

>
>> +	if (ret < 0)
>> +		goto err_regulator_disable;
>> +
>> +	return 0;
>> +
>> +err_regulator_disable:
>> +	regulator_disable(st->ref);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltc2497_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct ltc2497_st *st = iio_priv(indio_dev);
>> +
>> +	regulator_disable(st->ref);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id ltc2497_id[] = {
>> +	{ "ltc2497", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
>> +
>> +static const struct of_device_id ltc2497_of_match[] = {
>> +	{ .compatible = "lltc,ltc2497", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
>> +
>> +static struct i2c_driver ltc2497_driver = {
>> +	.driver = {
>> +		.name = "ltc2497",
>> +		.of_match_table = of_match_ptr(ltc2497_of_match),
>> +	},
>> +	.probe = ltc2497_probe,
>> +	.remove = ltc2497_remove,
>> +	.id_table = ltc2497_id,
>> +};
>> +module_i2c_driver(ltc2497_driver);
>> +
>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
>> +MODULE_LICENSE("GPL v2");
>>
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Otl-Aicher Strasse 60-64      80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-23 14:45     ` Michael Hennerich
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Hennerich @ 2017-03-23 14:45 UTC (permalink / raw)
  To: Peter Meerwald-Stadler
  Cc: jic23, lars, knaack.h, robh+dt, mark.rutland, linux-iio,
	devicetree, linux-kernel

On 23.03.2017 12:28, Peter Meerwald-Stadler wrote:
> On Thu, 23 Mar 2017, michael.hennerich@analog.com wrote:
>
>> From: Michael Hennerich <michael.hennerich@analog.com>
>
> comments below

Hi Peter,

Thanks for the review -
comments inline.

>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> ---
>>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>>  MAINTAINERS                                        |   1 +
>>  drivers/iio/adc/Kconfig                            |  11 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/ltc2497.c                          | 263 +++++++++++++++++++++
>>  5 files changed, 289 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>>  create mode 100644 drivers/iio/adc/ltc2497.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>> new file mode 100644
>> index 0000000..c2829c19
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>> @@ -0,0 +1,13 @@
>> +* Linear Technology / Analog Devices LTC2497 ADC
>> +
>> +Required properties:
>> + - compatible: Should be "lltc,ltc2497"
>> + - reg: Should contain the ADC I2C address
>> + - vref-supply: The regulator supply for ADC reference voltage
>> +
>> +Example:
>> +	ltc2497: adc@76 {
>> +		compatible = "lltc,ltc2497";
>> +		reg = <0x76>;
>> +		vref-supply = <&ltc2497_reg>;
>> +	};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a7d6f9a..173043c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>>  W:	http://ez.analog.com/community/linux-device-drivers
>>  S:	Supported
>>  F:	drivers/iio/*/ad*
>> +F:	drivers/iio/adc/ltc2497*
>>  X:	drivers/iio/*/adjd*
>>  F:	drivers/staging/iio/*/ad*
>>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 2268a6f..141cf4a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -326,6 +326,17 @@ config LTC2485
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called ltc2485.
>>
>> +config LTC2497
>> +	tristate "Linear Technology LTC2497 ADC driver"
>> +	depends on I2C
>> +	help
>> +	  Say yes here to build support for Linear Technology LTC2497
>> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
>> +	  Provides direct access via sysfs
>
> the line "Provides..." should be removed I think, it confuses me at least

Sure I can remove - originally added to make checkpatch happy.

>
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called ltc2497.
>> +
>>  config MAX1027
>>  	tristate "Maxim max1027 ADC driver"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 73dbe39..9d626b5 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>>  obj-$(CONFIG_LTC2485) += ltc2485.o
>> +obj-$(CONFIG_LTC2497) += ltc2497.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>>  obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
>> new file mode 100644
>> index 0000000..0452715
>> --- /dev/null
>> +++ b/drivers/iio/adc/ltc2497.c
>> @@ -0,0 +1,263 @@
>> +/*
>> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC
>> + *
>> + * Copyright (C) 2017 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2.
>> + *
>> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define LTC2497_ENABLE			0xA0
>> +#define LTC2497_SGL			(1 << 4)
>> +#define LTC2497_DIFF			(0 << 4)
>> +#define LTC2497_SIGN			(1 << 3)
>> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
>> +#define LTC2497_CONVERSION_TIME_MS	150ULL
>
> is the ULL really needed?

Needed? - maybe not - but probably not wrong ktime_ms_delta() returns 
64bit and we later do some comparisons...

>
>> +struct ltc2497_st {
>> +	struct i2c_client *client;
>> +	struct regulator *ref;
>> +	ktime_t	time_prev;
>> +	u8 addr_prev;
>> +};
>> +
>> +static int ltc2497_wait_conv(struct ltc2497_st *st)
>> +{
>> +	s64 time_elapsed;
>> +
>> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
>> +
>> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
>> +		/* delay if conversion time not passed
>> +		 * since last read or write
>> +		 */
>> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
>> +		return 0;
>> +	}
>> +
>> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
>> +		/* We're in automatic mode -
>> +		 * so the last reading is stil not outdated
>> +		 */
>> +		return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
>> +{
>> +	struct i2c_client *client = st->client;
>> +	__be32 buf = 0;
>> +	int ret;
>> +
>> +	ret = ltc2497_wait_conv(st);
>> +	if (ret < 0 || st->addr_prev != address) {
>> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
>
> use LTC2497_ENABLE instead of 0xa0

good catch.

>
>> +		if (ret < 0)
>> +			return ret;
>> +		st->addr_prev = address;
>> +		msleep(LTC2497_CONVERSION_TIME_MS);
>> +	}
>> +	ret = i2c_master_recv(client, (char *)&buf, 3);
>> +	if (ret < 0)  {
>> +		dev_err(&client->dev, "i2c_master_recv failed\n");
>> +		return ret;
>> +	}
>> +	st->time_prev = ktime_get();
>> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
>
> what is the purpose of - (1 << 17)? maybe add a comment?
> I'd rather do it with bit operations that arithmetic

Well - this is for offset binary to signed conversion.
I'll add a comment.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct ltc2497_st *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +		ret = ltc2497_read(st, chan->address, val);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		ret = regulator_get_voltage(st->ref);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*val = ret / 1000;
>> +		*val2 = 17;
>> +
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +#define LTC2497_CHAN(_chan, _addr) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.indexed = 1, \
>> +	.channel = (_chan), \
>> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>> +}
>> +
>> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.indexed = 1, \
>> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
>> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
>> +	.address = (_addr | _chan), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>> +	.differential = 1, \
>> +}
>> +
>> +static const struct iio_chan_spec ltc2497_channel[] = {
>> +	LTC2497_CHAN(0, LTC2497_SGL),
>> +	LTC2497_CHAN(1, LTC2497_SGL),
>> +	LTC2497_CHAN(2, LTC2497_SGL),
>> +	LTC2497_CHAN(3, LTC2497_SGL),
>> +	LTC2497_CHAN(4, LTC2497_SGL),
>> +	LTC2497_CHAN(5, LTC2497_SGL),
>> +	LTC2497_CHAN(6, LTC2497_SGL),
>> +	LTC2497_CHAN(7, LTC2497_SGL),
>> +	LTC2497_CHAN(8, LTC2497_SGL),
>> +	LTC2497_CHAN(9, LTC2497_SGL),
>> +	LTC2497_CHAN(10, LTC2497_SGL),
>> +	LTC2497_CHAN(11, LTC2497_SGL),
>> +	LTC2497_CHAN(12, LTC2497_SGL),
>> +	LTC2497_CHAN(13, LTC2497_SGL),
>> +	LTC2497_CHAN(14, LTC2497_SGL),
>> +	LTC2497_CHAN(15, LTC2497_SGL),
>> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
>> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
>> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
>> +};
>> +
>> +static const struct iio_info ltc2497_info = {
>> +	.read_raw = ltc2497_read_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static int ltc2497_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct ltc2497_st *st;
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
>> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
>> +		return -EOPNOTSUPP;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
>> +	st->client = client;
>> +
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->name = id->name;
>> +	indio_dev->info = &ltc2497_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ltc2497_channel;
>> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
>> +
>> +	st->ref = devm_regulator_get(&client->dev, "vref");
>> +	if (IS_ERR(st->ref))
>> +		return PTR_ERR(st->ref);
>> +
>> +	ret = regulator_enable(st->ref);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
>> +	if (ret < 0)
>> +		goto err_regulator_disable;
>> +
>> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
>> +	st->time_prev = ktime_get();
>> +
>> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>
> no devm_ since we have a non-empty _remove()?

ok

>
>> +	if (ret < 0)
>> +		goto err_regulator_disable;
>> +
>> +	return 0;
>> +
>> +err_regulator_disable:
>> +	regulator_disable(st->ref);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltc2497_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct ltc2497_st *st = iio_priv(indio_dev);
>> +
>> +	regulator_disable(st->ref);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id ltc2497_id[] = {
>> +	{ "ltc2497", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
>> +
>> +static const struct of_device_id ltc2497_of_match[] = {
>> +	{ .compatible = "lltc,ltc2497", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
>> +
>> +static struct i2c_driver ltc2497_driver = {
>> +	.driver = {
>> +		.name = "ltc2497",
>> +		.of_match_table = of_match_ptr(ltc2497_of_match),
>> +	},
>> +	.probe = ltc2497_probe,
>> +	.remove = ltc2497_remove,
>> +	.id_table = ltc2497_id,
>> +};
>> +module_i2c_driver(ltc2497_driver);
>> +
>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
>> +MODULE_LICENSE("GPL v2");
>>
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Otl-Aicher Strasse 60-64      80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-29  8:51     ` Michael Hennerich
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Hennerich @ 2017-03-29  8:51 UTC (permalink / raw)
  To: Lars-Peter Clausen, jic23, knaack.h, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, devicetree, linux-kernel

On 23.03.2017 12:05, Lars-Peter Clausen wrote:

Sorry - I missed some of this review feedback ...

>> +
>> +static int ltc2497_wait_conv(struct ltc2497_st *st)
>> +{
>> +	s64 time_elapsed;
>> +
>> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
>> +
>> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
>> +		/* delay if conversion time not passed
>> +		 * since last read or write
>> +		 */
>> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
>
> Considering how long this sleeps msleep_interruptible() might be the better
> choice.

Wondering what should be the outcome of this?
We can't simply replace it. Actually I've seen cases here in drivers/iio 
where the delay is essential, but the return value of 
msleep_interruptible() is not being checked.
Thus causing a malicious access, in case a signal is received.

We must delay here. If we switch to msleep_interruptible() the only 
reason for this would be to cancel the read and return -EINTR to the user.

Also there is another msleep below which would also need this kind of 
handling.

>
>> +		return 0;
>> +	}
>> +
>> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
>> +		/* We're in automatic mode -
>> +		 * so the last reading is stil not outdated
>> +		 */
>> +		return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
>> +{
>> +	struct i2c_client *client = st->client;
>> +	__be32 buf = 0;
>
> transfer buffers must not be on the stack to avoid issues if the controller
> should use DMA.
>
>> +	int ret;
>> +
>> +	ret = ltc2497_wait_conv(st);
>> +	if (ret < 0 || st->addr_prev != address) {
>> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
>> +		if (ret < 0)
>> +			return ret;
>> +		st->addr_prev = address;
>> +		msleep(LTC2497_CONVERSION_TIME_MS);
>> +	}
>> +	ret = i2c_master_recv(client, (char *)&buf, 3);
>> +	if (ret < 0)  {
>> +		dev_err(&client->dev, "i2c_master_recv failed\n");
>> +		return ret;
>> +	}
>> +	st->time_prev = ktime_get();
>> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
>> +
>> +	return ret;
>> +}
> [...]
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Otl-Aicher Strasse 60-64      80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC
@ 2017-03-29  8:51     ` Michael Hennerich
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Hennerich @ 2017-03-29  8:51 UTC (permalink / raw)
  To: Lars-Peter Clausen, jic23-DgEjT+Ai2ygdnm+yROfE0A,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 23.03.2017 12:05, Lars-Peter Clausen wrote:

Sorry - I missed some of this review feedback ...

>> +
>> +static int ltc2497_wait_conv(struct ltc2497_st *st)
>> +{
>> +	s64 time_elapsed;
>> +
>> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
>> +
>> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
>> +		/* delay if conversion time not passed
>> +		 * since last read or write
>> +		 */
>> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
>
> Considering how long this sleeps msleep_interruptible() might be the better
> choice.

Wondering what should be the outcome of this?
We can't simply replace it. Actually I've seen cases here in drivers/iio 
where the delay is essential, but the return value of 
msleep_interruptible() is not being checked.
Thus causing a malicious access, in case a signal is received.

We must delay here. If we switch to msleep_interruptible() the only 
reason for this would be to cancel the read and return -EINTR to the user.

Also there is another msleep below which would also need this kind of 
handling.

>
>> +		return 0;
>> +	}
>> +
>> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
>> +		/* We're in automatic mode -
>> +		 * so the last reading is stil not outdated
>> +		 */
>> +		return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
>> +{
>> +	struct i2c_client *client = st->client;
>> +	__be32 buf = 0;
>
> transfer buffers must not be on the stack to avoid issues if the controller
> should use DMA.
>
>> +	int ret;
>> +
>> +	ret = ltc2497_wait_conv(st);
>> +	if (ret < 0 || st->addr_prev != address) {
>> +		ret = i2c_smbus_write_byte(st->client, 0xA0 | address);
>> +		if (ret < 0)
>> +			return ret;
>> +		st->addr_prev = address;
>> +		msleep(LTC2497_CONVERSION_TIME_MS);
>> +	}
>> +	ret = i2c_master_recv(client, (char *)&buf, 3);
>> +	if (ret < 0)  {
>> +		dev_err(&client->dev, "i2c_master_recv failed\n");
>> +		return ret;
>> +	}
>> +	st->time_prev = ktime_get();
>> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
>> +
>> +	return ret;
>> +}
> [...]
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Otl-Aicher Strasse 60-64      80807 München
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Peter Kolberg, Ali Raza Husain, Eileen Wynne

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-29  8:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-23 10:35 [PATCH] iio/adc/ltc2497: Driver for Linear Technology LTC2497 ADC michael.hennerich
2017-03-23 10:35 ` michael.hennerich
2017-03-23 11:05 ` Lars-Peter Clausen
2017-03-23 11:05   ` Lars-Peter Clausen
2017-03-29  8:51   ` Michael Hennerich
2017-03-29  8:51     ` Michael Hennerich
2017-03-23 11:28 ` Peter Meerwald-Stadler
2017-03-23 11:28   ` Peter Meerwald-Stadler
2017-03-23 14:45   ` Michael Hennerich
2017-03-23 14:45     ` Michael Hennerich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.