On the parallel version, the current implementation is only compatible
with id tables and won't work with fw_nodes, this commit intends to fix
it.

Also, chip info is moved in the .h file so to be accessible to all the
driver files that can set a pointer to the corresponding chip as the
driver data.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 208 ++++++++++++++++++++++++-------------------
 drivers/iio/adc/ad7606.h     |  34 +++++--
 drivers/iio/adc/ad7606_par.c |  29 +++---
 drivers/iio/adc/ad7606_spi.c |  31 ++++---
 4 files changed, 173 insertions(+), 129 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index b98057138295..7f2ff1674638 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -51,6 +51,116 @@ static const unsigned int ad7616_oversampling_avail[8] = {
 	1, 2, 4, 8, 16, 32, 64, 128,
 };
 
+static const struct iio_chan_spec ad7605_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+	AD7605_CHANNEL(0),
+	AD7605_CHANNEL(1),
+	AD7605_CHANNEL(2),
+	AD7605_CHANNEL(3),
+};
+
+static const struct iio_chan_spec ad7606_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_CHANNEL(0),
+	AD7606_CHANNEL(1),
+	AD7606_CHANNEL(2),
+	AD7606_CHANNEL(3),
+	AD7606_CHANNEL(4),
+	AD7606_CHANNEL(5),
+	AD7606_CHANNEL(6),
+	AD7606_CHANNEL(7),
+};
+
+/*
+ * The current assumption that this driver makes for AD7616, is that it's
+ * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
+ * To activate them, following pins must be pulled high:
+ *	-SER/PAR
+ *	-SEQEN
+ * And following pins must be pulled low:
+ *	-WR/BURST
+ *	-DB4/SER1W
+ */
+static const struct iio_chan_spec ad7616_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(16),
+	AD7606_CHANNEL(0),
+	AD7606_CHANNEL(1),
+	AD7606_CHANNEL(2),
+	AD7606_CHANNEL(3),
+	AD7606_CHANNEL(4),
+	AD7606_CHANNEL(5),
+	AD7606_CHANNEL(6),
+	AD7606_CHANNEL(7),
+	AD7606_CHANNEL(8),
+	AD7606_CHANNEL(9),
+	AD7606_CHANNEL(10),
+	AD7606_CHANNEL(11),
+	AD7606_CHANNEL(12),
+	AD7606_CHANNEL(13),
+	AD7606_CHANNEL(14),
+	AD7606_CHANNEL(15),
+};
+
+const struct ad7606_chip_info ad7605_4_info = {
+	.channels = ad7605_channels,
+	.num_channels = 5,
+	.name = "ad7605-4",
+	.id = ID_AD7605_4,
+};
+EXPORT_SYMBOL_NS_GPL(ad7605_4_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7606_8_info = {
+	.channels = ad7606_channels,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-8",
+	.id = ID_AD7606_8,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_8_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7606_6_info = {
+	.channels = ad7606_channels,
+	.num_channels = 7,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-6",
+	.id = ID_AD7606_6,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_6_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7606_4_info = {
+	.channels = ad7606_channels,
+	.num_channels = 5,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-4",
+	.id = ID_AD7606_4,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_4_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7606b_info = {
+	.channels = ad7606_channels,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606B",
+	.id = ID_AD7606B,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606b_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7616_info = {
+	.channels = ad7616_channels,
+	.num_channels = 17,
+	.oversampling_avail = ad7616_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7616_oversampling_avail),
+	.os_req_reset = true,
+	.init_delay_ms = 15,
+	.name = "ad7616",
+	.id = ID_AD7616,
+};
+EXPORT_SYMBOL_NS_GPL(ad7616_info, IIO_AD7606);
+
 int ad7606_reset(struct ad7606_state *st)
 {
 	if (st->gpio_reset) {
@@ -395,96 +505,6 @@ static const struct attribute_group ad7606_attribute_group_range = {
 	.attrs = ad7606_attributes_range,
 };
 
-static const struct iio_chan_spec ad7605_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(4),
-	AD7605_CHANNEL(0),
-	AD7605_CHANNEL(1),
-	AD7605_CHANNEL(2),
-	AD7605_CHANNEL(3),
-};
-
-static const struct iio_chan_spec ad7606_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(8),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-};
-
-/*
- * The current assumption that this driver makes for AD7616, is that it's
- * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
- * To activate them, following pins must be pulled high:
- *	-SER/PAR
- *	-SEQEN
- * And following pins must be pulled low:
- *	-WR/BURST
- *	-DB4/SER1W
- */
-static const struct iio_chan_spec ad7616_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(16),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-	AD7606_CHANNEL(8),
-	AD7606_CHANNEL(9),
-	AD7606_CHANNEL(10),
-	AD7606_CHANNEL(11),
-	AD7606_CHANNEL(12),
-	AD7606_CHANNEL(13),
-	AD7606_CHANNEL(14),
-	AD7606_CHANNEL(15),
-};
-
-static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
-	/* More devices added in future */
-	[ID_AD7605_4] = {
-		.channels = ad7605_channels,
-		.num_channels = 5,
-	},
-	[ID_AD7606_8] = {
-		.channels = ad7606_channels,
-		.num_channels = 9,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606_6] = {
-		.channels = ad7606_channels,
-		.num_channels = 7,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606_4] = {
-		.channels = ad7606_channels,
-		.num_channels = 5,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606B] = {
-		.channels = ad7606_channels,
-		.num_channels = 9,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7616] = {
-		.channels = ad7616_channels,
-		.num_channels = 17,
-		.oversampling_avail = ad7616_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7616_oversampling_avail),
-		.os_req_reset = true,
-		.init_delay_ms = 15,
-	},
-};
-
 static int ad7606_request_gpios(struct ad7606_state *st)
 {
 	struct device *dev = st->dev;
@@ -624,7 +644,7 @@ static void ad7606_pwm_disable(void *data)
 }
 
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
+		 const struct ad7606_chip_info *chip_info,
 		 const struct ad7606_bus_ops *bops)
 {
 	struct ad7606_state *st;
@@ -653,7 +673,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		return dev_err_probe(dev, ret,
 				     "Failed to enable specified AVcc supply\n");
 
-	st->chip_info = &ad7606_chip_info_tbl[id];
+	st->chip_info = chip_info;
 
 	if (st->chip_info->oversampling_num) {
 		st->oversampling_avail = st->chip_info->oversampling_avail;
@@ -676,7 +696,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 			indio_dev->info = &ad7606_info_no_os_or_range;
 	}
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->name = name;
+	indio_dev->name = chip_info->name;
 	indio_dev->channels = st->chip_info->channels;
 	indio_dev->num_channels = st->chip_info->num_channels;
 
@@ -758,7 +778,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 					NULL,
 					&ad7606_interrupt,
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					name, indio_dev);
+					chip_info->name, indio_dev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index c13dda444526..18c87fe9a41a 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -38,8 +38,19 @@
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
 		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
 
+enum ad7606_supported_device_ids {
+	ID_AD7605_4,
+	ID_AD7606_8,
+	ID_AD7606_6,
+	ID_AD7606_4,
+	ID_AD7606B,
+	ID_AD7616,
+};
+
 /**
  * struct ad7606_chip_info - chip specific information
+ * @name		device name
+ * @id			device id
  * @channels:		channel specification
  * @num_channels:	number of channels
  * @oversampling_avail	pointer to the array which stores the available
@@ -50,6 +61,8 @@
  *			after a restart
  */
 struct ad7606_chip_info {
+	enum ad7606_supported_device_ids id;
+	const char			*name;
 	const struct iio_chan_spec	*channels;
 	unsigned int			num_channels;
 	const unsigned int		*oversampling_avail;
@@ -150,19 +163,22 @@ struct ad7606_bus_ops {
 };
 
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
+		 const struct ad7606_chip_info *info,
 		 const struct ad7606_bus_ops *bops);
 
 int ad7606_reset(struct ad7606_state *st);
 
-enum ad7606_supported_device_ids {
-	ID_AD7605_4,
-	ID_AD7606_8,
-	ID_AD7606_6,
-	ID_AD7606_4,
-	ID_AD7606B,
-	ID_AD7616,
-};
+extern const struct ad7606_chip_info ad7605_4_info;
+
+extern const struct ad7606_chip_info ad7606_8_info;
+
+extern const struct ad7606_chip_info ad7606_6_info;
+
+extern const struct ad7606_chip_info ad7606_4_info;
+
+extern const struct ad7606_chip_info ad7606b_info;
+
+extern const struct ad7606_chip_info ad7616_info;
 
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops ad7606_pm_ops;
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index d651639c45eb..7bac39033955 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -11,6 +11,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/types.h>
 
 #include <linux/iio/iio.h>
@@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
 
 static int ad7606_par_probe(struct platform_device *pdev)
 {
-	const struct platform_device_id *id = platform_get_device_id(pdev);
+	const struct ad7606_chip_info *chip_info;
+	const struct platform_device_id *id;
 	struct resource *res;
 	void __iomem *addr;
 	resource_size_t remap_size;
 	int irq;
 
+	if (dev_fwnode(&pdev->dev)) {
+		chip_info = device_get_match_data(&pdev->dev);
+	} else {
+		id = platform_get_device_id(pdev);
+		chip_info = (const struct ad7606_chip_info *)id->driver_data;
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
 	remap_size = resource_size(res);
 
 	return ad7606_probe(&pdev->dev, irq, addr,
-			    id->name, id->driver_data,
+			    chip_info,
 			    remap_size > 1 ? &ad7606_par16_bops :
 			    &ad7606_par8_bops);
 }
 
 static const struct platform_device_id ad7606_driver_ids[] = {
-	{ .name	= "ad7605-4", .driver_data = ID_AD7605_4, },
-	{ .name	= "ad7606-4", .driver_data = ID_AD7606_4, },
-	{ .name	= "ad7606-6", .driver_data = ID_AD7606_6, },
-	{ .name	= "ad7606-8", .driver_data = ID_AD7606_8, },
+	{ .name	= "ad7605-4", .driver_data = (kernel_ulong_t)&ad7605_4_info, },
+	{ .name	= "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, },
+	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
+	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
 
 static const struct of_device_id ad7606_of_match[] = {
-	{ .compatible = "adi,ad7605-4" },
-	{ .compatible = "adi,ad7606-4" },
-	{ .compatible = "adi,ad7606-6" },
-	{ .compatible = "adi,ad7606-8" },
+	{ .compatible = "adi,ad7605-4", .data = &ad7605_4_info },
+	{ .compatible = "adi,ad7606-4", .data = &ad7606_4_info },
+	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
+	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index d3fbc7c7a823..719f9e09a5f7 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -307,10 +307,10 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
 
 static int ad7606_spi_probe(struct spi_device *spi)
 {
-	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct ad7606_chip_info *chip_info = spi_get_device_match_data(spi);
 	const struct ad7606_bus_ops *bops;
 
-	switch (id->driver_data) {
+	switch (chip_info->id) {
 	case ID_AD7616:
 		bops = &ad7616_spi_bops;
 		break;
@@ -323,28 +323,27 @@ static int ad7606_spi_probe(struct spi_device *spi)
 	}
 
 	return ad7606_probe(&spi->dev, spi->irq, NULL,
-			    id->name, id->driver_data,
-			    bops);
+			    chip_info, bops);
 }
 
 static const struct spi_device_id ad7606_id_table[] = {
-	{ "ad7605-4", ID_AD7605_4 },
-	{ "ad7606-4", ID_AD7606_4 },
-	{ "ad7606-6", ID_AD7606_6 },
-	{ "ad7606-8", ID_AD7606_8 },
-	{ "ad7606b",  ID_AD7606B },
-	{ "ad7616",   ID_AD7616 },
+	{ "ad7605-4", (kernel_ulong_t)&ad7605_4_info },
+	{ "ad7606-4", (kernel_ulong_t)&ad7606_4_info },
+	{ "ad7606-6", (kernel_ulong_t)&ad7606_6_info },
+	{ "ad7606-8", (kernel_ulong_t)&ad7606_8_info },
+	{ "ad7606b",  (kernel_ulong_t)&ad7606b_info },
+	{ "ad7616",   (kernel_ulong_t)&ad7616_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad7606_id_table);
 
 static const struct of_device_id ad7606_of_match[] = {
-	{ .compatible = "adi,ad7605-4" },
-	{ .compatible = "adi,ad7606-4" },
-	{ .compatible = "adi,ad7606-6" },
-	{ .compatible = "adi,ad7606-8" },
-	{ .compatible = "adi,ad7606b" },
-	{ .compatible = "adi,ad7616" },
+	{ .compatible = "adi,ad7605-4", &ad7605_4_info },
+	{ .compatible = "adi,ad7606-4", &ad7606_4_info },
+	{ .compatible = "adi,ad7606-6", &ad7606_6_info },
+	{ .compatible = "adi,ad7606-8", &ad7606_8_info },
+	{ .compatible = "adi,ad7606b", &ad7606b_info },
+	{ .compatible = "adi,ad7616", &ad7616_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);

-- 
2.34.1

On Fri, 20 Sep 2024 17:33:27 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> On the parallel version, the current implementation is only compatible
> with id tables and won't work with fw_nodes, this commit intends to fix
> it.
> 
> Also, chip info is moved in the .h file so to be accessible to all the

chip info is not moved (I was going to say no to that) but an
extern is used to make it available. So say that rather than moved here.

> driver files that can set a pointer to the corresponding chip as the
> driver data.
> 

>  
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index c13dda444526..18c87fe9a41a 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -38,8 +38,19 @@
>  	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>  		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>  
> +enum ad7606_supported_device_ids {
> +	ID_AD7605_4,
> +	ID_AD7606_8,
> +	ID_AD7606_6,
> +	ID_AD7606_4,
> +	ID_AD7606B,
> +	ID_AD7616,
> +};
> +
>  /**
>   * struct ad7606_chip_info - chip specific information
> + * @name		device name
> + * @id			device id

ID in chip info normally indicates something bad in the design. In that somewhere
we have code that is ID dependent rather than all such code / data being
found directly in this structure (or callbacks found from here).
Can we avoid it here?

>   * @channels:		channel specification
>   * @num_channels:	number of channels
>   * @oversampling_avail	pointer to the array which stores the available
> @@ -50,6 +61,8 @@

...

> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index d651639c45eb..7bac39033955 100644
> --- a/drivers/iio/adc/ad7606_par.c
> +++ b/drivers/iio/adc/ad7606_par.c
> @@ -11,6 +11,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/types.h>
>  
>  #include <linux/iio/iio.h>
> @@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
>  
>  static int ad7606_par_probe(struct platform_device *pdev)
>  {
> -	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	const struct ad7606_chip_info *chip_info;
> +	const struct platform_device_id *id;
>  	struct resource *res;
>  	void __iomem *addr;
>  	resource_size_t remap_size;
>  	int irq;
>  
> +	if (dev_fwnode(&pdev->dev)) {
> +		chip_info = device_get_match_data(&pdev->dev);
> +	} else {
> +		id = platform_get_device_id(pdev);
> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
> +	}
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
>  	remap_size = resource_size(res);
>  
>  	return ad7606_probe(&pdev->dev, irq, addr,
> -			    id->name, id->driver_data,
Rewrap to move chip_info up a line perhaps.

> +			    chip_info,
>  			    remap_size > 1 ? &ad7606_par16_bops :
>  			    &ad7606_par8_bops);

On 9/29/24 14:44, Jonathan Cameron wrote:
> On Fri, 20 Sep 2024 17:33:27 +0000
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> On the parallel version, the current implementation is only compatible
>> with id tables and won't work with fw_nodes, this commit intends to fix
>> it.
>>
>> Also, chip info is moved in the .h file so to be accessible to all the
> chip info is not moved (I was going to say no to that) but an
> extern is used to make it available. So say that rather than moved here.
>
>> driver files that can set a pointer to the corresponding chip as the
>> driver data.
>>
>>   
>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>> index c13dda444526..18c87fe9a41a 100644
>> --- a/drivers/iio/adc/ad7606.h
>> +++ b/drivers/iio/adc/ad7606.h
>> @@ -38,8 +38,19 @@
>>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>   
>> +enum ad7606_supported_device_ids {
>> +	ID_AD7605_4,
>> +	ID_AD7606_8,
>> +	ID_AD7606_6,
>> +	ID_AD7606_4,
>> +	ID_AD7606B,
>> +	ID_AD7616,
>> +};
>> +
>>   /**
>>    * struct ad7606_chip_info - chip specific information
>> + * @name		device name
>> + * @id			device id
> ID in chip info normally indicates something bad in the design. In that somewhere
> we have code that is ID dependent rather than all such code / data being
> found directly in this structure (or callbacks found from here).
> Can we avoid it here?

Hi Jonathan,

chip_info has to describe the chip hardwarewise, but there are different 
bops depending on the wiring (interface used, and backend/no backend).

The easiest way I found was to use the ID in a switch/case to 
determinate which bops I should take (well it was only needed in the spi 
version since it is the one supporting almost all the chips while the 
other ones still support only one). For instance, the ad7606B will use 
ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.

If I can't use the ID, the only way I see is creating 3 fields in 
chip_info (spi_ops, par_ops, backend_ops) and to initialize every 
chip_info structure with its associated op(s) for the associated 
interface. This would also lead to declare the different instances of 
ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see 
no other option).

Do you think it's better that way ? Or do you have any other idea ?

Regards,

Guillaume

>
>>    * @channels:		channel specification
>>    * @num_channels:	number of channels
>>    * @oversampling_avail	pointer to the array which stores the available
>> @@ -50,6 +61,8 @@
> ...
>
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index d651639c45eb..7bac39033955 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/property.h>
>>   #include <linux/types.h>
>>   
>>   #include <linux/iio/iio.h>
>> @@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
>>   
>>   static int ad7606_par_probe(struct platform_device *pdev)
>>   {
>> -	const struct platform_device_id *id = platform_get_device_id(pdev);
>> +	const struct ad7606_chip_info *chip_info;
>> +	const struct platform_device_id *id;
>>   	struct resource *res;
>>   	void __iomem *addr;
>>   	resource_size_t remap_size;
>>   	int irq;
>>   
>> +	if (dev_fwnode(&pdev->dev)) {
>> +		chip_info = device_get_match_data(&pdev->dev);
>> +	} else {
>> +		id = platform_get_device_id(pdev);
>> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
>> +	}
>> +
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
>>   	remap_size = resource_size(res);
>>   
>>   	return ad7606_probe(&pdev->dev, irq, addr,
>> -			    id->name, id->driver_data,
> Rewrap to move chip_info up a line perhaps.
>
>> +			    chip_info,
>>   			    remap_size > 1 ? &ad7606_par16_bops :
>>   			    &ad7606_par8_bops);

On Wed, 2 Oct 2024 02:12:28 +0200
Guillaume Stols <gstols@baylibre.com> wrote:

> On 9/29/24 14:44, Jonathan Cameron wrote:
> > On Fri, 20 Sep 2024 17:33:27 +0000
> > Guillaume Stols <gstols@baylibre.com> wrote:
> >  
> >> On the parallel version, the current implementation is only compatible
> >> with id tables and won't work with fw_nodes, this commit intends to fix
> >> it.
> >>
> >> Also, chip info is moved in the .h file so to be accessible to all the  
> > chip info is not moved (I was going to say no to that) but an
> > extern is used to make it available. So say that rather than moved here.
> >  
> >> driver files that can set a pointer to the corresponding chip as the
> >> driver data.
> >>
> >>   
> >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> >> index c13dda444526..18c87fe9a41a 100644
> >> --- a/drivers/iio/adc/ad7606.h
> >> +++ b/drivers/iio/adc/ad7606.h
> >> @@ -38,8 +38,19 @@
> >>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> >>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> >>   
> >> +enum ad7606_supported_device_ids {
> >> +	ID_AD7605_4,
> >> +	ID_AD7606_8,
> >> +	ID_AD7606_6,
> >> +	ID_AD7606_4,
> >> +	ID_AD7606B,
> >> +	ID_AD7616,
> >> +};
> >> +
> >>   /**
> >>    * struct ad7606_chip_info - chip specific information
> >> + * @name		device name
> >> + * @id			device id  
> > ID in chip info normally indicates something bad in the design. In that somewhere
> > we have code that is ID dependent rather than all such code / data being
> > found directly in this structure (or callbacks found from here).
> > Can we avoid it here?  
> 
> Hi Jonathan,
> 
> chip_info has to describe the chip hardwarewise, but there are different 
> bops depending on the wiring (interface used, and backend/no backend).

Normal solution to this is multiple chip specific structures so they
become specific to a chip + some wiring option. Then you just
pick between static const structures.

Does that work here?

You will need them exposed (extern) from your header but that's
not too bad.

Aim is to pick just one structure that describes all the 'specific'
stuff for the driver.  That brings all that stuff into one place and
provides an easy way to extend to new combinations of options for
other devices.

Jonathan


> 
> The easiest way I found was to use the ID in a switch/case to 
> determinate which bops I should take (well it was only needed in the spi 
> version since it is the one supporting almost all the chips while the 
> other ones still support only one). For instance, the ad7606B will use 
> ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.
> 
> If I can't use the ID, the only way I see is creating 3 fields in 
> chip_info (spi_ops, par_ops, backend_ops) and to initialize every 
> chip_info structure with its associated op(s) for the associated 
> interface. This would also lead to declare the different instances of 
> ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see 
> no other option).
> 
> Do you think it's better that way ? Or do you have any other idea ?
> 
> Regards,
> 
> Guillaume
> 
> >  
> >>    * @channels:		channel specification
> >>    * @num_channels:	number of channels
> >>    * @oversampling_avail	pointer to the array which stores the available
> >> @@ -50,6 +61,8 @@  
> > ...
> >  
> >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> >> index d651639c45eb..7bac39033955 100644
> >> --- a/drivers/iio/adc/ad7606_par.c
> >> +++ b/drivers/iio/adc/ad7606_par.c
> >> @@ -11,6 +11,7 @@
> >>   #include <linux/mod_devicetable.h>
> >>   #include <linux/module.h>
> >>   #include <linux/platform_device.h>
> >> +#include <linux/property.h>
> >>   #include <linux/types.h>
> >>   
> >>   #include <linux/iio/iio.h>
> >> @@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
> >>   
> >>   static int ad7606_par_probe(struct platform_device *pdev)
> >>   {
> >> -	const struct platform_device_id *id = platform_get_device_id(pdev);
> >> +	const struct ad7606_chip_info *chip_info;
> >> +	const struct platform_device_id *id;
> >>   	struct resource *res;
> >>   	void __iomem *addr;
> >>   	resource_size_t remap_size;
> >>   	int irq;
> >>   
> >> +	if (dev_fwnode(&pdev->dev)) {
> >> +		chip_info = device_get_match_data(&pdev->dev);
> >> +	} else {
> >> +		id = platform_get_device_id(pdev);
> >> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
> >> +	}
> >> +
> >>   	irq = platform_get_irq(pdev, 0);
> >>   	if (irq < 0)
> >>   		return irq;
> >> @@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
> >>   	remap_size = resource_size(res);
> >>   
> >>   	return ad7606_probe(&pdev->dev, irq, addr,
> >> -			    id->name, id->driver_data,  
> > Rewrap to move chip_info up a line perhaps.
> >  
> >> +			    chip_info,
> >>   			    remap_size > 1 ? &ad7606_par16_bops :
> >>   			    &ad7606_par8_bops);  
> 

On 10/4/24 16:25, Jonathan Cameron wrote:
> On Wed, 2 Oct 2024 02:12:28 +0200
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> On 9/29/24 14:44, Jonathan Cameron wrote:
>>> On Fri, 20 Sep 2024 17:33:27 +0000
>>> Guillaume Stols <gstols@baylibre.com> wrote:
>>>   
>>>> On the parallel version, the current implementation is only compatible
>>>> with id tables and won't work with fw_nodes, this commit intends to fix
>>>> it.
>>>>
>>>> Also, chip info is moved in the .h file so to be accessible to all the
>>> chip info is not moved (I was going to say no to that) but an
>>> extern is used to make it available. So say that rather than moved here.
>>>   
>>>> driver files that can set a pointer to the corresponding chip as the
>>>> driver data.
>>>>
>>>>    
>>>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>>>> index c13dda444526..18c87fe9a41a 100644
>>>> --- a/drivers/iio/adc/ad7606.h
>>>> +++ b/drivers/iio/adc/ad7606.h
>>>> @@ -38,8 +38,19 @@
>>>>    	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>>>    		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>>>    
>>>> +enum ad7606_supported_device_ids {
>>>> +	ID_AD7605_4,
>>>> +	ID_AD7606_8,
>>>> +	ID_AD7606_6,
>>>> +	ID_AD7606_4,
>>>> +	ID_AD7606B,
>>>> +	ID_AD7616,
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct ad7606_chip_info - chip specific information
>>>> + * @name		device name
>>>> + * @id			device id
>>> ID in chip info normally indicates something bad in the design. In that somewhere
>>> we have code that is ID dependent rather than all such code / data being
>>> found directly in this structure (or callbacks found from here).
>>> Can we avoid it here?
>> Hi Jonathan,
>>
>> chip_info has to describe the chip hardwarewise, but there are different
>> bops depending on the wiring (interface used, and backend/no backend).
> Normal solution to this is multiple chip specific structures so they
> become specific to a chip + some wiring option. Then you just
> pick between static const structures.
>
> Does that work here?
>
> You will need them exposed (extern) from your header but that's
> not too bad.
>
> Aim is to pick just one structure that describes all the 'specific'
> stuff for the driver.  That brings all that stuff into one place and
> provides an easy way to extend to new combinations of options for
> other devices.
>
> Jonathan

I finally wrote a structure containing the couple chip_info/bops

/**
  * struct ad7606_bus_info - agregate ad7606_chip_info and ad7606_bus_ops
  * @chip_info        entry in the table of chips that describes this device
  * @bops        bus operations (SPI or parallel)
  */
struct ad7606_bus_info {
     const struct ad7606_chip_info    *chip_info;
     const struct ad7606_bus_ops    *bops;
};

and declared the following way

const struct ad7606_bus_info ad7606b_bus_info = {
     .chip_info = &ad7606b_info,
     .bops = &ad7606b_spi_bops,
};

const struct ad7606_bus_info ad7606c_16_bus_info = {
     .chip_info = &ad7606c_16_info,
     .bops = &ad7606b_spi_bops,

etc...

Will send the series later today, just doing some last checks.

>
>
>> The easiest way I found was to use the ID in a switch/case to
>> determinate which bops I should take (well it was only needed in the spi
>> version since it is the one supporting almost all the chips while the
>> other ones still support only one). For instance, the ad7606B will use
>> ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.
>>
>> If I can't use the ID, the only way I see is creating 3 fields in
>> chip_info (spi_ops, par_ops, backend_ops) and to initialize every
>> chip_info structure with its associated op(s) for the associated
>> interface. This would also lead to declare the different instances of
>> ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see
>> no other option).
>>
>> Do you think it's better that way ? Or do you have any other idea ?
>>
>> Regards,
>>
>> Guillaume
>>
>>>   
>>>>     * @channels:		channel specification
>>>>     * @num_channels:	number of channels
>>>>     * @oversampling_avail	pointer to the array which stores the available
>>>> @@ -50,6 +61,8 @@
>>> ...
>>>   
>>>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>>>> index d651639c45eb..7bac39033955 100644
>>>> --- a/drivers/iio/adc/ad7606_par.c
>>>> +++ b/drivers/iio/adc/ad7606_par.c
>>>> @@ -11,6 +11,7 @@
>>>>    #include <linux/mod_devicetable.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/platform_device.h>
>>>> +#include <linux/property.h>
>>>>    #include <linux/types.h>
>>>>    
>>>>    #include <linux/iio/iio.h>
>>>> @@ -89,12 +90,20 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
>>>>    
>>>>    static int ad7606_par_probe(struct platform_device *pdev)
>>>>    {
>>>> -	const struct platform_device_id *id = platform_get_device_id(pdev);
>>>> +	const struct ad7606_chip_info *chip_info;
>>>> +	const struct platform_device_id *id;
>>>>    	struct resource *res;
>>>>    	void __iomem *addr;
>>>>    	resource_size_t remap_size;
>>>>    	int irq;
>>>>    
>>>> +	if (dev_fwnode(&pdev->dev)) {
>>>> +		chip_info = device_get_match_data(&pdev->dev);
>>>> +	} else {
>>>> +		id = platform_get_device_id(pdev);
>>>> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
>>>> +	}
>>>> +
>>>>    	irq = platform_get_irq(pdev, 0);
>>>>    	if (irq < 0)
>>>>    		return irq;
>>>> @@ -106,25 +115,25 @@ static int ad7606_par_probe(struct platform_device *pdev)
>>>>    	remap_size = resource_size(res);
>>>>    
>>>>    	return ad7606_probe(&pdev->dev, irq, addr,
>>>> -			    id->name, id->driver_data,
>>> Rewrap to move chip_info up a line perhaps.
>>>   
>>>> +			    chip_info,
>>>>    			    remap_size > 1 ? &ad7606_par16_bops :
>>>>    			    &ad7606_par8_bops);

On Fri, Sep 20, 2024 at 7:33 PM Guillaume Stols <gstols@baylibre.com> wrote:
>
> On the parallel version, the current implementation is only compatible
> with id tables and won't work with fw_nodes, this commit intends to fix
> it.
>
> Also, chip info is moved in the .h file so to be accessible to all the
> driver files that can set a pointer to the corresponding chip as the
> driver data.

This sounds like two unrelated changes, so maybe we should have two patches?


>  static const struct of_device_id ad7606_of_match[] = {
> -       { .compatible = "adi,ad7605-4" },
> -       { .compatible = "adi,ad7606-4" },
> -       { .compatible = "adi,ad7606-6" },
> -       { .compatible = "adi,ad7606-8" },
> -       { .compatible = "adi,ad7606b" },
> -       { .compatible = "adi,ad7616" },
> +       { .compatible = "adi,ad7605-4", &ad7605_4_info },
> +       { .compatible = "adi,ad7606-4", &ad7606_4_info },
> +       { .compatible = "adi,ad7606-6", &ad7606_6_info },
> +       { .compatible = "adi,ad7606-8", &ad7606_8_info },
> +       { .compatible = "adi,ad7606b", &ad7606b_info },
> +       { .compatible = "adi,ad7616", &ad7616_info },

Since we have .compatible = , we should also have .data = for the chip info.

On 9/24/24 17:28, David Lechner wrote:
> On Fri, Sep 20, 2024 at 7:33 PM Guillaume Stols <gstols@baylibre.com> wrote:
>> On the parallel version, the current implementation is only compatible
>> with id tables and won't work with fw_nodes, this commit intends to fix
>> it.
>>
>> Also, chip info is moved in the .h file so to be accessible to all the
>> driver files that can set a pointer to the corresponding chip as the
>> driver data.
> This sounds like two unrelated changes, so maybe we should have two patches?
Those changes are closely related to each other, in the sense that we 
now gather the ad7606_chip_info structure directly from the id or match 
structure, and not anymore the id which is an index where you can get it 
as an element. I will update the commit message to highlight it more.
>
>
>>   static const struct of_device_id ad7606_of_match[] = {
>> -       { .compatible = "adi,ad7605-4" },
>> -       { .compatible = "adi,ad7606-4" },
>> -       { .compatible = "adi,ad7606-6" },
>> -       { .compatible = "adi,ad7606-8" },
>> -       { .compatible = "adi,ad7606b" },
>> -       { .compatible = "adi,ad7616" },
>> +       { .compatible = "adi,ad7605-4", &ad7605_4_info },
>> +       { .compatible = "adi,ad7606-4", &ad7606_4_info },
>> +       { .compatible = "adi,ad7606-6", &ad7606_6_info },
>> +       { .compatible = "adi,ad7606-8", &ad7606_8_info },
>> +       { .compatible = "adi,ad7606b", &ad7606b_info },
>> +       { .compatible = "adi,ad7616", &ad7616_info },
> Since we have .compatible = , we should also have .data = for the chip info.
ack