The ad7476 driver uses a table of structures for defining the IC variant
specific data. Table is indexed using enum values, which are picked by
SPI ID.
Having the table and an enum adds extra complexity. It is potentially
unsafe if someone alters the enumeration values, or size of the IC data
table.
Simplify this by dropping the table and using individual structures for
the IC specific data, and storing the IC specific structure's address
directly in the SPI ID data.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
100% Untested.
No functional changes intended
---
drivers/iio/adc/ad7476.c | 292 +++++++++++++++++++--------------------
1 file changed, 143 insertions(+), 149 deletions(-)
diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index aea734aa06bd..bdfffc4f5724 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -52,29 +52,6 @@ struct ad7476_state {
unsigned char data[ALIGN(2, sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
};
-enum ad7476_supported_device_ids {
- ID_AD7091,
- ID_AD7091R,
- ID_AD7273,
- ID_AD7274,
- ID_AD7276,
- ID_AD7277,
- ID_AD7278,
- ID_AD7466,
- ID_AD7467,
- ID_AD7468,
- ID_AD7475,
- ID_AD7495,
- ID_AD7940,
- ID_ADC081S,
- ID_ADC101S,
- ID_ADC121S,
- ID_ADS7866,
- ID_ADS7867,
- ID_ADS7868,
- ID_LTC2314_14,
-};
-
static void ad7091_convst(struct ad7476_state *st)
{
if (!st->convst_gpio)
@@ -190,102 +167,119 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
#define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
BIT(IIO_CHAN_INFO_RAW))
-static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
- [ID_AD7091] = {
- .channel[0] = AD7091R_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .convst_channel[0] = AD7091R_CONVST_CHAN(12),
- .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .reset = ad7091_reset,
- },
- [ID_AD7091R] = {
- .channel[0] = AD7091R_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .convst_channel[0] = AD7091R_CONVST_CHAN(12),
- .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .int_vref_uv = 2500000,
- .has_vref = true,
- .reset = ad7091_reset,
- },
- [ID_AD7273] = {
- .channel[0] = AD7940_CHAN(10),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .has_vref = true,
- },
- [ID_AD7274] = {
- .channel[0] = AD7940_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .has_vref = true,
- },
- [ID_AD7276] = {
- .channel[0] = AD7940_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7277] = {
- .channel[0] = AD7940_CHAN(10),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7278] = {
- .channel[0] = AD7940_CHAN(8),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7466] = {
- .channel[0] = AD7476_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7467] = {
- .channel[0] = AD7476_CHAN(10),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7468] = {
- .channel[0] = AD7476_CHAN(8),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_AD7475] = {
- .channel[0] = AD7476_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .has_vref = true,
- .has_vdrive = true,
- },
- [ID_AD7495] = {
- .channel[0] = AD7476_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .int_vref_uv = 2500000,
- .has_vdrive = true,
- },
- [ID_AD7940] = {
- .channel[0] = AD7940_CHAN(14),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADC081S] = {
- .channel[0] = ADC081S_CHAN(8),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADC101S] = {
- .channel[0] = ADC081S_CHAN(10),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADC121S] = {
- .channel[0] = ADC081S_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADS7866] = {
- .channel[0] = ADS786X_CHAN(12),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADS7867] = {
- .channel[0] = ADS786X_CHAN(10),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_ADS7868] = {
- .channel[0] = ADS786X_CHAN(8),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- },
- [ID_LTC2314_14] = {
- .channel[0] = AD7940_CHAN(14),
- .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
- .has_vref = true,
- },
+static const struct ad7476_chip_info ad7091_chip_info = {
+ .channel[0] = AD7091R_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .convst_channel[0] = AD7091R_CONVST_CHAN(12),
+ .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .reset = ad7091_reset,
+};
+
+static const struct ad7476_chip_info ad7091r_chip_info = {
+ .channel[0] = AD7091R_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .convst_channel[0] = AD7091R_CONVST_CHAN(12),
+ .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .int_vref_uv = 2500000,
+ .has_vref = true,
+ .reset = ad7091_reset,
+};
+
+static const struct ad7476_chip_info ad7273_chip_info = {
+ .channel[0] = AD7940_CHAN(10),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .has_vref = true,
+};
+
+static const struct ad7476_chip_info ad7274_chip_info = {
+ .channel[0] = AD7940_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .has_vref = true,
+};
+
+static const struct ad7476_chip_info ad7276_chip_info = {
+ .channel[0] = AD7940_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7277_chip_info = {
+ .channel[0] = AD7940_CHAN(10),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7278_chip_info = {
+ .channel[0] = AD7940_CHAN(8),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7466_chip_info = {
+ .channel[0] = AD7476_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7467_chip_info = {
+ .channel[0] = AD7476_CHAN(10),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7468_chip_info = {
+ .channel[0] = AD7476_CHAN(8),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7475_chip_info = {
+ .channel[0] = AD7476_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .has_vref = true,
+ .has_vdrive = true,
+};
+
+static const struct ad7476_chip_info ad7495_chip_info = {
+ .channel[0] = AD7476_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .int_vref_uv = 2500000,
+ .has_vdrive = true,
+};
+
+static const struct ad7476_chip_info ad7940_chip_info = {
+ .channel[0] = AD7940_CHAN(14),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc081s_chip_info = {
+ .channel[0] = ADC081S_CHAN(8),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc101s_chip_info = {
+ .channel[0] = ADC081S_CHAN(10),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc121s_chip_info = {
+ .channel[0] = ADC081S_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7866_chip_info = {
+ .channel[0] = ADS786X_CHAN(12),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7867_chip_info = {
+ .channel[0] = ADS786X_CHAN(10),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7868_chip_info = {
+ .channel[0] = ADS786X_CHAN(8),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ltc2314_14_chip_info = {
+ .channel[0] = AD7940_CHAN(14),
+ .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+ .has_vref = true,
};
static const struct iio_info ad7476_info = {
@@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
st = iio_priv(indio_dev);
st->chip_info =
- &ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+ (struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
reg = devm_regulator_get(&spi->dev, "vcc");
if (IS_ERR(reg))
@@ -408,41 +402,41 @@ static int ad7476_probe(struct spi_device *spi)
}
static const struct spi_device_id ad7476_id[] = {
- { "ad7091", ID_AD7091 },
- { "ad7091r", ID_AD7091R },
- { "ad7273", ID_AD7273 },
- { "ad7274", ID_AD7274 },
- { "ad7276", ID_AD7276},
- { "ad7277", ID_AD7277 },
- { "ad7278", ID_AD7278 },
- { "ad7466", ID_AD7466 },
- { "ad7467", ID_AD7467 },
- { "ad7468", ID_AD7468 },
- { "ad7475", ID_AD7475 },
- { "ad7476", ID_AD7466 },
- { "ad7476a", ID_AD7466 },
- { "ad7477", ID_AD7467 },
- { "ad7477a", ID_AD7467 },
- { "ad7478", ID_AD7468 },
- { "ad7478a", ID_AD7468 },
- { "ad7495", ID_AD7495 },
- { "ad7910", ID_AD7467 },
- { "ad7920", ID_AD7466 },
- { "ad7940", ID_AD7940 },
- { "adc081s", ID_ADC081S },
- { "adc101s", ID_ADC101S },
- { "adc121s", ID_ADC121S },
- { "ads7866", ID_ADS7866 },
- { "ads7867", ID_ADS7867 },
- { "ads7868", ID_ADS7868 },
+ { "ad7091", (kernel_ulong_t)&ad7091_chip_info },
+ { "ad7091r", (kernel_ulong_t)&ad7091r_chip_info },
+ { "ad7273", (kernel_ulong_t)&ad7273_chip_info },
+ { "ad7274", (kernel_ulong_t)&ad7274_chip_info },
+ { "ad7276", (kernel_ulong_t)&ad7276_chip_info },
+ { "ad7277", (kernel_ulong_t)&ad7277_chip_info },
+ { "ad7278", (kernel_ulong_t)&ad7278_chip_info },
+ { "ad7466", (kernel_ulong_t)&ad7466_chip_info },
+ { "ad7467", (kernel_ulong_t)&ad7467_chip_info },
+ { "ad7468", (kernel_ulong_t)&ad7468_chip_info },
+ { "ad7475", (kernel_ulong_t)&ad7475_chip_info },
+ { "ad7476", (kernel_ulong_t)&ad7466_chip_info },
+ { "ad7476a", (kernel_ulong_t)&ad7466_chip_info },
+ { "ad7477", (kernel_ulong_t)&ad7467_chip_info },
+ { "ad7477a", (kernel_ulong_t)&ad7467_chip_info },
+ { "ad7478", (kernel_ulong_t)&ad7468_chip_info },
+ { "ad7478a", (kernel_ulong_t)&ad7468_chip_info },
+ { "ad7495", (kernel_ulong_t)&ad7495_chip_info },
+ { "ad7910", (kernel_ulong_t)&ad7467_chip_info },
+ { "ad7920", (kernel_ulong_t)&ad7466_chip_info },
+ { "ad7940", (kernel_ulong_t)&ad7940_chip_info },
+ { "adc081s", (kernel_ulong_t)&adc081s_chip_info },
+ { "adc101s", (kernel_ulong_t)&adc101s_chip_info },
+ { "adc121s", (kernel_ulong_t)&adc121s_chip_info },
+ { "ads7866", (kernel_ulong_t)&ads7866_chip_info },
+ { "ads7867", (kernel_ulong_t)&ads7867_chip_info },
+ { "ads7868", (kernel_ulong_t)&ads7868_chip_info },
/*
* The ROHM BU79100G is identical to the TI's ADS7866 from the software
* point of view. The binding document mandates the ADS7866 to be
* marked as a fallback for the BU79100G, but we still need the SPI ID
* here to make the module loading work.
*/
- { "bu79100g", ID_ADS7866 },
- { "ltc2314-14", ID_LTC2314_14 },
+ { "bu79100g", (kernel_ulong_t)&ads7866_chip_info },
+ { "ltc2314-14", (kernel_ulong_t)<c2314_14_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad7476_id);
--
2.50.1
On Fri, 1 Aug 2025 13:07:13 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> The ad7476 driver uses a table of structures for defining the IC variant
> specific data. Table is indexed using enum values, which are picked by
> SPI ID.
>
> Having the table and an enum adds extra complexity. It is potentially
> unsafe if someone alters the enumeration values, or size of the IC data
> table.
>
> Simplify this by dropping the table and using individual structures for
> the IC specific data, and storing the IC specific structure's address
> directly in the SPI ID data.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
> 100% Untested.
> No functional changes intended
One tiny thing inline, otherwise looks good to me. This aligns with
how we prefer to do things these days. Tends to end up easier to read
than the enum array thing and best of all removes any temptation to use
the enum for anything else.
>
> static const struct iio_info ad7476_info = {
> @@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
>
> st = iio_priv(indio_dev);
> st->chip_info =
> - &ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> + (struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
Switch to spi_get_device_match_data()
which checks via generic firmware paths first (so DT here) and then the
old school tables. Also returns a void * so gets rid of need to cast.
Only works with all pointers (or a lot of care) because a value 0 is a
fail to match. So kind of enabled by your patch.
Jonathan
>
> reg = devm_regulator_get(&spi->dev, "vcc");
> if (IS_ERR(reg))
> @@ -408,41 +402,41 @@ static int ad7476_probe(struct spi_device *spi)
> }
On 01/08/2025 14:09, Jonathan Cameron wrote:
> On Fri, 1 Aug 2025 13:07:13 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
>> The ad7476 driver uses a table of structures for defining the IC variant
>> specific data. Table is indexed using enum values, which are picked by
>> SPI ID.
>>
>> Having the table and an enum adds extra complexity. It is potentially
>> unsafe if someone alters the enumeration values, or size of the IC data
>> table.
>>
>> Simplify this by dropping the table and using individual structures for
>> the IC specific data, and storing the IC specific structure's address
>> directly in the SPI ID data.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> 100% Untested.
>> No functional changes intended
>
> One tiny thing inline, otherwise looks good to me. This aligns with
> how we prefer to do things these days. Tends to end up easier to read
> than the enum array thing and best of all removes any temptation to use
> the enum for anything else.
>
>>
>> static const struct iio_info ad7476_info = {
>> @@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
>>
>> st = iio_priv(indio_dev);
>> st->chip_info =
>> - &ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>> + (struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
>
> Switch to spi_get_device_match_data()
> which checks via generic firmware paths first (so DT here) and then the
> old school tables. Also returns a void * so gets rid of need to cast.
Ah. Right! Thanks!
> Only works with all pointers (or a lot of care) because a value 0 is a
> fail to match. So kind of enabled by your patch.
Yours,
-- Matti
On Mon, Aug 4, 2025 at 7:57 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 01/08/2025 14:09, Jonathan Cameron wrote: > > On Fri, 1 Aug 2025 13:07:13 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: ... > >> st = iio_priv(indio_dev); > >> st->chip_info = > >> - &ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data]; > >> + (struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data; > > > > Switch to spi_get_device_match_data() > > which checks via generic firmware paths first (so DT here) and then the > > old school tables. Also returns a void * so gets rid of need to cast. > > Ah. Right! Thanks! More importantly it returns _const_ void *. And qualifier makes a lot of sense here. > > Only works with all pointers (or a lot of care) because a value 0 is a > > fail to match. So kind of enabled by your patch. -- With Best Regards, Andy Shevchenko
On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > The ad7476 driver uses a table of structures for defining the IC variant > specific data. Table is indexed using enum values, which are picked by > SPI ID. > > Having the table and an enum adds extra complexity. It is potentially > unsafe if someone alters the enumeration values, or size of the IC data > table. I don't see the problem here. I like the part about converting ID tables to use chip_info instead of plain integers, but other than that I do not see how enum is worse than the split version. -- With Best Regards, Andy Shevchenko
On 02/08/2025 01:01, Andy Shevchenko wrote: > On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen > <mazziesaccount@gmail.com> wrote: >> >> The ad7476 driver uses a table of structures for defining the IC variant >> specific data. Table is indexed using enum values, which are picked by >> SPI ID. >> >> Having the table and an enum adds extra complexity. It is potentially >> unsafe if someone alters the enumeration values, or size of the IC data >> table. > > I don't see the problem here. I like the part about converting ID > tables to use chip_info instead of plain integers, but other than that > I do not see how enum is worse than the split version. The potential culprit with using the enum for array indexing is, that it requires the array size and enum values (used for indexing) to stay in sync. Eg, used enum values must be smaller than the size of the array. Also, the chip-info items in the array must be kept in locations which match the enum values. Yes, we have ways to do this, often using the last enum value as the size of the array, and/or using designated array initializers. It still requires programmer to do this correctly. Changing enum at the top of the file may break the array indexing (in seemingly unrelated place, at the bottom of the file). I agree this is pretty trivial issue, but it's still a thing to keep in mind. Splitting the chip-info in own structs and using direct pointer to the struct makes it harder to get it wrong. Finally, dropping the enum makes adding code which does decisions based on the chip-ID less appealing. It hopefully encourages adding _all_ IC specific quirks in the chip-info instead, which will keep the code path (IMHO) cleaner when all chip-specifics are in the chip-info. Anyways, Thanks for the feedback! Yours, -- Matti
On Mon, Aug 4, 2025 at 7:56 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 02/08/2025 01:01, Andy Shevchenko wrote: > > On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen > > <mazziesaccount@gmail.com> wrote: > >> > >> The ad7476 driver uses a table of structures for defining the IC variant > >> specific data. Table is indexed using enum values, which are picked by > >> SPI ID. > >> > >> Having the table and an enum adds extra complexity. It is potentially > >> unsafe if someone alters the enumeration values, or size of the IC data > >> table. > > > > I don't see the problem here. I like the part about converting ID > > tables to use chip_info instead of plain integers, but other than that > > I do not see how enum is worse than the split version. > > The potential culprit with using the enum for array indexing is, that it > requires the array size and enum values (used for indexing) to stay in > sync. Eg, used enum values must be smaller than the size of the array. > Also, the chip-info items in the array must be kept in locations which > match the enum values. > > Yes, we have ways to do this, often using the last enum value as the > size of the array, > and/or using designated array initializers. That's what I kept in mind and seems already the case in this driver. That's why I doubt the brave statement in the commit message. > It still > requires programmer to do this correctly. Changing enum at the top of > the file may break the array indexing (in seemingly unrelated place, at > the bottom of the file). I agree this is pretty trivial issue, but it's > still a thing to keep in mind. > > Splitting the chip-info in own structs and using direct pointer to the > struct makes it harder to get it wrong. > > Finally, dropping the enum makes adding code which does decisions based > on the chip-ID less appealing. It hopefully encourages adding _all_ IC > specific quirks in the chip-info instead, which will keep the code path > (IMHO) cleaner when all chip-specifics are in the chip-info. Final argument makes sense to me. > Anyways, Thanks for the feedback! You're always welcome. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.