diff mbox series

[v6,5/5] iio: adc: ad7192: Add AD7194 support

Message ID 20240417170054.140587-6-alisa.roman@analog.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7192: Add AD7194 support | expand

Commit Message

Alisa-Dariana Roman April 17, 2024, 5 p.m. UTC 
Unlike the other AD719Xs, AD7194 has configurable differential
channels. The user can dynamically configure them in the devicetree.

Also modify config AD7192 description for better scaling.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/Kconfig  |  11 +++-
 drivers/iio/adc/ad7192.c | 122 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 126 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko April 17, 2024, 5:06 p.m. UTC | #1 
On Wed, Apr 17, 2024 at 8:01 PM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The user can dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.

...

> +       device_for_each_child_node(dev, child) {

You can use scoped variant AFAIU that's available in Jonathan's tree.

> +               *ad7194_channels = ad7194_chan_diff;
> +               ad7194_channels->scan_index = index++;
> +               ret = ad7194_parse_channel(child, ad7194_channels);
> +               if (ret) {

> +                       fwnode_handle_put(child);

With the above this wouldn't be needed.

> +                       return ret;
> +               }
> +               ad7194_channels++;
> +       }
Jonathan Cameron April 20, 2024, 10:55 a.m. UTC | #2 
On Wed, 17 Apr 2024 20:06:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 17, 2024 at 8:01 PM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> >
> > Unlike the other AD719Xs, AD7194 has configurable differential
> > channels. The user can dynamically configure them in the devicetree.
> >
> > Also modify config AD7192 description for better scaling.  
> 
> ...
> 
> > +       device_for_each_child_node(dev, child) {  
> 
> You can use scoped variant AFAIU that's available in Jonathan's tree.
> 
> > +               *ad7194_channels = ad7194_chan_diff;
> > +               ad7194_channels->scan_index = index++;
> > +               ret = ad7194_parse_channel(child, ad7194_channels);
> > +               if (ret) {  
> 
> > +                       fwnode_handle_put(child);  
> 
> With the above this wouldn't be needed.
> 
As it's a minor improvement, and I tend not to like unnecessary
interdependence of series in my tree (until they are in char-misc
and hence no chance of them changing), I'm fine with not using
that new functionality here. 

That will change once it's upstream of course!

I will send a pull request to Greg nice and early this cycle
so that should be in my upstream soon.

I'm also fine with you using this if you want to though!

Jonathan

> > +                       return ret;
> > +               }
> > +               ad7194_channels++;
> > +       }  
>
Jonathan Cameron April 20, 2024, 11:02 a.m. UTC | #3 
On Wed, 17 Apr 2024 20:00:54 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The user can dynamically configure them in the devicetree.
> 
> Also modify config AD7192 description for better scaling.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Late feedback (sorry!) but I think we should resolve the single ended
channel description so that it sits at the same level in DT binding
as do differential channels.  Current situation just feels inconsistent.

See DT binding reply.  Should be easy to do now, and wouldn't be possible
to add later.  It would be possible to add support for moving to
per channel DT entries for a driver that previously assumed all should be
available, but we can't easily move from assuming all single ended channels
are but differential are specified by DT child nodes.

Jonathan

> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8d56cf889973..dc113405f1bc 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@


> +static int ad7194_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *ad7194_channels;
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
> +	struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
> +	struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
> +	struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
> +	unsigned int num_channels, index = 0, ain_chan;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD7194_CH_DIFF_NR_MAX)
> +		return -EINVAL;
> +
> +	num_channels += AD7194_CH_BASE_NR;
> +
> +	ad7194_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad7194_channels), GFP_KERNEL);
> +	if (!ad7194_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad7194_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node(dev, child) {
> +		*ad7194_channels = ad7194_chan_diff;
> +		ad7194_channels->scan_index = index++;
> +		ret = ad7194_parse_channel(child, ad7194_channels);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +		ad7194_channels++;
> +	}
> +
> +	*ad7194_channels = ad7194_chan_temp;
> +	ad7194_channels->scan_index = index++;
> +	ad7194_channels->address = AD7194_CH_TEMP;
> +	ad7194_channels++;
> +
> +	for (ain_chan = 1; ain_chan <= 16; ain_chan++) {

I think it's worth making these more similar to the differential channels.
Seems odd to allow DT to provide that list, but not the same for single ended.
See comment on binding.  Should be fairly easy to add now, and if we
leave it until later, there won't be a way to move this forwards because
we won't be able to tell if no single ended channels in DT means none
are relevant, or if it means the DT file predates us adding per single ended
channel description.


> +		*ad7194_channels = ad7194_chan;
> +		ad7194_channels->scan_index = index++;
> +		ad7194_channels->channel = ain_chan;
> +		ad7194_channels->address = AD7194_CH_DIFF(ain_chan, 0);
> +		ad7194_channels++;
> +	}
> +
> +	*ad7194_channels = ad7194_chan_timestamp;
> +	ad7194_channels->scan_index = index;
> +
> +	return 0;
> +}
diff mbox series

Patch

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..74fecc284f1a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -88,12 +88,17 @@  config AD7173
 	  called ad7173.
 
 config AD7192
-	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+	tristate "Analog Devices AD7192 and similar ADC driver"
 	depends on SPI
 	select AD_SIGMA_DELTA
 	help
-	  Say yes here to build support for Analog Devices AD7190,
-	  AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+	  Say yes here to build support for Analog Devices SPI analog to digital
+	  converters (ADC):
+	  - AD7190
+	  - AD7192
+	  - AD7193
+	  - AD7194
+	  - AD7195
 	  If unsure, say N (but it's safe to say "Y").
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 8d56cf889973..dc113405f1bc 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7192 and similar SPI ADC driver
  *
  * Copyright 2011-2015 Analog Devices Inc.
  */
@@ -128,10 +128,21 @@ 
 #define AD7193_CH_AIN8		0x480 /* AIN7 - AINCOM */
 #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
 
+#define AD7194_CH_POS(x)	(((x) - 1) << 4)
+#define AD7194_CH_NEG(x)	((x) - 1)
+#define AD7194_CH_DIFF(pos, neg) \
+		(((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))
+#define AD7194_CH_TEMP		0x100 /* Temp sensor */
+#define AD7194_CH_BASE_NR	18
+#define AD7194_CH_AIN_START	1
+#define AD7194_CH_AIN_NR	16
+#define AD7194_CH_DIFF_NR_MAX	256
+
 /* ID Register Bit Designations (AD7192_REG_ID) */
 #define CHIPID_AD7190		0x4
 #define CHIPID_AD7192		0x0
 #define CHIPID_AD7193		0x2
+#define CHIPID_AD7194		0x3
 #define CHIPID_AD7195		0x6
 #define AD7192_ID_MASK		GENMASK(3, 0)
 
@@ -169,6 +180,7 @@  enum {
 	ID_AD7190,
 	ID_AD7192,
 	ID_AD7193,
+	ID_AD7194,
 	ID_AD7195,
 };
 
@@ -178,6 +190,7 @@  struct ad7192_chip_info {
 	const struct iio_chan_spec	*channels;
 	u8				num_channels;
 	const struct iio_info		*info;
+	int (*parse_channels)(struct iio_dev *indio_dev);
 };
 
 struct ad7192_state {
@@ -931,6 +944,15 @@  static const struct iio_info ad7192_info = {
 	.update_scan_mode = ad7192_update_scan_mode,
 };
 
+static const struct iio_info ad7194_info = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
+};
+
 static const struct iio_info ad7195_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -1022,6 +1044,84 @@  static const struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static int ad7194_parse_channel(struct fwnode_handle *child,
+				struct iio_chan_spec *ad7194_channel)
+{
+	u32 ain[2];
+	int ret;
+
+	ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
+					     ARRAY_SIZE(ain));
+	if (ret)
+		return ret;
+
+	if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
+	    !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
+		return -EINVAL;
+
+	ad7194_channel->channel = ain[0];
+	ad7194_channel->channel2 = ain[1];
+	ad7194_channel->address = AD7194_CH_DIFF(ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7194_parse_channels(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *ad7194_channels;
+	struct fwnode_handle *child;
+	struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
+	struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
+	struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
+	struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
+	unsigned int num_channels, index = 0, ain_chan;
+	int ret;
+
+	num_channels = device_get_child_node_count(dev);
+	if (num_channels > AD7194_CH_DIFF_NR_MAX)
+		return -EINVAL;
+
+	num_channels += AD7194_CH_BASE_NR;
+
+	ad7194_channels = devm_kcalloc(dev, num_channels,
+				       sizeof(*ad7194_channels), GFP_KERNEL);
+	if (!ad7194_channels)
+		return -ENOMEM;
+
+	indio_dev->channels = ad7194_channels;
+	indio_dev->num_channels = num_channels;
+
+	device_for_each_child_node(dev, child) {
+		*ad7194_channels = ad7194_chan_diff;
+		ad7194_channels->scan_index = index++;
+		ret = ad7194_parse_channel(child, ad7194_channels);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+		ad7194_channels++;
+	}
+
+	*ad7194_channels = ad7194_chan_temp;
+	ad7194_channels->scan_index = index++;
+	ad7194_channels->address = AD7194_CH_TEMP;
+	ad7194_channels++;
+
+	for (ain_chan = 1; ain_chan <= 16; ain_chan++) {
+		*ad7194_channels = ad7194_chan;
+		ad7194_channels->scan_index = index++;
+		ad7194_channels->channel = ain_chan;
+		ad7194_channels->address = AD7194_CH_DIFF(ain_chan, 0);
+		ad7194_channels++;
+	}
+
+	*ad7194_channels = ad7194_chan_timestamp;
+	ad7194_channels->scan_index = index;
+
+	return 0;
+}
+
 static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	[ID_AD7190] = {
 		.chip_id = CHIPID_AD7190,
@@ -1044,6 +1144,12 @@  static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7193_channels),
 		.info = &ad7192_info,
 	},
+	[ID_AD7194] = {
+		.chip_id = CHIPID_AD7194,
+		.name = "ad7194",
+		.info = &ad7194_info,
+		.parse_channels = ad7194_parse_channels,
+	},
 	[ID_AD7195] = {
 		.chip_id = CHIPID_AD7195,
 		.name = "ad7195",
@@ -1148,9 +1254,15 @@  static int ad7192_probe(struct spi_device *spi)
 
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = st->chip_info->channels;
-	indio_dev->num_channels = st->chip_info->num_channels;
 	indio_dev->info = st->chip_info->info;
+	if (st->chip_info->parse_channels) {
+		ret = st->chip_info->parse_channels(indio_dev);
+		if (ret)
+			return ret;
+	} else {
+		indio_dev->channels = st->chip_info->channels;
+		indio_dev->num_channels = st->chip_info->num_channels;
+	}
 
 	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);
 	if (ret)
@@ -1189,6 +1301,7 @@  static const struct of_device_id ad7192_of_match[] = {
 	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
 	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
 	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+	{ .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
 	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1198,6 +1311,7 @@  static const struct spi_device_id ad7192_ids[] = {
 	{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
 	{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
 	{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
+	{ "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
 	{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1214,6 +1328,6 @@  static struct spi_driver ad7192_driver = {
 module_spi_driver(ad7192_driver);
 
 MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
+MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);