[PATCH] iio: Add driver for TLA202x ADCs

[PATCH] iio: Add driver for TLA202x ADCs

[Ibtsam Ul-Haq]

Basic driver for Texas Instruments TLA202x series ADCs. Input
channels are configurable from the device tree. Full Scale Range
selection is also implemented. Trigger buffer support is not yet
implemented.

Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
---
 drivers/staging/iio/adc/Kconfig   |  11 +
 drivers/staging/iio/adc/Makefile  |   1 +
 drivers/staging/iio/adc/tla2024.c | 603 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 615 insertions(+)
 create mode 100644 drivers/staging/iio/adc/tla2024.c

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index e17efb0..580642d 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -80,4 +80,15 @@ config AD7280
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7280a
 
+config TLA2024
+	tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
+	depends on OF
+	depends on I2C
+	help
+	  Say yes here to build support for Texas Instruments TLA2024,
+	  TLA2022 or TLA2021 I2C Analog to Digital Converters.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called tla2024.
+
 endmenu
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index bf18bdd..76a574b5 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7816) += ad7816.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7280) += ad7280a.o
+obj-$(CONFIG_TLA2024) += tla2024.o
diff --git a/drivers/staging/iio/adc/tla2024.c b/drivers/staging/iio/adc/tla2024.c
new file mode 100644
index 0000000..fa2ad34
--- /dev/null
+++ b/drivers/staging/iio/adc/tla2024.c
@@ -0,0 +1,603 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
+ *
+ * Copyright (C) 2018 Koninklijke Philips N.V.
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/iio/iio.h>
+
+#define TLA2024_DATA 0x00
+#define DATA_RES_MASK GENMASK(15, 4)
+#define DATA_RES_SHIFT 4
+
+#define TLA2024_CONF 0x01
+#define CONF_OS_MASK BIT(15)
+#define CONF_OS_SHIFT 15
+#define CONF_MUX_MASK GENMASK(14, 12)
+#define CONF_MUX_SHIFT 12
+#define CONF_PGA_MASK GENMASK(11, 9)
+#define CONF_PGA_SHIFT 9
+#define CONF_MODE_MASK BIT(8)
+#define CONF_MODE_SHIFT 8
+#define CONF_DR_MASK GENMASK(7, 5)
+#define CONF_DR_SHIFT 5
+
+#define TLA2024_CONV_RETRY 10
+
+struct tla202x_model {
+	unsigned int mux_available;
+	unsigned int pga_available;
+};
+
+struct tla2024 {
+	struct i2c_client *i2c;
+	struct tla202x_model *model;
+	struct mutex lock; /* protect i2c transfers */
+	u16 conf_cache;
+	u8 used_mux_channels;
+};
+
+struct tla2024_channel {
+	int ainp;
+	int ainn;
+	char *datasheet_name;
+	unsigned int differential;
+};
+
+static const struct tla2024_channel tla2024_all_channels[] = {
+	{0, 1, "AIN0-AIN1", 1},
+	{0, 3, "AIN0-AIN3", 1},
+	{1, 3, "AIN1-AIN3", 1},
+	{2, 3, "AIN2-AIN3", 1},
+	{0, -1, "AIN0-GND", 0},
+	{1, -1, "AIN1-GND", 0},
+	{2, -1, "AIN2-GND", 0},
+	{3, -1, "AIN3-GND", 0},
+};
+
+static inline int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)
+{
+	u16 i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
+		if ((tla2024_all_channels[i].ainp == ainp_in) &&
+		    (tla2024_all_channels[i].ainn == ainn_in)) {
+			*idx = i;
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+#define TLA202x_MODEL(_mux, _pga)		\
+	{					\
+		.mux_available = (_mux),	\
+		.pga_available = (_pga),	\
+	}
+
+enum tla2024_model_id {
+	TLA2024 = 0,
+	TLA2022 = 1,
+	TLA2021 = 2,
+};
+
+static struct tla202x_model tla202x_models[] = {
+	TLA202x_MODEL(1, 1), // TLA2024
+	TLA202x_MODEL(0, 1), // TLA2022
+	TLA202x_MODEL(0, 0), // TLA2021
+};
+
+static const int tla2024_samp_freq_avail[] = {
+	128, 250, 490, 920, 1600, 2400, 3300 };
+
+static const int tla2024_pga_scale[] = {
+	3000, 2000, 1000, 500, 250, 125 };
+
+static const char * const tla2024_full_scale_range_avail[] = {
+	"6144000", "4096000", "2048000", "1024000", "512000", "256000" };
+
+static const char * const tla2024_conversion_mode_avail[] = {
+				"Continuous",
+				"Single-Shot" };
+
+static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
+		       u16 shift, u16 *val)
+{
+	int ret;
+	u16 data;
+
+	mutex_lock(&priv->lock);
+
+	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
+	if (ret < 0) {
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+
+	data = (u16)ret;
+	if (addr == TLA2024_CONF)
+		priv->conf_cache = data;
+
+	*val = (mask & data) >> shift;
+	mutex_unlock(&priv->lock);
+	return 0;
+}
+
+static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
+		       u16 shift, u16 val)
+{
+	int ret;
+	u16 data;
+
+	mutex_lock(&priv->lock);
+
+	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
+	if (ret < 0) {
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+	data = (u16)ret;
+	data &= ~mask;
+	data |= mask & (val << shift);
+
+	ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
+	if (!ret && addr == TLA2024_CONF)
+		priv->conf_cache = data;
+
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static int tla2024_get_conversion_mode(struct iio_dev *idev,
+				       const struct iio_chan_spec *chan)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	int ret;
+	u16 data;
+
+	ret = tla2024_get(priv, TLA2024_CONF, CONF_MODE_MASK,
+			  CONF_MODE_SHIFT, &data);
+	if (ret < 0)
+		return ret;
+
+	return data;
+}
+
+static int tla2024_set_conversion_mode(struct iio_dev *idev,
+				       const struct iio_chan_spec *chan,
+				       unsigned int data)
+{
+	struct tla2024 *priv = iio_priv(idev);
+
+	return tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
+			   CONF_MODE_SHIFT, data);
+}
+
+static int tla2024_get_full_scale_range(struct iio_dev *idev,
+					const struct iio_chan_spec *chan)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	int ret;
+	u16 data;
+
+	ret = tla2024_get(priv, TLA2024_CONF, CONF_PGA_MASK,
+			  CONF_PGA_SHIFT, &data);
+	if (ret < 0)
+		return ret;
+
+	return data;
+}
+
+static int tla2024_set_full_scale_range(struct iio_dev *idev,
+					const struct iio_chan_spec *chan,
+					unsigned int data)
+{
+	struct tla2024 *priv = iio_priv(idev);
+
+	return tla2024_set(priv, TLA2024_CONF, CONF_PGA_MASK,
+			   CONF_PGA_SHIFT, data);
+}
+
+static const struct iio_enum tla2024_conversion_mode = {
+	.items = tla2024_conversion_mode_avail,
+	.num_items = ARRAY_SIZE(tla2024_conversion_mode_avail),
+	.get = tla2024_get_conversion_mode,
+	.set = tla2024_set_conversion_mode,
+};
+
+static const struct iio_enum tla2024_full_scale_range = {
+	.items = tla2024_full_scale_range_avail,
+	.num_items = ARRAY_SIZE(tla2024_full_scale_range_avail),
+	.get = tla2024_get_full_scale_range,
+	.set = tla2024_set_full_scale_range,
+};
+
+#define TLA2024_IIO_ENUM_AVAIL(_name, _shared, _e)			\
+{									\
+	.name = (_name "_available"),					\
+	.shared = (_shared),						\
+	.read = iio_enum_available_read,				\
+	.private = (uintptr_t)(_e),			\
+}
+
+static const struct iio_chan_spec_ext_info tla202x_ext_info_pga[] = {
+	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
+		 &tla2024_conversion_mode),
+	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
+			       &tla2024_conversion_mode),
+	IIO_ENUM("full_scale_range", IIO_SHARED_BY_ALL,
+		 &tla2024_full_scale_range),
+	TLA2024_IIO_ENUM_AVAIL("full_scale_range", IIO_SHARED_BY_ALL,
+			       &tla2024_full_scale_range),
+	{},
+};
+
+static const struct iio_chan_spec_ext_info tla202x_ext_info[] = {
+	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
+		 &tla2024_conversion_mode),
+	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
+			       &tla2024_conversion_mode),
+	{},
+};
+
+static int tla2024_read_avail(struct iio_dev *idev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+
+		*length = ARRAY_SIZE(tla2024_samp_freq_avail);
+		*vals = tla2024_samp_freq_avail;
+
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
+static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
+{
+	u16 chan_idx = 0;
+	u32 tmp_p, tmp_n;
+	int ainp, ainn;
+	int ret;
+
+	ret = of_property_read_u32_index(ch, "single-ended", 0, &tmp_p);
+	if (ret) {
+		ret = of_property_read_u32_index(ch,
+						 "differential", 0, &tmp_p);
+		if (ret)
+			return ret;
+
+		ret = of_property_read_u32_index(ch,
+						 "differential", 1, &tmp_n);
+		if (ret)
+			return ret;
+
+		ainp = (int)tmp_p;
+		ainn = (int)tmp_n;
+	} else {
+		ainp = (int)tmp_p;
+		ainn = -1;
+	}
+
+	ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
+	if (ret < 0)
+		return ret;
+
+	// if model doesn"t have mux then only channel 0 is allowed
+	if (!priv->model->mux_available && chan_idx)
+		return -EINVAL;
+
+	// if already used
+	if ((priv->used_mux_channels) & (1 << chan_idx))
+		return -EINVAL;
+
+	return chan_idx;
+}
+
+static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
+			     struct iio_chan_spec *chan)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	u16 chan_idx = 0;
+	int ret;
+
+	ret = tla2024_of_find_chan(priv, node);
+	if (ret < 0)
+		return ret;
+
+	chan_idx = (u16)ret;
+	priv->used_mux_channels |= (1 << chan_idx);
+	chan->type = IIO_VOLTAGE;
+	chan->channel = tla2024_all_channels[chan_idx].ainp;
+	chan->channel2 = tla2024_all_channels[chan_idx].ainn;
+	chan->differential = tla2024_all_channels[chan_idx].differential;
+	chan->extend_name = node->name;
+	chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
+	chan->indexed = 1;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+					BIT(IIO_CHAN_INFO_PROCESSED);
+	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	chan->info_mask_shared_by_all_available =
+					BIT(IIO_CHAN_INFO_SAMP_FREQ);
+
+	if (priv->model->pga_available)
+		chan->ext_info = tla202x_ext_info_pga;
+	else
+		chan->ext_info = tla202x_ext_info;
+
+	return 0;
+}
+
+static int tla2024_wait(struct tla2024 *priv)
+{
+	int ret = 0;
+	u16 retry = TLA2024_CONV_RETRY;
+	u16 status;
+
+	do {
+		if (!--retry)
+			return -EIO;
+		ret = tla2024_get(priv, TLA2024_CONF, CONF_OS_MASK,
+				  CONF_OS_SHIFT, &status);
+		if (ret < 0)
+			return ret;
+		if (!status)
+			usleep_range(25, 1000);
+	} while (!status);
+
+	return ret;
+}
+
+static int tla2024_select_channel(struct tla2024 *priv,
+				  struct iio_chan_spec const *chan)
+{
+	int ret = 0;
+	int ainp = chan->channel;
+	int ainn = chan->channel2;
+	u16 tmp, chan_id = 0;
+
+	tla2024_find_chan_idx(ainp, ainn, &chan_id);
+
+	tmp = (priv->conf_cache & CONF_MUX_MASK) >> CONF_MUX_SHIFT;
+	if (tmp != chan_id)
+		ret = tla2024_set(priv, TLA2024_CONF, CONF_MUX_MASK,
+				  CONF_MUX_SHIFT, chan_id);
+	return ret;
+}
+
+static int tla2024_convert(struct tla2024 *priv)
+{
+	int ret = 0;
+
+	if (priv->conf_cache & CONF_MODE_MASK) {
+		ret = tla2024_set(priv, TLA2024_CONF, CONF_OS_MASK,
+				  CONF_OS_SHIFT, 1);
+		if (ret < 0)
+			return ret;
+		ret = tla2024_wait(priv);
+	}
+	return ret;
+}
+
+static int tla2024_read_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	int ret;
+	u16 data, idx;
+	s16 tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = tla2024_select_channel(priv, channel);
+		if (ret < 0)
+			return ret;
+
+		ret = tla2024_convert(priv);
+		if (ret < 0)
+			return ret;
+
+		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
+				  DATA_RES_SHIFT, &data);
+		if (ret < 0)
+			return ret;
+
+		*val = data;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = tla2024_select_channel(priv, channel);
+		if (ret < 0)
+			return ret;
+
+		ret = tla2024_convert(priv);
+		if (ret < 0)
+			return ret;
+
+		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
+				  DATA_RES_SHIFT, &data);
+		if (ret < 0)
+			return ret;
+
+		tmp = (s16)(data << DATA_RES_SHIFT);
+		idx = (priv->conf_cache & CONF_PGA_MASK) >> CONF_PGA_SHIFT;
+		*val = (tmp >> DATA_RES_SHIFT) * tla2024_pga_scale[idx];
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+
+		ret = tla2024_get(priv, TLA2024_CONF, CONF_DR_MASK,
+				  CONF_DR_SHIFT, &data);
+		if (ret < 0)
+			return ret;
+
+		*val = tla2024_samp_freq_avail[data];
+		return IIO_VAL_INT;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int tla2024_write_raw(struct iio_dev *idev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	u16 i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+
+		for (i = 0; i < ARRAY_SIZE(tla2024_samp_freq_avail); i++) {
+			if (tla2024_samp_freq_avail[i] == val)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(tla2024_samp_freq_avail))
+			return -EINVAL;
+
+		return tla2024_set(priv, TLA2024_CONF, CONF_DR_MASK,
+					CONF_DR_SHIFT, i);
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int tla2024_of_chan_init(struct iio_dev *idev)
+{
+	struct device_node *node = idev->dev.of_node;
+	struct device_node *child;
+	struct iio_chan_spec *channels;
+	int ret, i = 0, num_channels = 0;
+
+	num_channels = of_get_available_child_count(node);
+	if (!num_channels) {
+		dev_err(&idev->dev, "no channels configured\n");
+		return -ENODEV;
+	}
+
+	channels = devm_kcalloc(&idev->dev, num_channels,
+				sizeof(struct iio_chan_spec), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_available_child_of_node(node, child) {
+		ret = tla2024_init_chan(idev, child, &channels[i]);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+		i++;
+	}
+
+	idev->channels = channels;
+	idev->num_channels = num_channels;
+
+	return 0;
+}
+
+static const struct iio_info tla2024_info = {
+	.read_raw = tla2024_read_raw,
+	.write_raw = tla2024_write_raw,
+	.read_avail = tla2024_read_avail,
+};
+
+static int tla2024_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct iio_dev *iio;
+	struct tla2024 *priv;
+	struct tla202x_model *model;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	model = &tla202x_models[id->driver_data];
+
+	iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
+	if (!iio)
+		return -ENOMEM;
+
+	priv = iio_priv(iio);
+	priv->i2c = client;
+	priv->model = model;
+	mutex_init(&priv->lock);
+
+	iio->dev.parent = &client->dev;
+	iio->dev.of_node = client->dev.of_node;
+	iio->name = client->name;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->info = &tla2024_info;
+
+	ret = tla2024_of_chan_init(iio);
+	if (ret < 0)
+		return ret;
+
+	ret = tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
+			  CONF_MODE_SHIFT, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_device_register(iio);
+
+	return ret;
+}
+
+static int tla2024_remove(struct i2c_client *client)
+{
+	struct iio_dev *iio = i2c_get_clientdata(client);
+
+	iio_device_unregister(iio);
+	return 0;
+}
+
+static const struct i2c_device_id tla2024_id[] = {
+	{ "tla2024", TLA2024 },
+	{ "tla2022", TLA2022 },
+	{ "tla2021", TLA2021 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tla2024_id);
+
+static const struct of_device_id tla2024_of_match[] = {
+	{ .compatible = "ti,tla2024" },
+	{ .compatible = "ti,tla2022" },
+	{ .compatible = "ti,tla2021" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tla2024_of_match);
+
+static struct i2c_driver tla2024_driver = {
+	.driver = {
+		.name = "tla2024",
+		.of_match_table = of_match_ptr(tla2024_of_match),
+	},
+	.probe = tla2024_probe,
+	.remove = tla2024_remove,
+	.id_table = tla2024_id,
+};
+module_i2c_driver(tla2024_driver);
+
+MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
+MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH] iio: Add driver for TLA202x ADCs

[Peter Meerwald-Stadler]

Hello,

> Basic driver for Texas Instruments TLA202x series ADCs. Input
> channels are configurable from the device tree. Full Scale Range
> selection is also implemented. Trigger buffer support is not yet
> implemented.

some comments below; don't add to staging but to drivers/iio directly 
(staging is old stuff that has not yet been cleaned up)

regards, p.

> Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
> ---
>  drivers/staging/iio/adc/Kconfig   |  11 +
>  drivers/staging/iio/adc/Makefile  |   1 +
>  drivers/staging/iio/adc/tla2024.c | 603 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 615 insertions(+)
>  create mode 100644 drivers/staging/iio/adc/tla2024.c
> 
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index e17efb0..580642d 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -80,4 +80,15 @@ config AD7280
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7280a
>  
> +config TLA2024

please keep the entries in the file sorted

> +	tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
> +	depends on OF
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Texas Instruments TLA2024,
> +	  TLA2022 or TLA2021 I2C Analog to Digital Converters.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called tla2024.
> +
>  endmenu
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index bf18bdd..76a574b5 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7816) += ad7816.o
>  obj-$(CONFIG_AD7192) += ad7192.o
>  obj-$(CONFIG_AD7280) += ad7280a.o
> +obj-$(CONFIG_TLA2024) += tla2024.o

same here, alphabetic order

> diff --git a/drivers/staging/iio/adc/tla2024.c b/drivers/staging/iio/adc/tla2024.c
> new file mode 100644
> index 0000000..fa2ad34
> --- /dev/null
> +++ b/drivers/staging/iio/adc/tla2024.c
> @@ -0,0 +1,603 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
> + *
> + * Copyright (C) 2018 Koninklijke Philips N.V.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/iio/iio.h>
> +
> +#define TLA2024_DATA 0x00
> +#define DATA_RES_MASK GENMASK(15, 4)

we want consistently prefixed #defines, so TLA2024_DATA_RES_MASK

> +#define DATA_RES_SHIFT 4
> +
> +#define TLA2024_CONF 0x01
> +#define CONF_OS_MASK BIT(15)
> +#define CONF_OS_SHIFT 15
> +#define CONF_MUX_MASK GENMASK(14, 12)
> +#define CONF_MUX_SHIFT 12
> +#define CONF_PGA_MASK GENMASK(11, 9)
> +#define CONF_PGA_SHIFT 9
> +#define CONF_MODE_MASK BIT(8)
> +#define CONF_MODE_SHIFT 8
> +#define CONF_DR_MASK GENMASK(7, 5)
> +#define CONF_DR_SHIFT 5
> +
> +#define TLA2024_CONV_RETRY 10
> +
> +struct tla202x_model {
> +	unsigned int mux_available;
> +	unsigned int pga_available;
> +};
> +
> +struct tla2024 {
> +	struct i2c_client *i2c;
> +	struct tla202x_model *model;
> +	struct mutex lock; /* protect i2c transfers */
> +	u16 conf_cache;
> +	u8 used_mux_channels;
> +};
> +
> +struct tla2024_channel {
> +	int ainp;
> +	int ainn;
> +	char *datasheet_name;

const?

> +	unsigned int differential;

bool?

> +};
> +
> +static const struct tla2024_channel tla2024_all_channels[] = {
> +	{0, 1, "AIN0-AIN1", 1},
> +	{0, 3, "AIN0-AIN3", 1},
> +	{1, 3, "AIN1-AIN3", 1},
> +	{2, 3, "AIN2-AIN3", 1},
> +	{0, -1, "AIN0-GND", 0},
> +	{1, -1, "AIN1-GND", 0},
> +	{2, -1, "AIN2-GND", 0},
> +	{3, -1, "AIN3-GND", 0},
> +};
> +
> +static inline int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)

don't use inline, let the compiler figure out what to inline
why use u16 for the index/counter? just use int (or unsigned int)

> +{
> +	u16 i = 0;

initialization not needed

> +
> +	for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
> +		if ((tla2024_all_channels[i].ainp == ainp_in) &&
> +		    (tla2024_all_channels[i].ainn == ainn_in)) {
> +			*idx = i;
> +			return 0;
> +		}
> +	}

maybe a newline here

> +	return -EINVAL;
> +}
> +
> +#define TLA202x_MODEL(_mux, _pga)		\
> +	{					\
> +		.mux_available = (_mux),	\
> +		.pga_available = (_pga),	\
> +	}
> +
> +enum tla2024_model_id {
> +	TLA2024 = 0,
> +	TLA2022 = 1,
> +	TLA2021 = 2,
> +};
> +
> +static struct tla202x_model tla202x_models[] = {
> +	TLA202x_MODEL(1, 1), // TLA2024
> +	TLA202x_MODEL(0, 1), // TLA2022
> +	TLA202x_MODEL(0, 0), // TLA2021
> +};
> +
> +static const int tla2024_samp_freq_avail[] = {

maybe annotate what unit

> +	128, 250, 490, 920, 1600, 2400, 3300 };
> +
> +static const int tla2024_pga_scale[] = {
> +	3000, 2000, 1000, 500, 250, 125 };
> +
> +static const char * const tla2024_full_scale_range_avail[] = {
> +	"6144000", "4096000", "2048000", "1024000", "512000", "256000" };
> +
> +static const char * const tla2024_conversion_mode_avail[] = {
> +				"Continuous",
> +				"Single-Shot" };
> +
> +static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
> +		       u16 shift, u16 *val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	mutex_lock(&priv->lock);

the mutex seems to protect the conf_cache rather than the i2c transfer

> +
> +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
> +
> +	data = (u16)ret;

cast not needed

> +	if (addr == TLA2024_CONF)
> +		priv->conf_cache = data;
> +
> +	*val = (mask & data) >> shift;
> +	mutex_unlock(&priv->lock);

maybe newline here

> +	return 0;
> +}
> +
> +static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
> +		       u16 shift, u16 val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);

couldn't conf_cache be used here?

> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
> +	data = (u16)ret;

cast not needed

> +	data &= ~mask;
> +	data |= mask & (val << shift);
> +
> +	ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
> +	if (!ret && addr == TLA2024_CONF)
> +		priv->conf_cache = data;
> +
> +	mutex_unlock(&priv->lock);

maybe newline here

> +	return ret;
> +}
> +

not sure if the following helper functions are worth the effort;
parameter chan is not used

> +static int tla2024_get_conversion_mode(struct iio_dev *idev,
> +				       const struct iio_chan_spec *chan)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	int ret;
> +	u16 data;
> +
> +	ret = tla2024_get(priv, TLA2024_CONF, CONF_MODE_MASK,
> +			  CONF_MODE_SHIFT, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	return data;
> +}
> +
> +static int tla2024_set_conversion_mode(struct iio_dev *idev,
> +				       const struct iio_chan_spec *chan,
> +				       unsigned int data)

data should be u16?

> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +
> +	return tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
> +			   CONF_MODE_SHIFT, data);
> +}
> +
> +static int tla2024_get_full_scale_range(struct iio_dev *idev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	int ret;
> +	u16 data;
> +
> +	ret = tla2024_get(priv, TLA2024_CONF, CONF_PGA_MASK,
> +			  CONF_PGA_SHIFT, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	return data;
> +}
> +
> +static int tla2024_set_full_scale_range(struct iio_dev *idev,
> +					const struct iio_chan_spec *chan,
> +					unsigned int data)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +
> +	return tla2024_set(priv, TLA2024_CONF, CONF_PGA_MASK,
> +			   CONF_PGA_SHIFT, data);
> +}
> +
> +static const struct iio_enum tla2024_conversion_mode = {
> +	.items = tla2024_conversion_mode_avail,
> +	.num_items = ARRAY_SIZE(tla2024_conversion_mode_avail),
> +	.get = tla2024_get_conversion_mode,
> +	.set = tla2024_set_conversion_mode,
> +};
> +
> +static const struct iio_enum tla2024_full_scale_range = {
> +	.items = tla2024_full_scale_range_avail,
> +	.num_items = ARRAY_SIZE(tla2024_full_scale_range_avail),
> +	.get = tla2024_get_full_scale_range,
> +	.set = tla2024_set_full_scale_range,
> +};
> +
> +#define TLA2024_IIO_ENUM_AVAIL(_name, _shared, _e)			\
> +{									\
> +	.name = (_name "_available"),					\
> +	.shared = (_shared),						\
> +	.read = iio_enum_available_read,				\
> +	.private = (uintptr_t)(_e),			\
> +}
> +
> +static const struct iio_chan_spec_ext_info tla202x_ext_info_pga[] = {
> +	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
> +		 &tla2024_conversion_mode),
> +	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
> +			       &tla2024_conversion_mode),
> +	IIO_ENUM("full_scale_range", IIO_SHARED_BY_ALL,
> +		 &tla2024_full_scale_range),
> +	TLA2024_IIO_ENUM_AVAIL("full_scale_range", IIO_SHARED_BY_ALL,
> +			       &tla2024_full_scale_range),
> +	{},
> +};
> +
> +static const struct iio_chan_spec_ext_info tla202x_ext_info[] = {
> +	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
> +		 &tla2024_conversion_mode),
> +	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
> +			       &tla2024_conversion_mode),
> +	{},
> +};
> +
> +static int tla2024_read_avail(struct iio_dev *idev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +
> +		*length = ARRAY_SIZE(tla2024_samp_freq_avail);
> +		*vals = tla2024_samp_freq_avail;
> +
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> +{
> +	u16 chan_idx = 0;
> +	u32 tmp_p, tmp_n;
> +	int ainp, ainn;
> +	int ret;
> +
> +	ret = of_property_read_u32_index(ch, "single-ended", 0, &tmp_p);
> +	if (ret) {
> +		ret = of_property_read_u32_index(ch,
> +						 "differential", 0, &tmp_p);
> +		if (ret)
> +			return ret;
> +
> +		ret = of_property_read_u32_index(ch,
> +						 "differential", 1, &tmp_n);
> +		if (ret)
> +			return ret;
> +
> +		ainp = (int)tmp_p;
> +		ainn = (int)tmp_n;
> +	} else {
> +		ainp = (int)tmp_p;
> +		ainn = -1;
> +	}
> +
> +	ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
> +	if (ret < 0)
> +		return ret;
> +
> +	// if model doesn"t have mux then only channel 0 is allowed

are C++ style comments permitted these days?

> +	if (!priv->model->mux_available && chan_idx)
> +		return -EINVAL;
> +
> +	// if already used
> +	if ((priv->used_mux_channels) & (1 << chan_idx))
> +		return -EINVAL;
> +
> +	return chan_idx;
> +}
> +
> +static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
> +			     struct iio_chan_spec *chan)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	u16 chan_idx = 0;

initialization not needed

> +	int ret;
> +
> +	ret = tla2024_of_find_chan(priv, node);
> +	if (ret < 0)
> +		return ret;
> +
> +	chan_idx = (u16)ret;

cast not needed

> +	priv->used_mux_channels |= (1 << chan_idx);

could use BIT() macro

> +	chan->type = IIO_VOLTAGE;
> +	chan->channel = tla2024_all_channels[chan_idx].ainp;
> +	chan->channel2 = tla2024_all_channels[chan_idx].ainn;
> +	chan->differential = tla2024_all_channels[chan_idx].differential;
> +	chan->extend_name = node->name;
> +	chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
> +	chan->indexed = 1;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					BIT(IIO_CHAN_INFO_PROCESSED);
> +	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	chan->info_mask_shared_by_all_available =
> +					BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +
> +	if (priv->model->pga_available)
> +		chan->ext_info = tla202x_ext_info_pga;
> +	else
> +		chan->ext_info = tla202x_ext_info;
> +
> +	return 0;
> +}
> +
> +static int tla2024_wait(struct tla2024 *priv)
> +{
> +	int ret = 0;

initialization not needed

> +	u16 retry = TLA2024_CONV_RETRY;

retry is just a counter, should be int or unsigned int

> +	u16 status;
> +
> +	do {
> +		if (!--retry)
> +			return -EIO;
> +		ret = tla2024_get(priv, TLA2024_CONF, CONF_OS_MASK,
> +				  CONF_OS_SHIFT, &status);
> +		if (ret < 0)
> +			return ret;
> +		if (!status)
> +			usleep_range(25, 1000);
> +	} while (!status);
> +
> +	return ret;
> +}
> +
> +static int tla2024_select_channel(struct tla2024 *priv,
> +				  struct iio_chan_spec const *chan)
> +{
> +	int ret = 0;
> +	int ainp = chan->channel;
> +	int ainn = chan->channel2;
> +	u16 tmp, chan_id = 0;
> +
> +	tla2024_find_chan_idx(ainp, ainn, &chan_id);
> +
> +	tmp = (priv->conf_cache & CONF_MUX_MASK) >> CONF_MUX_SHIFT;
> +	if (tmp != chan_id)
> +		ret = tla2024_set(priv, TLA2024_CONF, CONF_MUX_MASK,
> +				  CONF_MUX_SHIFT, chan_id);
> +	return ret;
> +}
> +
> +static int tla2024_convert(struct tla2024 *priv)
> +{
> +	int ret = 0;
> +
> +	if (priv->conf_cache & CONF_MODE_MASK) {
> +		ret = tla2024_set(priv, TLA2024_CONF, CONF_OS_MASK,
> +				  CONF_OS_SHIFT, 1);
> +		if (ret < 0)
> +			return ret;
> +		ret = tla2024_wait(priv);
> +	}
> +	return ret;
> +}
> +
> +static int tla2024_read_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	int ret;
> +	u16 data, idx;
> +	s16 tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

I think there should be a mutex here to protect the selection of a channel 
and get actualy reading of data

> +		ret = tla2024_select_channel(priv, channel);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tla2024_convert(priv);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
> +				  DATA_RES_SHIFT, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = data;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = tla2024_select_channel(priv, channel);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tla2024_convert(priv);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
> +				  DATA_RES_SHIFT, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		tmp = (s16)(data << DATA_RES_SHIFT);
> +		idx = (priv->conf_cache & CONF_PGA_MASK) >> CONF_PGA_SHIFT;
> +		*val = (tmp >> DATA_RES_SHIFT) * tla2024_pga_scale[idx];
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +
> +		ret = tla2024_get(priv, TLA2024_CONF, CONF_DR_MASK,
> +				  CONF_DR_SHIFT, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = tla2024_samp_freq_avail[data];
> +		return IIO_VAL_INT;
> +
> +	default:
> +		break;

just
return -EINVAL
here

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int tla2024_write_raw(struct iio_dev *idev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	u16 i;

use int

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +
> +		for (i = 0; i < ARRAY_SIZE(tla2024_samp_freq_avail); i++) {
> +			if (tla2024_samp_freq_avail[i] == val)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(tla2024_samp_freq_avail))
> +			return -EINVAL;
> +
> +		return tla2024_set(priv, TLA2024_CONF, CONF_DR_MASK,
> +					CONF_DR_SHIFT, i);
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int tla2024_of_chan_init(struct iio_dev *idev)
> +{
> +	struct device_node *node = idev->dev.of_node;
> +	struct device_node *child;
> +	struct iio_chan_spec *channels;
> +	int ret, i = 0, num_channels = 0;

initialization not needed

> +
> +	num_channels = of_get_available_child_count(node);
> +	if (!num_channels) {
> +		dev_err(&idev->dev, "no channels configured\n");
> +		return -ENODEV;
> +	}
> +
> +	channels = devm_kcalloc(&idev->dev, num_channels,
> +				sizeof(struct iio_chan_spec), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	for_each_available_child_of_node(node, child) {
> +		ret = tla2024_init_chan(idev, child, &channels[i]);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +		i++;
> +	}
> +
> +	idev->channels = channels;
> +	idev->num_channels = num_channels;
> +
> +	return 0;
> +}
> +
> +static const struct iio_info tla2024_info = {
> +	.read_raw = tla2024_read_raw,
> +	.write_raw = tla2024_write_raw,
> +	.read_avail = tla2024_read_avail,
> +};
> +
> +static int tla2024_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *iio;
> +	struct tla2024 *priv;
> +	struct tla202x_model *model;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EOPNOTSUPP;
> +
> +	model = &tla202x_models[id->driver_data];
> +
> +	iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(iio);
> +	priv->i2c = client;
> +	priv->model = model;
> +	mutex_init(&priv->lock);
> +
> +	iio->dev.parent = &client->dev;
> +	iio->dev.of_node = client->dev.of_node;
> +	iio->name = client->name;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &tla2024_info;
> +
> +	ret = tla2024_of_chan_init(iio);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
> +			  CONF_MODE_SHIFT, 1);

so what is the default mode? maybe use MAGIC for 1?

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(iio);

could use devm_ variant? to be able to drop _remove()
and return directly

> +
> +	return ret;
> +}
> +
> +static int tla2024_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *iio = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(iio);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tla2024_id[] = {
> +	{ "tla2024", TLA2024 },
> +	{ "tla2022", TLA2022 },
> +	{ "tla2021", TLA2021 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tla2024_id);
> +
> +static const struct of_device_id tla2024_of_match[] = {
> +	{ .compatible = "ti,tla2024" },
> +	{ .compatible = "ti,tla2022" },
> +	{ .compatible = "ti,tla2021" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tla2024_of_match);
> +
> +static struct i2c_driver tla2024_driver = {
> +	.driver = {
> +		.name = "tla2024",
> +		.of_match_table = of_match_ptr(tla2024_of_match),
> +	},
> +	.probe = tla2024_probe,
> +	.remove = tla2024_remove,
> +	.id_table = tla2024_id,
> +};
> +module_i2c_driver(tla2024_driver);
> +
> +MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
> +MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

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

Re: [PATCH] iio: Add driver for TLA202x ADCs

[Jonathan Cameron]

On Tue, 20 Nov 2018 07:11:55 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> Hello,
> 
> > Basic driver for Texas Instruments TLA202x series ADCs. Input
> > channels are configurable from the device tree. Full Scale Range
> > selection is also implemented. Trigger buffer support is not yet
> > implemented.  
> 
> some comments below; don't add to staging but to drivers/iio directly 
> (staging is old stuff that has not yet been cleaned up)

Unless of course, you aren't planning on doing the cleanup this still
needs?  I which case it would be fine to submit to staging but the
description should be very clear on that!

> 
> regards, p.

A few more comments from me.

Thanks,

Jonathan
> 
> > Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
> > ---
> >  drivers/staging/iio/adc/Kconfig   |  11 +
> >  drivers/staging/iio/adc/Makefile  |   1 +
> >  drivers/staging/iio/adc/tla2024.c | 603 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 615 insertions(+)
> >  create mode 100644 drivers/staging/iio/adc/tla2024.c
> > 
> > diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> > index e17efb0..580642d 100644
> > --- a/drivers/staging/iio/adc/Kconfig
> > +++ b/drivers/staging/iio/adc/Kconfig
> > @@ -80,4 +80,15 @@ config AD7280
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ad7280a
> >  
> > +config TLA2024  
> 
> please keep the entries in the file sorted
> 
> > +	tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
> > +	depends on OF
> > +	depends on I2C
> > +	help
> > +	  Say yes here to build support for Texas Instruments TLA2024,
> > +	  TLA2022 or TLA2021 I2C Analog to Digital Converters.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called tla2024.
> > +
> >  endmenu
> > diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> > index bf18bdd..76a574b5 100644
> > --- a/drivers/staging/iio/adc/Makefile
> > +++ b/drivers/staging/iio/adc/Makefile
> > @@ -10,3 +10,4 @@ obj-$(CONFIG_AD7780) += ad7780.o
> >  obj-$(CONFIG_AD7816) += ad7816.o
> >  obj-$(CONFIG_AD7192) += ad7192.o
> >  obj-$(CONFIG_AD7280) += ad7280a.o
> > +obj-$(CONFIG_TLA2024) += tla2024.o  
> 
> same here, alphabetic order
> 
> > diff --git a/drivers/staging/iio/adc/tla2024.c b/drivers/staging/iio/adc/tla2024.c
> > new file mode 100644
> > index 0000000..fa2ad34
> > --- /dev/null
> > +++ b/drivers/staging/iio/adc/tla2024.c
> > @@ -0,0 +1,603 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
> > + *
> > + * Copyright (C) 2018 Koninklijke Philips N.V.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/of.h>
> > +#include <linux/iio/iio.h>
When not reason to do otherwise, alphabetical order preferred for
includes.

> > +
> > +#define TLA2024_DATA 0x00
> > +#define DATA_RES_MASK GENMASK(15, 4)  
> 
> we want consistently prefixed #defines, so TLA2024_DATA_RES_MASK
> 
> > +#define DATA_RES_SHIFT 4
> > +
> > +#define TLA2024_CONF 0x01
> > +#define CONF_OS_MASK BIT(15)
> > +#define CONF_OS_SHIFT 15
> > +#define CONF_MUX_MASK GENMASK(14, 12)
> > +#define CONF_MUX_SHIFT 12
> > +#define CONF_PGA_MASK GENMASK(11, 9)
> > +#define CONF_PGA_SHIFT 9
> > +#define CONF_MODE_MASK BIT(8)
> > +#define CONF_MODE_SHIFT 8
> > +#define CONF_DR_MASK GENMASK(7, 5)
> > +#define CONF_DR_SHIFT 5
> > +
> > +#define TLA2024_CONV_RETRY 10
> > +
> > +struct tla202x_model {
> > +	unsigned int mux_available;
> > +	unsigned int pga_available;
> > +};
> > +
> > +struct tla2024 {
> > +	struct i2c_client *i2c;
> > +	struct tla202x_model *model;
> > +	struct mutex lock; /* protect i2c transfers */

That comment could do to be more specific.  The actual
transfers don't need protecting as that is done inside
the i2c subsystem. Ah I see Peter mentioned this
below.

> > +	u16 conf_cache;
> > +	u8 used_mux_channels;
> > +};
> > +
> > +struct tla2024_channel {
> > +	int ainp;
> > +	int ainn;
> > +	char *datasheet_name;  
> 
> const?
> 
> > +	unsigned int differential;  
> 
> bool?
> 
> > +};
> > +
> > +static const struct tla2024_channel tla2024_all_channels[] = {
> > +	{0, 1, "AIN0-AIN1", 1},
> > +	{0, 3, "AIN0-AIN3", 1},
> > +	{1, 3, "AIN1-AIN3", 1},
> > +	{2, 3, "AIN2-AIN3", 1},
> > +	{0, -1, "AIN0-GND", 0},

I guess if it's what it says on the data sheet it is fine, but for
single ended use, "AIN0" seems sufficient description to me.

> > +	{1, -1, "AIN1-GND", 0},
> > +	{2, -1, "AIN2-GND", 0},
> > +	{3, -1, "AIN3-GND", 0},
> > +};
> > +
> > +static inline int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)  
> 
> don't use inline, let the compiler figure out what to inline
> why use u16 for the index/counter? just use int (or unsigned int)
> 
> > +{
> > +	u16 i = 0;  
> 
> initialization not needed
> 
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
> > +		if ((tla2024_all_channels[i].ainp == ainp_in) &&
> > +		    (tla2024_all_channels[i].ainn == ainn_in)) {
> > +			*idx = i;
> > +			return 0;
> > +		}
> > +	}  
> 
> maybe a newline here
> 
> > +	return -EINVAL;
> > +}
> > +
> > +#define TLA202x_MODEL(_mux, _pga)		\
> > +	{					\
> > +		.mux_available = (_mux),	\
> > +		.pga_available = (_pga),	\
> > +	}
> > +
> > +enum tla2024_model_id {
> > +	TLA2024 = 0,
> > +	TLA2022 = 1,
> > +	TLA2021 = 2,
> > +};
> > +
> > +static struct tla202x_model tla202x_models[] = {
> > +	TLA202x_MODEL(1, 1), // TLA2024
> > +	TLA202x_MODEL(0, 1), // TLA2022
> > +	TLA202x_MODEL(0, 0), // TLA2021
> > +};
> > +
> > +static const int tla2024_samp_freq_avail[] = {  
> 
> maybe annotate what unit
> 
> > +	128, 250, 490, 920, 1600, 2400, 3300 };
> > +
> > +static const int tla2024_pga_scale[] = {
> > +	3000, 2000, 1000, 500, 250, 125 };
> > +
> > +static const char * const tla2024_full_scale_range_avail[] = {
> > +	"6144000", "4096000", "2048000", "1024000", "512000", "256000" };
> > +
> > +static const char * const tla2024_conversion_mode_avail[] = {
> > +				"Continuous",
> > +				"Single-Shot" };

This is one that gets proposed a few times a year and the reality is that
it is not much use to generic userspace which doesn't understand these
terms. So it shouldn't be given control of them.

Usual mapping is, oneshot corresponds to sysfs read, and once you
start using the buffer interface (don't think you have that here
yet) then you can move to the continuous mode.

There can be finer grained steps using info like the expected sampling
frequency but normally that's a later step when someone needs
that level of control.

> > +
> > +static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
> > +		       u16 shift, u16 *val)
> > +{
> > +	int ret;
> > +	u16 data;
> > +
> > +	mutex_lock(&priv->lock);  
> 
> the mutex seems to protect the conf_cache rather than the i2c transfer
> 
> > +
> > +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> > +	if (ret < 0) {
> > +		mutex_unlock(&priv->lock);
> > +		return ret;
> > +	}
> > +
> > +	data = (u16)ret;  
> 
> cast not needed
> 
> > +	if (addr == TLA2024_CONF)
> > +		priv->conf_cache = data;
> > +
> > +	*val = (mask & data) >> shift;
> > +	mutex_unlock(&priv->lock);  
> 
> maybe newline here
> 
> > +	return 0;
> > +}
> > +
> > +static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
> > +		       u16 shift, u16 val)
> > +{
> > +	int ret;
> > +	u16 data;
> > +
> > +	mutex_lock(&priv->lock);
> > +
> > +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);  
> 
> couldn't conf_cache be used here?
> 
> > +	if (ret < 0) {
> > +		mutex_unlock(&priv->lock);
> > +		return ret;
> > +	}
> > +	data = (u16)ret;  
> 
> cast not needed
> 
> > +	data &= ~mask;
> > +	data |= mask & (val << shift);
> > +
> > +	ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
> > +	if (!ret && addr == TLA2024_CONF)
> > +		priv->conf_cache = data;
> > +
> > +	mutex_unlock(&priv->lock);  
> 
> maybe newline here
> 
> > +	return ret;
> > +}
> > +  
> 
> not sure if the following helper functions are worth the effort;
> parameter chan is not used
> 
> > +static int tla2024_get_conversion_mode(struct iio_dev *idev,
> > +				       const struct iio_chan_spec *chan)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +	int ret;
> > +	u16 data;
> > +
> > +	ret = tla2024_get(priv, TLA2024_CONF, CONF_MODE_MASK,
> > +			  CONF_MODE_SHIFT, &data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return data;
> > +}
> > +
> > +static int tla2024_set_conversion_mode(struct iio_dev *idev,
> > +				       const struct iio_chan_spec *chan,
> > +				       unsigned int data)  
> 
> data should be u16?
> 
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +
> > +	return tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
> > +			   CONF_MODE_SHIFT, data);
> > +}
> > +
> > +static int tla2024_get_full_scale_range(struct iio_dev *idev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +	int ret;
> > +	u16 data;
> > +
> > +	ret = tla2024_get(priv, TLA2024_CONF, CONF_PGA_MASK,
> > +			  CONF_PGA_SHIFT, &data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return data;
> > +}
> > +
> > +static int tla2024_set_full_scale_range(struct iio_dev *idev,
> > +					const struct iio_chan_spec *chan,
> > +					unsigned int data)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +
> > +	return tla2024_set(priv, TLA2024_CONF, CONF_PGA_MASK,
> > +			   CONF_PGA_SHIFT, data);
> > +}
> > +
> > +static const struct iio_enum tla2024_conversion_mode = {
> > +	.items = tla2024_conversion_mode_avail,
> > +	.num_items = ARRAY_SIZE(tla2024_conversion_mode_avail),
> > +	.get = tla2024_get_conversion_mode,
> > +	.set = tla2024_set_conversion_mode,
> > +};
> > +
> > +static const struct iio_enum tla2024_full_scale_range = {
> > +	.items = tla2024_full_scale_range_avail,
> > +	.num_items = ARRAY_SIZE(tla2024_full_scale_range_avail),
> > +	.get = tla2024_get_full_scale_range,
> > +	.set = tla2024_set_full_scale_range,
> > +};
> > +
> > +#define TLA2024_IIO_ENUM_AVAIL(_name, _shared, _e)			\
> > +{									\
> > +	.name = (_name "_available"),					\
> > +	.shared = (_shared),						\
> > +	.read = iio_enum_available_read,				\
> > +	.private = (uintptr_t)(_e),			\
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info tla202x_ext_info_pga[] = {
> > +	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
> > +		 &tla2024_conversion_mode),
> > +	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
> > +			       &tla2024_conversion_mode),
> > +	IIO_ENUM("full_scale_range", IIO_SHARED_BY_ALL,
> > +		 &tla2024_full_scale_range),

I haven't dug in the datasheet to understand this but the lack of range
attributes in IIO is entirely deliberate.  We want one standard
control for this and it's almost always _SCALE. If we had allowed
both range and scale to be controlled it would be ambiguous for userspace.
Scale came first ;)

You can then read back the resulting range using the read_avail
callback and the range specifier for that.


> > +	TLA2024_IIO_ENUM_AVAIL("full_scale_range", IIO_SHARED_BY_ALL,
> > +			       &tla2024_full_scale_range),
> > +	{},
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info tla202x_ext_info[] = {
> > +	IIO_ENUM("conversion_mode", IIO_SHARED_BY_ALL,
> > +		 &tla2024_conversion_mode),
The moment I see new ABI, I look for the docs.  It must
be documented under
Documentation/ABI/testing/sysfs-bus-iio*


> > +	TLA2024_IIO_ENUM_AVAIL("conversion_mode", IIO_SHARED_BY_ALL,
> > +			       &tla2024_conversion_mode),
> > +	{},
> > +};
> > +
> > +static int tla2024_read_avail(struct iio_dev *idev,
> > +			      struct iio_chan_spec const *chan,
> > +			      const int **vals, int *type, int *length,
> > +			      long mask)
> > +{
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +
> > +		*length = ARRAY_SIZE(tla2024_samp_freq_avail);
> > +		*vals = tla2024_samp_freq_avail;
> > +
> > +		*type = IIO_VAL_INT;
> > +		return IIO_AVAIL_LIST;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> > +{
> > +	u16 chan_idx = 0;
> > +	u32 tmp_p, tmp_n;
> > +	int ainp, ainn;
> > +	int ret;
> > +
> > +	ret = of_property_read_u32_index(ch, "single-ended", 0, &tmp_p);
There is now (only very recently) a generic description in DT for
acpi channels.  Please look at that:

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/devicetree/bindings/iio/adc/adc.txt?h=togreg&id=00426e99789357dbff7e719a092ce36a3ce49d94

It is minimalist.  If it is single ended, you can tell that by the absence
of diff-channels.

If the binding needs additional elements (it probably does)
then propose them for that generic binding.  We are trying to move
away from each driver doing it's own thing given all this stuff is
pretty generic!


> > +	if (ret) {
> > +		ret = of_property_read_u32_index(ch,
> > +						 "differential", 0, &tmp_p);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = of_property_read_u32_index(ch,
> > +						 "differential", 1, &tmp_n);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ainp = (int)tmp_p;
> > +		ainn = (int)tmp_n;
> > +	} else {
> > +		ainp = (int)tmp_p;
> > +		ainn = -1;
> > +	}
> > +
> > +	ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	// if model doesn"t have mux then only channel 0 is allowed  
> 
> are C++ style comments permitted these days?

Generally no.  Keep to style of the subsystem.

> 
> > +	if (!priv->model->mux_available && chan_idx)
> > +		return -EINVAL;
> > +
> > +	// if already used
> > +	if ((priv->used_mux_channels) & (1 << chan_idx))
> > +		return -EINVAL;
> > +
> > +	return chan_idx;
> > +}
> > +
> > +static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
> > +			     struct iio_chan_spec *chan)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +	u16 chan_idx = 0;  
> 
> initialization not needed
> 
> > +	int ret;
> > +
> > +	ret = tla2024_of_find_chan(priv, node);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	chan_idx = (u16)ret;  
> 
> cast not needed
> 
> > +	priv->used_mux_channels |= (1 << chan_idx);  
> 
> could use BIT() macro
> 
> > +	chan->type = IIO_VOLTAGE;
> > +	chan->channel = tla2024_all_channels[chan_idx].ainp;
> > +	chan->channel2 = tla2024_all_channels[chan_idx].ainn;
> > +	chan->differential = tla2024_all_channels[chan_idx].differential;
> > +	chan->extend_name = node->name;
> > +	chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
> > +	chan->indexed = 1;
> > +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +					BIT(IIO_CHAN_INFO_PROCESSED);

We very very rarely accept drivers providing bother raw and processed
for a given channel.  The only exceptions to this tend to be drivers
where we got it wrong initially, where we provided raw initially then
discovered that the conversion was non linear, but had to maintain
the existing userspace ABI.

So if it can be done via a linear conversion provided with offset and
scale then it should always be _RAW with scale and offset if needed.
Otherwise is should be processed.

> > +	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > +	chan->info_mask_shared_by_all_available =
> > +					BIT(IIO_CHAN_INFO_SAMP_FREQ);
> > +
> > +	if (priv->model->pga_available)
> > +		chan->ext_info = tla202x_ext_info_pga;
> > +	else
> > +		chan->ext_info = tla202x_ext_info;
> > +
> > +	return 0;
> > +}
> > +
> > +static int tla2024_wait(struct tla2024 *priv)
> > +{
> > +	int ret = 0;  
> 
> initialization not needed
> 
> > +	u16 retry = TLA2024_CONV_RETRY;  
> 
> retry is just a counter, should be int or unsigned int
> 
> > +	u16 status;
> > +
> > +	do {
> > +		if (!--retry)
> > +			return -EIO;
> > +		ret = tla2024_get(priv, TLA2024_CONF, CONF_OS_MASK,
> > +				  CONF_OS_SHIFT, &status);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (!status)
> > +			usleep_range(25, 1000);
> > +	} while (!status);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tla2024_select_channel(struct tla2024 *priv,
> > +				  struct iio_chan_spec const *chan)
> > +{
> > +	int ret = 0;
> > +	int ainp = chan->channel;
> > +	int ainn = chan->channel2;
> > +	u16 tmp, chan_id = 0;
> > +
> > +	tla2024_find_chan_idx(ainp, ainn, &chan_id);
> > +
> > +	tmp = (priv->conf_cache & CONF_MUX_MASK) >> CONF_MUX_SHIFT;
> > +	if (tmp != chan_id)

if (tmp == chan_id)
	return 0;

return tla24...

This one is a bit marginal but slightly nicer to read.

> > +		ret = tla2024_set(priv, TLA2024_CONF, CONF_MUX_MASK,
> > +				  CONF_MUX_SHIFT, chan_id);
> > +	return ret;
> > +}
> > +
> > +static int tla2024_convert(struct tla2024 *priv)
> > +{
> > +	int ret = 0;
> > +
> > +	if (priv->conf_cache & CONF_MODE_MASK) {
> > +		ret = tla2024_set(priv, TLA2024_CONF, CONF_OS_MASK,
> > +				  CONF_OS_SHIFT, 1);
> > +		if (ret < 0)
> > +			return ret;
> > +		ret = tla2024_wait(priv);
> > +	}
Flip this logic to simplify the function.

if (!(priv->conf_cache & CONF_MODE_MASK))
	return 0;

ret = tla...

return tla2024_wait(priv);

Simpler code and lower indent making it easier to read.


> > +	return ret;
> > +}
> > +
> > +static int tla2024_read_raw(struct iio_dev *idev,
> > +			    struct iio_chan_spec const *channel, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +	int ret;
> > +	u16 data, idx;
> > +	s16 tmp;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:  
> 
> I think there should be a mutex here to protect the selection of a channel 
> and get actualy reading of data
> 
> > +		ret = tla2024_select_channel(priv, channel);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = tla2024_convert(priv);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
> > +				  DATA_RES_SHIFT, &data);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = data;
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		ret = tla2024_select_channel(priv, channel);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = tla2024_convert(priv);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = tla2024_get(priv, TLA2024_DATA, DATA_RES_MASK,
> > +				  DATA_RES_SHIFT, &data);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		tmp = (s16)(data << DATA_RES_SHIFT);
> > +		idx = (priv->conf_cache & CONF_PGA_MASK) >> CONF_PGA_SHIFT;
> > +		*val = (tmp >> DATA_RES_SHIFT) * tla2024_pga_scale[idx];
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +
> > +		ret = tla2024_get(priv, TLA2024_CONF, CONF_DR_MASK,
> > +				  CONF_DR_SHIFT, &data);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = tla2024_samp_freq_avail[data];
> > +		return IIO_VAL_INT;
> > +
> > +	default:
> > +		break;  
> 
> just
> return -EINVAL
> here
> 
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int tla2024_write_raw(struct iio_dev *idev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct tla2024 *priv = iio_priv(idev);
> > +	u16 i;  
> 
> use int
> 
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +
> > +		for (i = 0; i < ARRAY_SIZE(tla2024_samp_freq_avail); i++) {
> > +			if (tla2024_samp_freq_avail[i] == val)
> > +				break;
> > +		}
> > +
> > +		if (i == ARRAY_SIZE(tla2024_samp_freq_avail))
> > +			return -EINVAL;
> > +
> > +		return tla2024_set(priv, TLA2024_CONF, CONF_DR_MASK,
> > +					CONF_DR_SHIFT, i);
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int tla2024_of_chan_init(struct iio_dev *idev)
> > +{
> > +	struct device_node *node = idev->dev.of_node;
> > +	struct device_node *child;
> > +	struct iio_chan_spec *channels;
> > +	int ret, i = 0, num_channels = 0;  
> 
> initialization not needed
> 
> > +
> > +	num_channels = of_get_available_child_count(node);
> > +	if (!num_channels) {
> > +		dev_err(&idev->dev, "no channels configured\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	channels = devm_kcalloc(&idev->dev, num_channels,
> > +				sizeof(struct iio_chan_spec), GFP_KERNEL);
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	i = 0;
> > +	for_each_available_child_of_node(node, child) {
> > +		ret = tla2024_init_chan(idev, child, &channels[i]);
> > +		if (ret) {
> > +			of_node_put(child);
> > +			return ret;
> > +		}
> > +		i++;
> > +	}
> > +
> > +	idev->channels = channels;
> > +	idev->num_channels = num_channels;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_info tla2024_info = {
> > +	.read_raw = tla2024_read_raw,
> > +	.write_raw = tla2024_write_raw,
> > +	.read_avail = tla2024_read_avail,
> > +};
> > +
> > +static int tla2024_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct iio_dev *iio;
> > +	struct tla2024 *priv;
> > +	struct tla202x_model *model;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_WORD_DATA))
> > +		return -EOPNOTSUPP;
> > +
> > +	model = &tla202x_models[id->driver_data];
> > +
> > +	iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> > +	if (!iio)
> > +		return -ENOMEM;
> > +
> > +	priv = iio_priv(iio);
> > +	priv->i2c = client;
> > +	priv->model = model;
> > +	mutex_init(&priv->lock);
> > +
> > +	iio->dev.parent = &client->dev;
> > +	iio->dev.of_node = client->dev.of_node;
> > +	iio->name = client->name;
> > +	iio->modes = INDIO_DIRECT_MODE;
> > +	iio->info = &tla2024_info;
> > +
> > +	ret = tla2024_of_chan_init(iio);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = tla2024_set(priv, TLA2024_CONF, CONF_MODE_MASK,
> > +			  CONF_MODE_SHIFT, 1);  
> 
> so what is the default mode? maybe use MAGIC for 1?
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = iio_device_register(iio);  
> 
> could use devm_ variant? to be able to drop _remove()
> and return directly
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int tla2024_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > +
> > +	iio_device_unregister(iio);
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id tla2024_id[] = {
> > +	{ "tla2024", TLA2024 },
> > +	{ "tla2022", TLA2022 },
> > +	{ "tla2021", TLA2021 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tla2024_id);
> > +
> > +static const struct of_device_id tla2024_of_match[] = {
> > +	{ .compatible = "ti,tla2024" },
> > +	{ .compatible = "ti,tla2022" },
> > +	{ .compatible = "ti,tla2021" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, tla2024_of_match);
> > +
> > +static struct i2c_driver tla2024_driver = {
> > +	.driver = {
> > +		.name = "tla2024",
> > +		.of_match_table = of_match_ptr(tla2024_of_match),
> > +	},
> > +	.probe = tla2024_probe,
> > +	.remove = tla2024_remove,
> > +	.id_table = tla2024_id,
> > +};
> > +module_i2c_driver(tla2024_driver);
> > +
> > +MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
> > +MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
> > +MODULE_LICENSE("GPL v2");
> >   
> 

Re: [PATCH] iio: Add driver for TLA202x ADCs

[Ibtsam Ul-Haq]

Hi,

Thanks a lot for the review. I shall apply all these in the v2, I do
have just one question, below.

> > > +
> > > +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> > > +{
> > > +   u16 chan_idx = 0;
> > > +   u32 tmp_p, tmp_n;
> > > +   int ainp, ainn;
> > > +   int ret;
> > > +
> > > +   ret = of_property_read_u32_index(ch, "single-ended", 0, &tmp_p);
> There is now (only very recently) a generic description in DT for
> acpi channels.  Please look at that:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/devicetree/bindings/iio/adc/adc.txt?h=togreg&id=00426e99789357dbff7e719a092ce36a3ce49d94
>
> It is minimalist.  If it is single ended, you can tell that by the absence
> of diff-channels.
>

How should we specify which single ended channel to use? For example,
say I want to configure the driver for reading single-ended values
only from input pin AIN3. When we only signify a single-ended channel
by the absence of "diff-channels", where should the "3" be specified?

> If the binding needs additional elements (it probably does)
> then propose them for that generic binding.  We are trying to move
> away from each driver doing it's own thing given all this stuff is
> pretty generic!
>

Sorry for trimming the message. Gmail was otherwise blocking my
outbound message, and I could not find any other way to get it
through.

Best regards,
Ibtsam Haq

Re: [PATCH] iio: Add driver for TLA202x ADCs

[Jonathan Cameron]

On Mon, 21 Jan 2019 10:49:17 +0100
Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com> wrote:

> Hi,
> 
> Thanks a lot for the review. I shall apply all these in the v2, I do
> have just one question, below.
> 
> > > > +
> > > > +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> > > > +{
> > > > +   u16 chan_idx = 0;
> > > > +   u32 tmp_p, tmp_n;
> > > > +   int ainp, ainn;
> > > > +   int ret;
> > > > +
> > > > +   ret = of_property_read_u32_index(ch, "single-ended", 0, &tmp_p);  
> > There is now (only very recently) a generic description in DT for
> > acpi channels.  Please look at that:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/devicetree/bindings/iio/adc/adc.txt?h=togreg&id=00426e99789357dbff7e719a092ce36a3ce49d94
> >
> > It is minimalist.  If it is single ended, you can tell that by the absence
> > of diff-channels.
> >  
> 
> How should we specify which single ended channel to use? For example,
> say I want to configure the driver for reading single-ended values
> only from input pin AIN3. When we only signify a single-ended channel
> by the absence of "diff-channels", where should the "3" be specified?

Fair question.  Stefan, what do you think makes sense?
(obviously others welcome but my guess is Stefan has thought about this
more than most ;)

Jonathan



> 
> > If the binding needs additional elements (it probably does)
> > then propose them for that generic binding.  We are trying to move
> > away from each driver doing it's own thing given all this stuff is
> > pretty generic!
> >  
> 
> Sorry for trimming the message. Gmail was otherwise blocking my
> outbound message, and I could not find any other way to get it
> through.
> 
> Best regards,
> Ibtsam Haq

[PATCH v2] iio: Add driver for TLA202x ADCs

[Ibtsam Ul-Haq]

Basic driver for Texas Instruments TLA202x series ADCs. Input
channels are configurable from the device tree. Input scaling
is supported. Trigger buffer support is not yet implemented.

Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
---
Changes in v2:
- changed commit message
- Added mutex to protect channel selection
- Removed redundant mutex around i2c transfers
- Removed PROCESSED channel output
- Added SCALE info
- Removed Continuous Mode since continuous read not supported
- Removed SAMP_FREQ info since continuous read not supported
---
 drivers/iio/adc/Kconfig      |  11 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-tla2024.c | 463 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 475 insertions(+)
 create mode 100644 drivers/iio/adc/ti-tla2024.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5762565..8c214c8 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -816,6 +816,17 @@ config TI_AM335X_ADC
 	  To compile this driver as a module, choose M here: the module will be
 	  called ti_am335x_adc.
 
+config TI_TLA2024
+	tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
+	depends on OF
+	depends on I2C
+	help
+	  Say yes here to build support for Texas Instruments TLA2024,
+	  TLA2022 or TLA2021 I2C Analog to Digital Converters.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ti-tla2024.
+
 config TI_TLC4541
 	tristate "Texas Instruments TLC4541 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9874e05..819f35b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
+obj-$(CONFIG_TI_TLA2024) += ti-tla2024.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
 obj-$(CONFIG_VF610_ADC) += vf610_adc.o
diff --git a/drivers/iio/adc/ti-tla2024.c b/drivers/iio/adc/ti-tla2024.c
new file mode 100644
index 0000000..e902eb8
--- /dev/null
+++ b/drivers/iio/adc/ti-tla2024.c
@@ -0,0 +1,463 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
+ *
+ * Copyright (C) 2019 Koninklijke Philips N.V.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#define TLA2024_DATA 0x00
+#define TLA2024_DATA_RES_MASK GENMASK(15, 4)
+#define TLA2024_DATA_RES_SHIFT 4
+
+#define TLA2024_CONF 0x01
+#define TLA2024_CONF_OS_MASK BIT(15)
+#define TLA2024_CONF_OS_SHIFT 15
+#define TLA2024_CONF_MUX_MASK GENMASK(14, 12)
+#define TLA2024_CONF_MUX_SHIFT 12
+#define TLA2024_CONF_PGA_MASK GENMASK(11, 9)
+#define TLA2024_CONF_PGA_SHIFT 9
+#define TLA2024_CONF_MODE_MASK BIT(8)
+#define TLA2024_CONF_MODE_SHIFT 8
+#define TLA2024_CONF_DR_MASK GENMASK(7, 5)
+#define TLA2024_CONF_DR_SHIFT 5
+
+#define TLA2024_CONV_RETRY 10
+
+struct tla202x_model {
+	unsigned int mux_available;
+	unsigned int pga_available;
+};
+
+struct tla2024 {
+	struct i2c_client *i2c;
+	struct tla202x_model *model;
+	struct mutex lock; /* protect channel selection */
+	u8 used_mux_channels;
+};
+
+struct tla2024_channel {
+	int ainp;
+	int ainn;
+	const char *datasheet_name;
+	bool differential;
+};
+
+static const struct tla2024_channel tla2024_all_channels[] = {
+	{0, 1, "AIN0-AIN1", 1},
+	{0, 3, "AIN0-AIN3", 1},
+	{1, 3, "AIN1-AIN3", 1},
+	{2, 3, "AIN2-AIN3", 1},
+	{0, -1, "AIN0-GND", 0},
+	{1, -1, "AIN1-GND", 0},
+	{2, -1, "AIN2-GND", 0},
+	{3, -1, "AIN3-GND", 0},
+};
+
+static int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)
+{
+	u16 i;
+
+	for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
+		if ((tla2024_all_channels[i].ainp == ainp_in) &&
+		    (tla2024_all_channels[i].ainn == ainn_in)) {
+			*idx = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+#define TLA202x_MODEL(_mux, _pga)		\
+	{					\
+		.mux_available = (_mux),	\
+		.pga_available = (_pga),	\
+	}
+
+enum tla2024_model_id {
+	TLA2024 = 0,
+	TLA2022 = 1,
+	TLA2021 = 2,
+};
+
+static struct tla202x_model tla202x_models[] = {
+	TLA202x_MODEL(1, 1), // TLA2024
+	TLA202x_MODEL(0, 1), // TLA2022
+	TLA202x_MODEL(0, 0), // TLA2021
+};
+
+static const int tla2024_scale[] = { /* scale, int plus micro */
+	3, 0, 2, 0, 1, 0, 0, 500000, 0, 250000, 0, 125000 };
+
+static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
+		       u16 shift, u16 *val)
+{
+	int ret;
+	u16 data;
+
+	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
+	if (ret < 0)
+		return ret;
+
+	data = ret;
+	*val = (mask & data) >> shift;
+
+	return 0;
+}
+
+static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
+		       u16 shift, u16 val)
+{
+	int ret;
+	u16 data;
+
+	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
+	if (ret < 0)
+		return ret;
+
+	data = ret;
+	data &= ~mask;
+	data |= mask & (val << shift);
+
+	ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
+
+	return ret;
+}
+
+static int tla2024_read_avail(struct iio_dev *idev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+
+		*length = ARRAY_SIZE(tla2024_scale);
+		*vals = tla2024_scale;
+
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
+static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
+{
+	u16 chan_idx = 0;
+	u32 tmp_p, tmp_n;
+	int ainp, ainn;
+	int ret;
+
+	ret = of_property_read_u32_index(ch, "single-channel", 0, &tmp_p);
+	if (ret) {
+		ret = of_property_read_u32_index(ch,
+						 "diff-channels", 0, &tmp_p);
+		if (ret)
+			return ret;
+
+		ret = of_property_read_u32_index(ch,
+						 "diff-channels", 1, &tmp_n);
+		if (ret)
+			return ret;
+
+		ainp = (int)tmp_p;
+		ainn = (int)tmp_n;
+	} else {
+		ainp = (int)tmp_p;
+		ainn = -1;
+	}
+
+	ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
+	if (ret < 0)
+		return ret;
+
+	/* if model doesn"t have mux then only channel 0 is allowed */
+	if (!priv->model->mux_available && chan_idx)
+		return -EINVAL;
+
+	/* if already used */
+	if ((priv->used_mux_channels) & (1 << chan_idx))
+		return -EINVAL;
+
+	return chan_idx;
+}
+
+static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
+			     struct iio_chan_spec *chan)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	u16 chan_idx;
+	int ret;
+
+	ret = tla2024_of_find_chan(priv, node);
+	if (ret < 0)
+		return ret;
+
+	chan_idx = ret;
+	priv->used_mux_channels |= BIT(chan_idx);
+	chan->type = IIO_VOLTAGE;
+	chan->channel = tla2024_all_channels[chan_idx].ainp;
+	chan->channel2 = tla2024_all_channels[chan_idx].ainn;
+	chan->differential = tla2024_all_channels[chan_idx].differential;
+	chan->extend_name = node->name;
+	chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
+	chan->indexed = 1;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE);
+	chan->info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE);
+
+	return 0;
+}
+
+static int tla2024_wait(struct tla2024 *priv)
+{
+	int ret;
+	unsigned int retry = TLA2024_CONV_RETRY;
+	u16 status;
+
+	do {
+		if (!--retry)
+			return -EIO;
+		ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
+				  TLA2024_CONF_OS_SHIFT, &status);
+		if (ret < 0)
+			return ret;
+		if (!status)
+			usleep_range(25, 1000);
+	} while (!status);
+
+	return ret;
+}
+
+static int tla2024_select_channel(struct tla2024 *priv,
+				  struct iio_chan_spec const *chan)
+{
+	int ret;
+	int ainp = chan->channel;
+	int ainn = chan->channel2;
+	u16 chan_id = 0;
+
+	ret = tla2024_find_chan_idx(ainp, ainn, &chan_id);
+	if (ret < 0)
+		return ret;
+
+	return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MUX_MASK,
+					   TLA2024_CONF_MUX_SHIFT, chan_id);
+}
+
+static int tla2024_convert(struct tla2024 *priv)
+{
+	int ret = 0;
+
+	ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
+			  TLA2024_CONF_OS_SHIFT, 1);
+	if (ret < 0)
+		return ret;
+
+	return tla2024_wait(priv);
+}
+
+static int tla2024_read_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	int ret;
+	u16 data;
+	s16 tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&priv->lock);
+		ret = tla2024_select_channel(priv, channel);
+		if (ret < 0)
+			goto return_unlock;
+
+		ret = tla2024_convert(priv);
+		if (ret < 0)
+			goto return_unlock;
+
+		ret = tla2024_get(priv, TLA2024_DATA, TLA2024_DATA_RES_MASK,
+				  TLA2024_DATA_RES_SHIFT, &data);
+		if (ret < 0)
+			goto return_unlock;
+
+		tmp = (s16)(data << TLA2024_DATA_RES_SHIFT);
+		*val = tmp >> TLA2024_DATA_RES_SHIFT;
+		ret = IIO_VAL_INT;
+
+return_unlock:
+		mutex_unlock(&priv->lock);
+		return ret;
+
+	case IIO_CHAN_INFO_SCALE:
+		if (!(priv->model->pga_available)) {
+			*val = 1; /* Scale always 1 mV when no PGA */
+			return IIO_VAL_INT;
+		}
+		ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
+				  TLA2024_CONF_PGA_SHIFT, &data);
+		if (ret < 0)
+			return ret;
+
+		/* gain for the 3bit pga values 6 and 7 is same as at 5 */
+		if (data >= (ARRAY_SIZE(tla2024_scale) >> 1))
+			data = (ARRAY_SIZE(tla2024_scale) >> 1) - 1;
+
+		*val = tla2024_scale[data << 1];
+		*val2 = tla2024_scale[(data << 1) + 1];
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tla2024_write_raw(struct iio_dev *idev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct tla2024 *priv = iio_priv(idev);
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (!(priv->model->pga_available))
+			return -EINVAL; /* scale can't be changed if no pga */
+
+		for (i = 0; i < ARRAY_SIZE(tla2024_scale); i = i + 2) {
+			if ((tla2024_scale[i] == val) &&
+			    (tla2024_scale[i + 1] == val2))
+				break;
+		}
+
+		if (i == ARRAY_SIZE(tla2024_scale))
+			return -EINVAL;
+
+		return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
++					TLA2024_CONF_PGA_SHIFT, i >> 1);
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int tla2024_of_chan_init(struct iio_dev *idev)
+{
+	struct device_node *node = idev->dev.of_node;
+	struct device_node *child;
+	struct iio_chan_spec *channels;
+	int ret, i, num_channels;
+
+	num_channels = of_get_available_child_count(node);
+	if (!num_channels) {
+		dev_err(&idev->dev, "no channels configured\n");
+		return -ENODEV;
+	}
+
+	channels = devm_kcalloc(&idev->dev, num_channels,
+				sizeof(struct iio_chan_spec), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_available_child_of_node(node, child) {
+		ret = tla2024_init_chan(idev, child, &channels[i]);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+		i++;
+	}
+
+	idev->channels = channels;
+	idev->num_channels = num_channels;
+
+	return 0;
+}
+
+static const struct iio_info tla2024_info = {
+	.read_raw = tla2024_read_raw,
+	.write_raw = tla2024_write_raw,
+	.read_avail = tla2024_read_avail,
+};
+
+static int tla2024_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct iio_dev *iio;
+	struct tla2024 *priv;
+	struct tla202x_model *model;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	model = &tla202x_models[id->driver_data];
+
+	iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
+	if (!iio)
+		return -ENOMEM;
+
+	priv = iio_priv(iio);
+	priv->i2c = client;
+	priv->model = model;
+	mutex_init(&priv->lock);
+
+	iio->dev.parent = &client->dev;
+	iio->dev.of_node = client->dev.of_node;
+	iio->name = client->name;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->info = &tla2024_info;
+
+	ret = tla2024_of_chan_init(iio);
+	if (ret < 0)
+		return ret;
+
+	ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MODE_MASK,
+			  TLA2024_CONF_MODE_SHIFT, 1);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, iio);
+}
+
+static const struct i2c_device_id tla2024_id[] = {
+	{ "tla2024", TLA2024 },
+	{ "tla2022", TLA2022 },
+	{ "tla2021", TLA2021 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tla2024_id);
+
+static const struct of_device_id tla2024_of_match[] = {
+	{ .compatible = "ti,tla2024" },
+	{ .compatible = "ti,tla2022" },
+	{ .compatible = "ti,tla2021" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tla2024_of_match);
+
+static struct i2c_driver tla2024_driver = {
+	.driver = {
+		.name = "tla2024",
+		.of_match_table = of_match_ptr(tla2024_of_match),
+	},
+	.probe = tla2024_probe,
+	.id_table = tla2024_id,
+};
+module_i2c_driver(tla2024_driver);
+
+MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
+MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2] iio: Add driver for TLA202x ADCs

[Jonathan Cameron]

On Wed, 13 Mar 2019 12:14:03 +0100
Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com> wrote:

> Basic driver for Texas Instruments TLA202x series ADCs. Input
> channels are configurable from the device tree. Input scaling
> is supported. Trigger buffer support is not yet implemented.
> 
> Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
One quick process thing - and this varies a bit by subsystem
so worth checking for each one what happened for previous drivers..

Please don't post new versions in replies to old threads.
They become buried in most email clients and it's not always
obvious whether different branches of the thread are talking
about particular versions of the driver.

Much better to just start a fresh thread and rely on the
consistent naming to make it obvious that it's a new version
of the same patch.

Anyhow, on to the review.

This needs a devicetree binding document.

Various minor bits inline.

Jonathan
> ---
> Changes in v2:
> - changed commit message
> - Added mutex to protect channel selection
> - Removed redundant mutex around i2c transfers
> - Removed PROCESSED channel output
> - Added SCALE info
> - Removed Continuous Mode since continuous read not supported
> - Removed SAMP_FREQ info since continuous read not supported
> ---
>  drivers/iio/adc/Kconfig      |  11 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ti-tla2024.c | 463 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 475 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-tla2024.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5762565..8c214c8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -816,6 +816,17 @@ config TI_AM335X_ADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ti_am335x_adc.
>  
> +config TI_TLA2024
> +	tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
> +	depends on OF
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Texas Instruments TLA2024,
> +	  TLA2022 or TLA2021 I2C Analog to Digital Converters.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ti-tla2024.
> +
>  config TI_TLC4541
>  	tristate "Texas Instruments TLC4541 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9874e05..819f35b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> +obj-$(CONFIG_TI_TLA2024) += ti-tla2024.o
>  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
>  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
>  obj-$(CONFIG_VF610_ADC) += vf610_adc.o
> diff --git a/drivers/iio/adc/ti-tla2024.c b/drivers/iio/adc/ti-tla2024.c
> new file mode 100644
> index 0000000..e902eb8
> --- /dev/null
> +++ b/drivers/iio/adc/ti-tla2024.c
> @@ -0,0 +1,463 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
> + *
> + * Copyright (C) 2019 Koninklijke Philips N.V.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#define TLA2024_DATA 0x00
> +#define TLA2024_DATA_RES_MASK GENMASK(15, 4)
> +#define TLA2024_DATA_RES_SHIFT 4
> +
> +#define TLA2024_CONF 0x01
> +#define TLA2024_CONF_OS_MASK BIT(15)
> +#define TLA2024_CONF_OS_SHIFT 15
> +#define TLA2024_CONF_MUX_MASK GENMASK(14, 12)
> +#define TLA2024_CONF_MUX_SHIFT 12
> +#define TLA2024_CONF_PGA_MASK GENMASK(11, 9)
> +#define TLA2024_CONF_PGA_SHIFT 9
> +#define TLA2024_CONF_MODE_MASK BIT(8)
> +#define TLA2024_CONF_MODE_SHIFT 8
> +#define TLA2024_CONF_DR_MASK GENMASK(7, 5)
> +#define TLA2024_CONF_DR_SHIFT 5
> +
> +#define TLA2024_CONV_RETRY 10
> +
> +struct tla202x_model {
> +	unsigned int mux_available;
> +	unsigned int pga_available;
> +};
> +
> +struct tla2024 {
> +	struct i2c_client *i2c;
> +	struct tla202x_model *model;
> +	struct mutex lock; /* protect channel selection */
> +	u8 used_mux_channels;
> +};
> +
> +struct tla2024_channel {
> +	int ainp;
> +	int ainn;
> +	const char *datasheet_name;
> +	bool differential;
> +};
> +
> +static const struct tla2024_channel tla2024_all_channels[] = {
> +	{0, 1, "AIN0-AIN1", 1},
> +	{0, 3, "AIN0-AIN3", 1},
> +	{1, 3, "AIN1-AIN3", 1},
> +	{2, 3, "AIN2-AIN3", 1},
> +	{0, -1, "AIN0-GND", 0},

Single ended, so normal convention would just be AIN0

> +	{1, -1, "AIN1-GND", 0},
> +	{2, -1, "AIN2-GND", 0},
> +	{3, -1, "AIN3-GND", 0},
> +};
> +
> +static int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)
> +{
> +	u16 i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
> +		if ((tla2024_all_channels[i].ainp == ainp_in) &&
> +		    (tla2024_all_channels[i].ainn == ainn_in)) {
> +			*idx = i;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +#define TLA202x_MODEL(_mux, _pga)		\
> +	{					\
> +		.mux_available = (_mux),	\
> +		.pga_available = (_pga),	\
> +	}
> +
> +enum tla2024_model_id {
> +	TLA2024 = 0,
> +	TLA2022 = 1,
> +	TLA2021 = 2,
> +};
> +
> +static struct tla202x_model tla202x_models[] = {
> +	TLA202x_MODEL(1, 1), // TLA2024
[TLA2024] = TLA202x_MODEL(1, 1), 

Makes it clear which is which and avoids the ugly c++ comments.

> +	TLA202x_MODEL(0, 1), // TLA2022
> +	TLA202x_MODEL(0, 0), // TLA2021
> +};
> +
> +static const int tla2024_scale[] = { /* scale, int plus micro */
> +	3, 0, 2, 0, 1, 0, 0, 500000, 0, 250000, 0, 125000 };
> +
> +static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
> +		       u16 shift, u16 *val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> +	if (ret < 0)
> +		return ret;
> +
> +	data = ret;
> +	*val = (mask & data) >> shift;
I'm going to guess that your shift is always just that needed
to move the mask to 0?  As such we have the really neat
FIELD_GET macros to avoid the need to pass both shifts and
masks around (it computes the shift from the mask).

> +
> +	return 0;
> +}
> +
> +static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
> +		       u16 shift, u16 val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> +	if (ret < 0)
> +		return ret;
> +
> +	data = ret;
> +	data &= ~mask;
> +	data |= mask & (val << shift);
> +
> +	ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
> +
> +	return ret;
> +}
> +
> +static int tla2024_read_avail(struct iio_dev *idev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +
> +		*length = ARRAY_SIZE(tla2024_scale);
> +		*vals = tla2024_scale;
> +
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> +{
> +	u16 chan_idx = 0;
> +	u32 tmp_p, tmp_n;
> +	int ainp, ainn;
> +	int ret;
> +
> +	ret = of_property_read_u32_index(ch, "single-channel", 0, &tmp_p);

This property isn't currently in the dt docs (I think...)
Hence the generic binding doc needs amending.

> +	if (ret) {
> +		ret = of_property_read_u32_index(ch,
> +						 "diff-channels", 0, &tmp_p);
> +		if (ret)
> +			return ret;
> +
> +		ret = of_property_read_u32_index(ch,
> +						 "diff-channels", 1, &tmp_n);
> +		if (ret)
> +			return ret;
> +
> +		ainp = (int)tmp_p;
> +		ainn = (int)tmp_n;
> +	} else {
> +		ainp = (int)tmp_p;
> +		ainn = -1;

Given this value is 'magic' anyway just use MAX_UINT instead and avoid
the need for negative handling?  That gets rid of the need for tmp_p, tmp_n
above.

> +	}
> +
> +	ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* if model doesn"t have mux then only channel 0 is allowed */
> +	if (!priv->model->mux_available && chan_idx)
> +		return -EINVAL;
> +
> +	/* if already used */
> +	if ((priv->used_mux_channels) & (1 << chan_idx))
> +		return -EINVAL;
> +
> +	return chan_idx;
> +}
> +
> +static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
> +			     struct iio_chan_spec *chan)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	u16 chan_idx;
> +	int ret;
> +
> +	ret = tla2024_of_find_chan(priv, node);
> +	if (ret < 0)
> +		return ret;
> +
> +	chan_idx = ret;
> +	priv->used_mux_channels |= BIT(chan_idx);
> +	chan->type = IIO_VOLTAGE;
> +	chan->channel = tla2024_all_channels[chan_idx].ainp;
> +	chan->channel2 = tla2024_all_channels[chan_idx].ainn;
> +	chan->differential = tla2024_all_channels[chan_idx].differential;
> +	chan->extend_name = node->name;
This is a bad idea.  You've just created a device specific userspace ABI
so all generic tools cannot be used.  What is the intended purpose of
doing this?

> +	chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
> +	chan->indexed = 1;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE);
> +
> +	return 0;
> +}
> +
> +static int tla2024_wait(struct tla2024 *priv)
> +{
> +	int ret;
> +	unsigned int retry = TLA2024_CONV_RETRY;
> +	u16 status;
> +
> +	do {
> +		if (!--retry)
> +			return -EIO;
> +		ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> +				  TLA2024_CONF_OS_SHIFT, &status);
> +		if (ret < 0)
> +			return ret;
> +		if (!status)
> +			usleep_range(25, 1000);
> +	} while (!status);
> +
> +	return ret;
> +}
> +
> +static int tla2024_select_channel(struct tla2024 *priv,
> +				  struct iio_chan_spec const *chan)
> +{
> +	int ret;
> +	int ainp = chan->channel;
> +	int ainn = chan->channel2;
> +	u16 chan_id = 0;
> +
> +	ret = tla2024_find_chan_idx(ainp, ainn, &chan_id);
> +	if (ret < 0)
> +		return ret;
> +
> +	return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MUX_MASK,
> +					   TLA2024_CONF_MUX_SHIFT, chan_id);
> +}
> +
> +static int tla2024_convert(struct tla2024 *priv)
> +{
> +	int ret = 0;
> +
> +	ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> +			  TLA2024_CONF_OS_SHIFT, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return tla2024_wait(priv);
> +}
> +
> +static int tla2024_read_raw(struct iio_dev *idev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	int ret;
> +	u16 data;
> +	s16 tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&priv->lock);
> +		ret = tla2024_select_channel(priv, channel);
> +		if (ret < 0)
> +			goto return_unlock;
> +
> +		ret = tla2024_convert(priv);
> +		if (ret < 0)
> +			goto return_unlock;
> +
> +		ret = tla2024_get(priv, TLA2024_DATA, TLA2024_DATA_RES_MASK,
> +				  TLA2024_DATA_RES_SHIFT, &data);
> +		if (ret < 0)
> +			goto return_unlock;
> +
> +		tmp = (s16)(data << TLA2024_DATA_RES_SHIFT);
> +		*val = tmp >> TLA2024_DATA_RES_SHIFT;
> +		ret = IIO_VAL_INT;
> +
The moment we get a deeply indented goto target like this it
always suggests to me that we should consider pulling the code
being protected by the lock out to a separate function, leaving
the lock external so you can have a simple

lock
ret = functioncall
unlock
if (ret)... or just return ret;

> +return_unlock:
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		if (!(priv->model->pga_available)) {
> +			*val = 1; /* Scale always 1 mV when no PGA */
> +			return IIO_VAL_INT;
> +		}
> +		ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> +				  TLA2024_CONF_PGA_SHIFT, &data);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* gain for the 3bit pga values 6 and 7 is same as at 5 */
> +		if (data >= (ARRAY_SIZE(tla2024_scale) >> 1))
> +			data = (ARRAY_SIZE(tla2024_scale) >> 1) - 1;
> +
> +		*val = tla2024_scale[data << 1];
> +		*val2 = tla2024_scale[(data << 1) + 1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tla2024_write_raw(struct iio_dev *idev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct tla2024 *priv = iio_priv(idev);
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (!(priv->model->pga_available))
> +			return -EINVAL; /* scale can't be changed if no pga */
> +
> +		for (i = 0; i < ARRAY_SIZE(tla2024_scale); i = i + 2) {
> +			if ((tla2024_scale[i] == val) &&
> +			    (tla2024_scale[i + 1] == val2))
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(tla2024_scale))
> +			return -EINVAL;
> +

This need a lock I think...

> +		return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> ++					TLA2024_CONF_PGA_SHIFT, i >> 1);
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int tla2024_of_chan_init(struct iio_dev *idev)
> +{
> +	struct device_node *node = idev->dev.of_node;
> +	struct device_node *child;
> +	struct iio_chan_spec *channels;
> +	int ret, i, num_channels;

> +
> +	num_channels = of_get_available_child_count(node);
> +	if (!num_channels) {
> +		dev_err(&idev->dev, "no channels configured\n");
> +		return -ENODEV;
> +	}
> +
> +	channels = devm_kcalloc(&idev->dev, num_channels,
> +				sizeof(struct iio_chan_spec), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	for_each_available_child_of_node(node, child) {
> +		ret = tla2024_init_chan(idev, child, &channels[i]);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +		i++;
> +	}
> +
> +	idev->channels = channels;
> +	idev->num_channels = num_channels;
> +
> +	return 0;
> +}
> +
> +static const struct iio_info tla2024_info = {
> +	.read_raw = tla2024_read_raw,
> +	.write_raw = tla2024_write_raw,
> +	.read_avail = tla2024_read_avail,
> +};
> +
> +static int tla2024_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *iio;
> +	struct tla2024 *priv;
> +	struct tla202x_model *model;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EOPNOTSUPP;
> +
> +	model = &tla202x_models[id->driver_data];

What is the point in this local variable?  I would just
assign this directly below.

> +
> +	iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(iio);
> +	priv->i2c = client;
> +	priv->model = model;
> +	mutex_init(&priv->lock);
> +
> +	iio->dev.parent = &client->dev;
> +	iio->dev.of_node = client->dev.of_node;
> +	iio->name = client->name;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &tla2024_info;
> +
> +	ret = tla2024_of_chan_init(iio);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MODE_MASK,
> +			  TLA2024_CONF_MODE_SHIFT, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, iio);
> +}
> +
> +static const struct i2c_device_id tla2024_id[] = {
> +	{ "tla2024", TLA2024 },
> +	{ "tla2022", TLA2022 },
> +	{ "tla2021", TLA2021 },

Really minor point, but reverse numerical order is 'unusual'.. 

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tla2024_id);
> +
> +static const struct of_device_id tla2024_of_match[] = {
> +	{ .compatible = "ti,tla2024" },
> +	{ .compatible = "ti,tla2022" },
> +	{ .compatible = "ti,tla2021" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tla2024_of_match);
> +
> +static struct i2c_driver tla2024_driver = {
> +	.driver = {
> +		.name = "tla2024",
> +		.of_match_table = of_match_ptr(tla2024_of_match),
> +	},
> +	.probe = tla2024_probe,
> +	.id_table = tla2024_id,
> +};
> +module_i2c_driver(tla2024_driver);
> +
> +MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
> +MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2] iio: Add driver for TLA202x ADCs

[Ibtsam Ul-Haq]

Thanks for the review. I shall make these changes in v3. One comment below.

On Sat, Mar 16, 2019 at 3:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 13 Mar 2019 12:14:03 +0100
> Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com> wrote:
>
> > Basic driver for Texas Instruments TLA202x series ADCs. Input
> > channels are configurable from the device tree. Input scaling
> > is supported. Trigger buffer support is not yet implemented.
> >
> > Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>
> One quick process thing - and this varies a bit by subsystem
> so worth checking for each one what happened for previous drivers..
>
> Please don't post new versions in replies to old threads.
> They become buried in most email clients and it's not always
> obvious whether different branches of the thread are talking
> about particular versions of the driver.
>
> Much better to just start a fresh thread and rely on the
> consistent naming to make it obvious that it's a new version
> of the same patch.
>
> Anyhow, on to the review.
>
> This needs a devicetree binding document.
>
> Various minor bits inline.
>
> Jonathan
> > ---
> > Changes in v2:
> > - changed commit message
> > - Added mutex to protect channel selection
> > - Removed redundant mutex around i2c transfers
> > - Removed PROCESSED channel output
> > - Added SCALE info
> > - Removed Continuous Mode since continuous read not supported
> > - Removed SAMP_FREQ info since continuous read not supported
> > ---
> >  drivers/iio/adc/Kconfig      |  11 +
> >  drivers/iio/adc/Makefile     |   1 +
> >  drivers/iio/adc/ti-tla2024.c | 463 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 475 insertions(+)
> >  create mode 100644 drivers/iio/adc/ti-tla2024.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 5762565..8c214c8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -816,6 +816,17 @@ config TI_AM335X_ADC
> >         To compile this driver as a module, choose M here: the module will be
> >         called ti_am335x_adc.
> >
> > +config TI_TLA2024
> > +     tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
> > +     depends on OF
> > +     depends on I2C
> > +     help
> > +       Say yes here to build support for Texas Instruments TLA2024,
> > +       TLA2022 or TLA2021 I2C Analog to Digital Converters.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called ti-tla2024.
> > +
> >  config TI_TLC4541
> >       tristate "Texas Instruments TLC4541 ADC driver"
> >       depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 9874e05..819f35b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -75,6 +75,7 @@ obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> >  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> >  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> >  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> > +obj-$(CONFIG_TI_TLA2024) += ti-tla2024.o
> >  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> >  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> >  obj-$(CONFIG_VF610_ADC) += vf610_adc.o
> > diff --git a/drivers/iio/adc/ti-tla2024.c b/drivers/iio/adc/ti-tla2024.c
> > new file mode 100644
> > index 0000000..e902eb8
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-tla2024.c
> > @@ -0,0 +1,463 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
> > + *
> > + * Copyright (C) 2019 Koninklijke Philips N.V.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +
> > +#define TLA2024_DATA 0x00
> > +#define TLA2024_DATA_RES_MASK GENMASK(15, 4)
> > +#define TLA2024_DATA_RES_SHIFT 4
> > +
> > +#define TLA2024_CONF 0x01
> > +#define TLA2024_CONF_OS_MASK BIT(15)
> > +#define TLA2024_CONF_OS_SHIFT 15
> > +#define TLA2024_CONF_MUX_MASK GENMASK(14, 12)
> > +#define TLA2024_CONF_MUX_SHIFT 12
> > +#define TLA2024_CONF_PGA_MASK GENMASK(11, 9)
> > +#define TLA2024_CONF_PGA_SHIFT 9
> > +#define TLA2024_CONF_MODE_MASK BIT(8)
> > +#define TLA2024_CONF_MODE_SHIFT 8
> > +#define TLA2024_CONF_DR_MASK GENMASK(7, 5)
> > +#define TLA2024_CONF_DR_SHIFT 5
> > +
> > +#define TLA2024_CONV_RETRY 10
> > +
> > +struct tla202x_model {
> > +     unsigned int mux_available;
> > +     unsigned int pga_available;
> > +};
> > +
> > +struct tla2024 {
> > +     struct i2c_client *i2c;
> > +     struct tla202x_model *model;
> > +     struct mutex lock; /* protect channel selection */
> > +     u8 used_mux_channels;
> > +};
> > +
> > +struct tla2024_channel {
> > +     int ainp;
> > +     int ainn;
> > +     const char *datasheet_name;
> > +     bool differential;
> > +};
> > +
> > +static const struct tla2024_channel tla2024_all_channels[] = {
> > +     {0, 1, "AIN0-AIN1", 1},
> > +     {0, 3, "AIN0-AIN3", 1},
> > +     {1, 3, "AIN1-AIN3", 1},
> > +     {2, 3, "AIN2-AIN3", 1},
> > +     {0, -1, "AIN0-GND", 0},
>
> Single ended, so normal convention would just be AIN0
>
> > +     {1, -1, "AIN1-GND", 0},
> > +     {2, -1, "AIN2-GND", 0},
> > +     {3, -1, "AIN3-GND", 0},
> > +};
> > +
> > +static int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)
> > +{
> > +     u16 i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
> > +             if ((tla2024_all_channels[i].ainp == ainp_in) &&
> > +                 (tla2024_all_channels[i].ainn == ainn_in)) {
> > +                     *idx = i;
> > +                     return 0;
> > +             }
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +#define TLA202x_MODEL(_mux, _pga)            \
> > +     {                                       \
> > +             .mux_available = (_mux),        \
> > +             .pga_available = (_pga),        \
> > +     }
> > +
> > +enum tla2024_model_id {
> > +     TLA2024 = 0,
> > +     TLA2022 = 1,
> > +     TLA2021 = 2,
> > +};
> > +
> > +static struct tla202x_model tla202x_models[] = {
> > +     TLA202x_MODEL(1, 1), // TLA2024
> [TLA2024] = TLA202x_MODEL(1, 1),
>
> Makes it clear which is which and avoids the ugly c++ comments.
>
> > +     TLA202x_MODEL(0, 1), // TLA2022
> > +     TLA202x_MODEL(0, 0), // TLA2021
> > +};
> > +
> > +static const int tla2024_scale[] = { /* scale, int plus micro */
> > +     3, 0, 2, 0, 1, 0, 0, 500000, 0, 250000, 0, 125000 };
> > +
> > +static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
> > +                    u16 shift, u16 *val)
> > +{
> > +     int ret;
> > +     u16 data;
> > +
> > +     ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     data = ret;
> > +     *val = (mask & data) >> shift;
> I'm going to guess that your shift is always just that needed
> to move the mask to 0?  As such we have the really neat
> FIELD_GET macros to avoid the need to pass both shifts and
> masks around (it computes the shift from the mask).
>

yes but my understanding was that FIELD_GET/FIELD_PREP
require the "mask" to be a compile-time constant, so I could not
use them in the somewhat generic get/set.

I could still use them if I just remove the get/set. Then I could use
i2c read/write and apply FIELD_GET/PREP on the output/input with
a context-specific static mask. Is this the preferred way? Sorry if I
am missing something completely...

> > +
> > +     return 0;
> > +}
> > +
> > +static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
> > +                    u16 shift, u16 val)
> > +{
> > +     int ret;
> > +     u16 data;
> > +
> > +     ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     data = ret;
> > +     data &= ~mask;
> > +     data |= mask & (val << shift);
> > +
> > +     ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
> > +
> > +     return ret;
> > +}
> > +
> > +static int tla2024_read_avail(struct iio_dev *idev,
> > +                           struct iio_chan_spec const *chan,
> > +                           const int **vals, int *type, int *length,
> > +                           long mask)
> > +{
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_SCALE:
> > +
> > +             *length = ARRAY_SIZE(tla2024_scale);
> > +             *vals = tla2024_scale;
> > +
> > +             *type = IIO_VAL_INT_PLUS_MICRO;
> > +             return IIO_AVAIL_LIST;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> > +{
> > +     u16 chan_idx = 0;
> > +     u32 tmp_p, tmp_n;
> > +     int ainp, ainn;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32_index(ch, "single-channel", 0, &tmp_p);
>
> This property isn't currently in the dt docs (I think...)
> Hence the generic binding doc needs amending.
>
> > +     if (ret) {
> > +             ret = of_property_read_u32_index(ch,
> > +                                              "diff-channels", 0, &tmp_p);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = of_property_read_u32_index(ch,
> > +                                              "diff-channels", 1, &tmp_n);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ainp = (int)tmp_p;
> > +             ainn = (int)tmp_n;
> > +     } else {
> > +             ainp = (int)tmp_p;
> > +             ainn = -1;
>
> Given this value is 'magic' anyway just use MAX_UINT instead and avoid
> the need for negative handling?  That gets rid of the need for tmp_p, tmp_n
> above.
>
> > +     }
> > +
> > +     ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* if model doesn"t have mux then only channel 0 is allowed */
> > +     if (!priv->model->mux_available && chan_idx)
> > +             return -EINVAL;
> > +
> > +     /* if already used */
> > +     if ((priv->used_mux_channels) & (1 << chan_idx))
> > +             return -EINVAL;
> > +
> > +     return chan_idx;
> > +}
> > +
> > +static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
> > +                          struct iio_chan_spec *chan)
> > +{
> > +     struct tla2024 *priv = iio_priv(idev);
> > +     u16 chan_idx;
> > +     int ret;
> > +
> > +     ret = tla2024_of_find_chan(priv, node);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     chan_idx = ret;
> > +     priv->used_mux_channels |= BIT(chan_idx);
> > +     chan->type = IIO_VOLTAGE;
> > +     chan->channel = tla2024_all_channels[chan_idx].ainp;
> > +     chan->channel2 = tla2024_all_channels[chan_idx].ainn;
> > +     chan->differential = tla2024_all_channels[chan_idx].differential;
> > +     chan->extend_name = node->name;
> This is a bad idea.  You've just created a device specific userspace ABI
> so all generic tools cannot be used.  What is the intended purpose of
> doing this?
>
> > +     chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
> > +     chan->indexed = 1;
> > +     chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +     chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE);
> > +     chan->info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE);
> > +
> > +     return 0;
> > +}
> > +
> > +static int tla2024_wait(struct tla2024 *priv)
> > +{
> > +     int ret;
> > +     unsigned int retry = TLA2024_CONV_RETRY;
> > +     u16 status;
> > +
> > +     do {
> > +             if (!--retry)
> > +                     return -EIO;
> > +             ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> > +                               TLA2024_CONF_OS_SHIFT, &status);
> > +             if (ret < 0)
> > +                     return ret;
> > +             if (!status)
> > +                     usleep_range(25, 1000);
> > +     } while (!status);
> > +
> > +     return ret;
> > +}
> > +
> > +static int tla2024_select_channel(struct tla2024 *priv,
> > +                               struct iio_chan_spec const *chan)
> > +{
> > +     int ret;
> > +     int ainp = chan->channel;
> > +     int ainn = chan->channel2;
> > +     u16 chan_id = 0;
> > +
> > +     ret = tla2024_find_chan_idx(ainp, ainn, &chan_id);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MUX_MASK,
> > +                                        TLA2024_CONF_MUX_SHIFT, chan_id);
> > +}
> > +
> > +static int tla2024_convert(struct tla2024 *priv)
> > +{
> > +     int ret = 0;
> > +
> > +     ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> > +                       TLA2024_CONF_OS_SHIFT, 1);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return tla2024_wait(priv);
> > +}
> > +
> > +static int tla2024_read_raw(struct iio_dev *idev,
> > +                         struct iio_chan_spec const *channel, int *val,
> > +                         int *val2, long mask)
> > +{
> > +     struct tla2024 *priv = iio_priv(idev);
> > +     int ret;
> > +     u16 data;
> > +     s16 tmp;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             mutex_lock(&priv->lock);
> > +             ret = tla2024_select_channel(priv, channel);
> > +             if (ret < 0)
> > +                     goto return_unlock;
> > +
> > +             ret = tla2024_convert(priv);
> > +             if (ret < 0)
> > +                     goto return_unlock;
> > +
> > +             ret = tla2024_get(priv, TLA2024_DATA, TLA2024_DATA_RES_MASK,
> > +                               TLA2024_DATA_RES_SHIFT, &data);
> > +             if (ret < 0)
> > +                     goto return_unlock;
> > +
> > +             tmp = (s16)(data << TLA2024_DATA_RES_SHIFT);
> > +             *val = tmp >> TLA2024_DATA_RES_SHIFT;
> > +             ret = IIO_VAL_INT;
> > +
> The moment we get a deeply indented goto target like this it
> always suggests to me that we should consider pulling the code
> being protected by the lock out to a separate function, leaving
> the lock external so you can have a simple
>
> lock
> ret = functioncall
> unlock
> if (ret)... or just return ret;
>
> > +return_unlock:
> > +             mutex_unlock(&priv->lock);
> > +             return ret;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             if (!(priv->model->pga_available)) {
> > +                     *val = 1; /* Scale always 1 mV when no PGA */
> > +                     return IIO_VAL_INT;
> > +             }
> > +             ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> > +                               TLA2024_CONF_PGA_SHIFT, &data);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             /* gain for the 3bit pga values 6 and 7 is same as at 5 */
> > +             if (data >= (ARRAY_SIZE(tla2024_scale) >> 1))
> > +                     data = (ARRAY_SIZE(tla2024_scale) >> 1) - 1;
> > +
> > +             *val = tla2024_scale[data << 1];
> > +             *val2 = tla2024_scale[(data << 1) + 1];
> > +             return IIO_VAL_INT_PLUS_MICRO;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static int tla2024_write_raw(struct iio_dev *idev,
> > +                          struct iio_chan_spec const *chan,
> > +                          int val, int val2, long mask)
> > +{
> > +     struct tla2024 *priv = iio_priv(idev);
> > +     int i;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_SCALE:
> > +             if (!(priv->model->pga_available))
> > +                     return -EINVAL; /* scale can't be changed if no pga */
> > +
> > +             for (i = 0; i < ARRAY_SIZE(tla2024_scale); i = i + 2) {
> > +                     if ((tla2024_scale[i] == val) &&
> > +                         (tla2024_scale[i + 1] == val2))
> > +                             break;
> > +             }
> > +
> > +             if (i == ARRAY_SIZE(tla2024_scale))
> > +                     return -EINVAL;
> > +
>
> This need a lock I think...
>
> > +             return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> > ++                                    TLA2024_CONF_PGA_SHIFT, i >> 1);
> > +
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int tla2024_of_chan_init(struct iio_dev *idev)
> > +{
> > +     struct device_node *node = idev->dev.of_node;
> > +     struct device_node *child;
> > +     struct iio_chan_spec *channels;
> > +     int ret, i, num_channels;
>
> > +
> > +     num_channels = of_get_available_child_count(node);
> > +     if (!num_channels) {
> > +             dev_err(&idev->dev, "no channels configured\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     channels = devm_kcalloc(&idev->dev, num_channels,
> > +                             sizeof(struct iio_chan_spec), GFP_KERNEL);
> > +     if (!channels)
> > +             return -ENOMEM;
> > +
> > +     i = 0;
> > +     for_each_available_child_of_node(node, child) {
> > +             ret = tla2024_init_chan(idev, child, &channels[i]);
> > +             if (ret) {
> > +                     of_node_put(child);
> > +                     return ret;
> > +             }
> > +             i++;
> > +     }
> > +
> > +     idev->channels = channels;
> > +     idev->num_channels = num_channels;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct iio_info tla2024_info = {
> > +     .read_raw = tla2024_read_raw,
> > +     .write_raw = tla2024_write_raw,
> > +     .read_avail = tla2024_read_avail,
> > +};
> > +
> > +static int tla2024_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *id)
> > +{
> > +     struct iio_dev *iio;
> > +     struct tla2024 *priv;
> > +     struct tla202x_model *model;
> > +     int ret;
> > +
> > +     if (!i2c_check_functionality(client->adapter,
> > +                                  I2C_FUNC_SMBUS_WORD_DATA))
> > +             return -EOPNOTSUPP;
> > +
> > +     model = &tla202x_models[id->driver_data];
>
> What is the point in this local variable?  I would just
> assign this directly below.
>
> > +
> > +     iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> > +     if (!iio)
> > +             return -ENOMEM;
> > +
> > +     priv = iio_priv(iio);
> > +     priv->i2c = client;
> > +     priv->model = model;
> > +     mutex_init(&priv->lock);
> > +
> > +     iio->dev.parent = &client->dev;
> > +     iio->dev.of_node = client->dev.of_node;
> > +     iio->name = client->name;
> > +     iio->modes = INDIO_DIRECT_MODE;
> > +     iio->info = &tla2024_info;
> > +
> > +     ret = tla2024_of_chan_init(iio);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MODE_MASK,
> > +                       TLA2024_CONF_MODE_SHIFT, 1);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return devm_iio_device_register(&client->dev, iio);
> > +}
> > +
> > +static const struct i2c_device_id tla2024_id[] = {
> > +     { "tla2024", TLA2024 },
> > +     { "tla2022", TLA2022 },
> > +     { "tla2021", TLA2021 },
>
> Really minor point, but reverse numerical order is 'unusual'..
>
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tla2024_id);
> > +
> > +static const struct of_device_id tla2024_of_match[] = {
> > +     { .compatible = "ti,tla2024" },
> > +     { .compatible = "ti,tla2022" },
> > +     { .compatible = "ti,tla2021" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, tla2024_of_match);
> > +
> > +static struct i2c_driver tla2024_driver = {
> > +     .driver = {
> > +             .name = "tla2024",
> > +             .of_match_table = of_match_ptr(tla2024_of_match),
> > +     },
> > +     .probe = tla2024_probe,
> > +     .id_table = tla2024_id,
> > +};
> > +module_i2c_driver(tla2024_driver);
> > +
> > +MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
> > +MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
> > +MODULE_LICENSE("GPL v2");
>

Re: [PATCH v2] iio: Add driver for TLA202x ADCs

[Jonathan Cameron]

On Fri, 22 Mar 2019 10:24:00 +0100
Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com> wrote:

> Thanks for the review. I shall make these changes in v3. One comment below.
> 
> On Sat, Mar 16, 2019 at 3:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 13 Mar 2019 12:14:03 +0100
> > Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com> wrote:
> >  
> > > Basic driver for Texas Instruments TLA202x series ADCs. Input
> > > channels are configurable from the device tree. Input scaling
> > > is supported. Trigger buffer support is not yet implemented.
> > >
> > > Signed-off-by: Ibtsam Ul-Haq <ibtsam.haq.0x01@gmail.com>  
> > One quick process thing - and this varies a bit by subsystem
> > so worth checking for each one what happened for previous drivers..
> >
> > Please don't post new versions in replies to old threads.
> > They become buried in most email clients and it's not always
> > obvious whether different branches of the thread are talking
> > about particular versions of the driver.
> >
> > Much better to just start a fresh thread and rely on the
> > consistent naming to make it obvious that it's a new version
> > of the same patch.
> >
> > Anyhow, on to the review.
> >
> > This needs a devicetree binding document.
> >
> > Various minor bits inline.
> >
> > Jonathan  
> > > ---
> > > Changes in v2:
> > > - changed commit message
> > > - Added mutex to protect channel selection
> > > - Removed redundant mutex around i2c transfers
> > > - Removed PROCESSED channel output
> > > - Added SCALE info
> > > - Removed Continuous Mode since continuous read not supported
> > > - Removed SAMP_FREQ info since continuous read not supported
> > > ---
> > >  drivers/iio/adc/Kconfig      |  11 +
> > >  drivers/iio/adc/Makefile     |   1 +
> > >  drivers/iio/adc/ti-tla2024.c | 463 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 475 insertions(+)
> > >  create mode 100644 drivers/iio/adc/ti-tla2024.c
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 5762565..8c214c8 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -816,6 +816,17 @@ config TI_AM335X_ADC
> > >         To compile this driver as a module, choose M here: the module will be
> > >         called ti_am335x_adc.
> > >
> > > +config TI_TLA2024
> > > +     tristate "Texas Instruments TLA2024/TLA2022/TLA2021 ADC driver"
> > > +     depends on OF
> > > +     depends on I2C
> > > +     help
> > > +       Say yes here to build support for Texas Instruments TLA2024,
> > > +       TLA2022 or TLA2021 I2C Analog to Digital Converters.
> > > +
> > > +       To compile this driver as a module, choose M here: the
> > > +       module will be called ti-tla2024.
> > > +
> > >  config TI_TLC4541
> > >       tristate "Texas Instruments TLC4541 ADC driver"
> > >       depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 9874e05..819f35b 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -75,6 +75,7 @@ obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> > >  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> > >  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> > >  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> > > +obj-$(CONFIG_TI_TLA2024) += ti-tla2024.o
> > >  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> > >  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> > >  obj-$(CONFIG_VF610_ADC) += vf610_adc.o
> > > diff --git a/drivers/iio/adc/ti-tla2024.c b/drivers/iio/adc/ti-tla2024.c
> > > new file mode 100644
> > > index 0000000..e902eb8
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ti-tla2024.c
> > > @@ -0,0 +1,463 @@
> > > +/* SPDX-License-Identifier: GPL-2.0
> > > + *
> > > + * Texas Instruments TLA2021/TLA2022/TLA2024 12-bit ADC driver
> > > + *
> > > + * Copyright (C) 2019 Koninklijke Philips N.V.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +
> > > +#define TLA2024_DATA 0x00
> > > +#define TLA2024_DATA_RES_MASK GENMASK(15, 4)
> > > +#define TLA2024_DATA_RES_SHIFT 4
> > > +
> > > +#define TLA2024_CONF 0x01
> > > +#define TLA2024_CONF_OS_MASK BIT(15)
> > > +#define TLA2024_CONF_OS_SHIFT 15
> > > +#define TLA2024_CONF_MUX_MASK GENMASK(14, 12)
> > > +#define TLA2024_CONF_MUX_SHIFT 12
> > > +#define TLA2024_CONF_PGA_MASK GENMASK(11, 9)
> > > +#define TLA2024_CONF_PGA_SHIFT 9
> > > +#define TLA2024_CONF_MODE_MASK BIT(8)
> > > +#define TLA2024_CONF_MODE_SHIFT 8
> > > +#define TLA2024_CONF_DR_MASK GENMASK(7, 5)
> > > +#define TLA2024_CONF_DR_SHIFT 5
> > > +
> > > +#define TLA2024_CONV_RETRY 10
> > > +
> > > +struct tla202x_model {
> > > +     unsigned int mux_available;
> > > +     unsigned int pga_available;
> > > +};
> > > +
> > > +struct tla2024 {
> > > +     struct i2c_client *i2c;
> > > +     struct tla202x_model *model;
> > > +     struct mutex lock; /* protect channel selection */
> > > +     u8 used_mux_channels;
> > > +};
> > > +
> > > +struct tla2024_channel {
> > > +     int ainp;
> > > +     int ainn;
> > > +     const char *datasheet_name;
> > > +     bool differential;
> > > +};
> > > +
> > > +static const struct tla2024_channel tla2024_all_channels[] = {
> > > +     {0, 1, "AIN0-AIN1", 1},
> > > +     {0, 3, "AIN0-AIN3", 1},
> > > +     {1, 3, "AIN1-AIN3", 1},
> > > +     {2, 3, "AIN2-AIN3", 1},
> > > +     {0, -1, "AIN0-GND", 0},  
> >
> > Single ended, so normal convention would just be AIN0
> >  
> > > +     {1, -1, "AIN1-GND", 0},
> > > +     {2, -1, "AIN2-GND", 0},
> > > +     {3, -1, "AIN3-GND", 0},
> > > +};
> > > +
> > > +static int tla2024_find_chan_idx(int ainp_in, int ainn_in, u16 *idx)
> > > +{
> > > +     u16 i;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(tla2024_all_channels); i++) {
> > > +             if ((tla2024_all_channels[i].ainp == ainp_in) &&
> > > +                 (tla2024_all_channels[i].ainn == ainn_in)) {
> > > +                     *idx = i;
> > > +                     return 0;
> > > +             }
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +#define TLA202x_MODEL(_mux, _pga)            \
> > > +     {                                       \
> > > +             .mux_available = (_mux),        \
> > > +             .pga_available = (_pga),        \
> > > +     }
> > > +
> > > +enum tla2024_model_id {
> > > +     TLA2024 = 0,
> > > +     TLA2022 = 1,
> > > +     TLA2021 = 2,
> > > +};
> > > +
> > > +static struct tla202x_model tla202x_models[] = {
> > > +     TLA202x_MODEL(1, 1), // TLA2024  
> > [TLA2024] = TLA202x_MODEL(1, 1),
> >
> > Makes it clear which is which and avoids the ugly c++ comments.
> >  
> > > +     TLA202x_MODEL(0, 1), // TLA2022
> > > +     TLA202x_MODEL(0, 0), // TLA2021
> > > +};
> > > +
> > > +static const int tla2024_scale[] = { /* scale, int plus micro */
> > > +     3, 0, 2, 0, 1, 0, 0, 500000, 0, 250000, 0, 125000 };
> > > +
> > > +static int tla2024_get(struct tla2024 *priv, u8 addr, u16 mask,
> > > +                    u16 shift, u16 *val)
> > > +{
> > > +     int ret;
> > > +     u16 data;
> > > +
> > > +     ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     data = ret;
> > > +     *val = (mask & data) >> shift;  
> > I'm going to guess that your shift is always just that needed
> > to move the mask to 0?  As such we have the really neat
> > FIELD_GET macros to avoid the need to pass both shifts and
> > masks around (it computes the shift from the mask).
> >  
> 
> yes but my understanding was that FIELD_GET/FIELD_PREP
> require the "mask" to be a compile-time constant, so I could not
> use them in the somewhat generic get/set.

Ah good point.

oops.
> 
> I could still use them if I just remove the get/set. Then I could use
> i2c read/write and apply FIELD_GET/PREP on the output/input with
> a context-specific static mask. Is this the preferred way? Sorry if I
> am missing something completely...

Certainly there isn't a lot of point in the get function as it's
a very light weight wrapper.  Set however, is less clear so
just leave things as they are.

Sorry for the confusing comments.

Jonathan



> 
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int tla2024_set(struct tla2024 *priv, u8 addr, u16 mask,
> > > +                    u16 shift, u16 val)
> > > +{
> > > +     int ret;
> > > +     u16 data;
> > > +
> > > +     ret = i2c_smbus_read_word_swapped(priv->i2c, addr);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     data = ret;
> > > +     data &= ~mask;
> > > +     data |= mask & (val << shift);
> > > +
> > > +     ret = i2c_smbus_write_word_swapped(priv->i2c, addr, data);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int tla2024_read_avail(struct iio_dev *idev,
> > > +                           struct iio_chan_spec const *chan,
> > > +                           const int **vals, int *type, int *length,
> > > +                           long mask)
> > > +{
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +
> > > +             *length = ARRAY_SIZE(tla2024_scale);
> > > +             *vals = tla2024_scale;
> > > +
> > > +             *type = IIO_VAL_INT_PLUS_MICRO;
> > > +             return IIO_AVAIL_LIST;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +static int tla2024_of_find_chan(struct tla2024 *priv, struct device_node *ch)
> > > +{
> > > +     u16 chan_idx = 0;
> > > +     u32 tmp_p, tmp_n;
> > > +     int ainp, ainn;
> > > +     int ret;
> > > +
> > > +     ret = of_property_read_u32_index(ch, "single-channel", 0, &tmp_p);  
> >
> > This property isn't currently in the dt docs (I think...)
> > Hence the generic binding doc needs amending.
> >  
> > > +     if (ret) {
> > > +             ret = of_property_read_u32_index(ch,
> > > +                                              "diff-channels", 0, &tmp_p);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = of_property_read_u32_index(ch,
> > > +                                              "diff-channels", 1, &tmp_n);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ainp = (int)tmp_p;
> > > +             ainn = (int)tmp_n;
> > > +     } else {
> > > +             ainp = (int)tmp_p;
> > > +             ainn = -1;  
> >
> > Given this value is 'magic' anyway just use MAX_UINT instead and avoid
> > the need for negative handling?  That gets rid of the need for tmp_p, tmp_n
> > above.
> >  
> > > +     }
> > > +
> > > +     ret = tla2024_find_chan_idx(ainp, ainn, &chan_idx);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /* if model doesn"t have mux then only channel 0 is allowed */
> > > +     if (!priv->model->mux_available && chan_idx)
> > > +             return -EINVAL;
> > > +
> > > +     /* if already used */
> > > +     if ((priv->used_mux_channels) & (1 << chan_idx))
> > > +             return -EINVAL;
> > > +
> > > +     return chan_idx;
> > > +}
> > > +
> > > +static int tla2024_init_chan(struct iio_dev *idev, struct device_node *node,
> > > +                          struct iio_chan_spec *chan)
> > > +{
> > > +     struct tla2024 *priv = iio_priv(idev);
> > > +     u16 chan_idx;
> > > +     int ret;
> > > +
> > > +     ret = tla2024_of_find_chan(priv, node);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     chan_idx = ret;
> > > +     priv->used_mux_channels |= BIT(chan_idx);
> > > +     chan->type = IIO_VOLTAGE;
> > > +     chan->channel = tla2024_all_channels[chan_idx].ainp;
> > > +     chan->channel2 = tla2024_all_channels[chan_idx].ainn;
> > > +     chan->differential = tla2024_all_channels[chan_idx].differential;
> > > +     chan->extend_name = node->name;  
> > This is a bad idea.  You've just created a device specific userspace ABI
> > so all generic tools cannot be used.  What is the intended purpose of
> > doing this?
> >  
> > > +     chan->datasheet_name = tla2024_all_channels[chan_idx].datasheet_name;
> > > +     chan->indexed = 1;
> > > +     chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > +     chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE);
> > > +     chan->info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SCALE);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int tla2024_wait(struct tla2024 *priv)
> > > +{
> > > +     int ret;
> > > +     unsigned int retry = TLA2024_CONV_RETRY;
> > > +     u16 status;
> > > +
> > > +     do {
> > > +             if (!--retry)
> > > +                     return -EIO;
> > > +             ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> > > +                               TLA2024_CONF_OS_SHIFT, &status);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +             if (!status)
> > > +                     usleep_range(25, 1000);
> > > +     } while (!status);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int tla2024_select_channel(struct tla2024 *priv,
> > > +                               struct iio_chan_spec const *chan)
> > > +{
> > > +     int ret;
> > > +     int ainp = chan->channel;
> > > +     int ainn = chan->channel2;
> > > +     u16 chan_id = 0;
> > > +
> > > +     ret = tla2024_find_chan_idx(ainp, ainn, &chan_id);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MUX_MASK,
> > > +                                        TLA2024_CONF_MUX_SHIFT, chan_id);
> > > +}
> > > +
> > > +static int tla2024_convert(struct tla2024 *priv)
> > > +{
> > > +     int ret = 0;
> > > +
> > > +     ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_OS_MASK,
> > > +                       TLA2024_CONF_OS_SHIFT, 1);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return tla2024_wait(priv);
> > > +}
> > > +
> > > +static int tla2024_read_raw(struct iio_dev *idev,
> > > +                         struct iio_chan_spec const *channel, int *val,
> > > +                         int *val2, long mask)
> > > +{
> > > +     struct tla2024 *priv = iio_priv(idev);
> > > +     int ret;
> > > +     u16 data;
> > > +     s16 tmp;
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_RAW:
> > > +             mutex_lock(&priv->lock);
> > > +             ret = tla2024_select_channel(priv, channel);
> > > +             if (ret < 0)
> > > +                     goto return_unlock;
> > > +
> > > +             ret = tla2024_convert(priv);
> > > +             if (ret < 0)
> > > +                     goto return_unlock;
> > > +
> > > +             ret = tla2024_get(priv, TLA2024_DATA, TLA2024_DATA_RES_MASK,
> > > +                               TLA2024_DATA_RES_SHIFT, &data);
> > > +             if (ret < 0)
> > > +                     goto return_unlock;
> > > +
> > > +             tmp = (s16)(data << TLA2024_DATA_RES_SHIFT);
> > > +             *val = tmp >> TLA2024_DATA_RES_SHIFT;
> > > +             ret = IIO_VAL_INT;
> > > +  
> > The moment we get a deeply indented goto target like this it
> > always suggests to me that we should consider pulling the code
> > being protected by the lock out to a separate function, leaving
> > the lock external so you can have a simple
> >
> > lock
> > ret = functioncall
> > unlock
> > if (ret)... or just return ret;
> >  
> > > +return_unlock:
> > > +             mutex_unlock(&priv->lock);
> > > +             return ret;
> > > +
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             if (!(priv->model->pga_available)) {
> > > +                     *val = 1; /* Scale always 1 mV when no PGA */
> > > +                     return IIO_VAL_INT;
> > > +             }
> > > +             ret = tla2024_get(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> > > +                               TLA2024_CONF_PGA_SHIFT, &data);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             /* gain for the 3bit pga values 6 and 7 is same as at 5 */
> > > +             if (data >= (ARRAY_SIZE(tla2024_scale) >> 1))
> > > +                     data = (ARRAY_SIZE(tla2024_scale) >> 1) - 1;
> > > +
> > > +             *val = tla2024_scale[data << 1];
> > > +             *val2 = tla2024_scale[(data << 1) + 1];
> > > +             return IIO_VAL_INT_PLUS_MICRO;
> > > +
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int tla2024_write_raw(struct iio_dev *idev,
> > > +                          struct iio_chan_spec const *chan,
> > > +                          int val, int val2, long mask)
> > > +{
> > > +     struct tla2024 *priv = iio_priv(idev);
> > > +     int i;
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             if (!(priv->model->pga_available))
> > > +                     return -EINVAL; /* scale can't be changed if no pga */
> > > +
> > > +             for (i = 0; i < ARRAY_SIZE(tla2024_scale); i = i + 2) {
> > > +                     if ((tla2024_scale[i] == val) &&
> > > +                         (tla2024_scale[i + 1] == val2))
> > > +                             break;
> > > +             }
> > > +
> > > +             if (i == ARRAY_SIZE(tla2024_scale))
> > > +                     return -EINVAL;
> > > +  
> >
> > This need a lock I think...
> >  
> > > +             return tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_PGA_MASK,
> > > ++                                    TLA2024_CONF_PGA_SHIFT, i >> 1);
> > > +
> > > +     default:
> > > +             break;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +static int tla2024_of_chan_init(struct iio_dev *idev)
> > > +{
> > > +     struct device_node *node = idev->dev.of_node;
> > > +     struct device_node *child;
> > > +     struct iio_chan_spec *channels;
> > > +     int ret, i, num_channels;  
> >  
> > > +
> > > +     num_channels = of_get_available_child_count(node);
> > > +     if (!num_channels) {
> > > +             dev_err(&idev->dev, "no channels configured\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     channels = devm_kcalloc(&idev->dev, num_channels,
> > > +                             sizeof(struct iio_chan_spec), GFP_KERNEL);
> > > +     if (!channels)
> > > +             return -ENOMEM;
> > > +
> > > +     i = 0;
> > > +     for_each_available_child_of_node(node, child) {
> > > +             ret = tla2024_init_chan(idev, child, &channels[i]);
> > > +             if (ret) {
> > > +                     of_node_put(child);
> > > +                     return ret;
> > > +             }
> > > +             i++;
> > > +     }
> > > +
> > > +     idev->channels = channels;
> > > +     idev->num_channels = num_channels;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct iio_info tla2024_info = {
> > > +     .read_raw = tla2024_read_raw,
> > > +     .write_raw = tla2024_write_raw,
> > > +     .read_avail = tla2024_read_avail,
> > > +};
> > > +
> > > +static int tla2024_probe(struct i2c_client *client,
> > > +                      const struct i2c_device_id *id)
> > > +{
> > > +     struct iio_dev *iio;
> > > +     struct tla2024 *priv;
> > > +     struct tla202x_model *model;
> > > +     int ret;
> > > +
> > > +     if (!i2c_check_functionality(client->adapter,
> > > +                                  I2C_FUNC_SMBUS_WORD_DATA))
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     model = &tla202x_models[id->driver_data];  
> >
> > What is the point in this local variable?  I would just
> > assign this directly below.
> >  
> > > +
> > > +     iio = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> > > +     if (!iio)
> > > +             return -ENOMEM;
> > > +
> > > +     priv = iio_priv(iio);
> > > +     priv->i2c = client;
> > > +     priv->model = model;
> > > +     mutex_init(&priv->lock);
> > > +
> > > +     iio->dev.parent = &client->dev;
> > > +     iio->dev.of_node = client->dev.of_node;
> > > +     iio->name = client->name;
> > > +     iio->modes = INDIO_DIRECT_MODE;
> > > +     iio->info = &tla2024_info;
> > > +
> > > +     ret = tla2024_of_chan_init(iio);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     ret = tla2024_set(priv, TLA2024_CONF, TLA2024_CONF_MODE_MASK,
> > > +                       TLA2024_CONF_MODE_SHIFT, 1);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return devm_iio_device_register(&client->dev, iio);
> > > +}
> > > +
> > > +static const struct i2c_device_id tla2024_id[] = {
> > > +     { "tla2024", TLA2024 },
> > > +     { "tla2022", TLA2022 },
> > > +     { "tla2021", TLA2021 },  
> >
> > Really minor point, but reverse numerical order is 'unusual'..
> >  
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, tla2024_id);
> > > +
> > > +static const struct of_device_id tla2024_of_match[] = {
> > > +     { .compatible = "ti,tla2024" },
> > > +     { .compatible = "ti,tla2022" },
> > > +     { .compatible = "ti,tla2021" },
> > > +     { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, tla2024_of_match);
> > > +
> > > +static struct i2c_driver tla2024_driver = {
> > > +     .driver = {
> > > +             .name = "tla2024",
> > > +             .of_match_table = of_match_ptr(tla2024_of_match),
> > > +     },
> > > +     .probe = tla2024_probe,
> > > +     .id_table = tla2024_id,
> > > +};
> > > +module_i2c_driver(tla2024_driver);
> > > +
> > > +MODULE_AUTHOR("Ibtsam Haq <ibtsam.haq@philips.com>");
> > > +MODULE_DESCRIPTION("Texas Instruments TLA2021/TLA2022/TLA2024 ADC driver");
> > > +MODULE_LICENSE("GPL v2");  
> >