From: Michael Hennerich <[email protected]>
Signed-off-by: Michael Hennerich <[email protected]>
---
.../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 = <<c2497_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 = <c2497_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 <[email protected]>");
+MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
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, [email protected] wrote:
> From: Michael Hennerich <[email protected]>
Needs a commit message.
>
> Signed-off-by: Michael Hennerich <[email protected]>
> ---
> .../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 = <<c2497_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;
> +}
[...]
On Thu, 23 Mar 2017, [email protected] wrote:
> From: Michael Hennerich <[email protected]>
comments below
> Signed-off-by: Michael Hennerich <[email protected]>
> ---
> .../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 = <<c2497_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 = <c2497_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 <[email protected]>");
> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
> +MODULE_LICENSE("GPL v2");
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
On 23.03.2017 12:28, Peter Meerwald-Stadler wrote:
> On Thu, 23 Mar 2017, [email protected] wrote:
>
>> From: Michael Hennerich <[email protected]>
>
> comments below
Hi Peter,
Thanks for the review -
comments inline.
>
>> Signed-off-by: Michael Hennerich <[email protected]>
>> ---
>> .../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 = <<c2497_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 = <c2497_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 <[email protected]>");
>> +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
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