| Message ID | 20250408132120.836461-3-sbellary@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ti-adc128s052: Add support for adc102s021 | expand |
On 4/8/25 8:21 AM, Sukrut Bellary wrote: > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > interface. The device family responds with 12-bit data, of which the LSB > bits are transmitted by the lower resolution devices as 0. > The unavailable bits are 0 in LSB. > Shift is calculated per resolution and used in scaling and > raw data read. Could improve the line wrapping in the commit message if there is a v4. > > Lets reuse the driver to support the family of devices with name > ADC<bb><c>S<sss>, where > * bb is the resolution in number of bits (8, 10, 12) > * c is the number of channels (1, 2, 4, 8) > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > and 101 for 1 MSPS) > > Complete datasheets are available at TI's website here: > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > Tested only with ti-adc102s051 on BegalePlay SBC. > https://www.beagleboard.org/boards/beagleplay > > Co-developed-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > --- I didn't see any serious issues, just some room for more polish... > Changes in v3: > - used be16_to_cpu() for the endian conversion. > - used config index enum while setting up the adc128_config[] > > - Link to v2: > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > Changes in v2: > - Arranged of_device_id and spi_device_id in numeric order. > - Used enum to index into adc128_config. > - Reorder adc128_config in alphabetical. > - Include channel resolution information. > - Shift is calculated per resolution and used in scaling and > raw data read. > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > --- > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > 1 file changed, 112 insertions(+), 37 deletions(-) > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > index a456ea78462f..d4b76fd85abd 100644 > --- a/drivers/iio/adc/ti-adc128s052.c > +++ b/drivers/iio/adc/ti-adc128s052.c > @@ -7,6 +7,22 @@ > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > + * > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > + * interface. This driver supports the whole family of devices with a name > + * ADC<bb><c>S<sss>, where > + * bb is the resolution in number of bits (8, 10, 12) > + * c is the number of channels (1, 2, 4, 8) > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > + * and 101 for 1 MSPS) Looks like odd line wrapping. I assume bullet points were meant here? * ... where: * - bb is ... * - c is ... * - sss is ... > + * > + * Complete datasheets are available at TI's website here: > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > + * > + * 8, 10, and 12 bits converters send 12-bit data with > + * unavailable bits set to 0 in LSB. > + * Shift is calculated per resolution and used in scaling and > + * raw data read. > */ > > #include <linux/err.h> > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > if (ret < 0) > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > + return be16_to_cpu(*((__be16 *)adc->buffer)); > } > > static int adc128_read_raw(struct iio_dev *indio_dev, > @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > if (ret < 0) > return ret; > > - *val = ret; > + *val = (ret >> channel->scan_type.shift) & > + GENMASK(channel->scan_type.realbits - 1, 0); It's a bit odd to do this here instead of in the helper function since the helper function is doing some rearranging of bits already. Could pass scan_type to the helper function and do it all in one place. > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > return ret; > > *val = ret / 1000; > - *val2 = 12; > + *val2 = channel->scan_type.realbits; > return IIO_VAL_FRACTIONAL_LOG2; > > default: > @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > } > > -#define ADC128_VOLTAGE_CHANNEL(num) \ > - { \ > - .type = IIO_VOLTAGE, \ > - .indexed = 1, \ > - .channel = (num), \ > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (num), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_index = (num), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = (real_bits), \ > + .storagebits = (store_bits), \ It looks like storagebits is always 16, so we could drop that parameter. > + .shift = (12 - real_bits), \ > + }, \ > } > > -static const struct iio_chan_spec adc128s052_channels[] = { > - ADC128_VOLTAGE_CHANNEL(0), > - ADC128_VOLTAGE_CHANNEL(1), > - ADC128_VOLTAGE_CHANNEL(2), > - ADC128_VOLTAGE_CHANNEL(3), > - ADC128_VOLTAGE_CHANNEL(4), > - ADC128_VOLTAGE_CHANNEL(5), > - ADC128_VOLTAGE_CHANNEL(6), > - ADC128_VOLTAGE_CHANNEL(7), > +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16) > +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16) > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16) > + > +static const struct iio_chan_spec adc082s021_channels[] = { > + ADC082_VOLTAGE_CHANNEL(0), > + ADC082_VOLTAGE_CHANNEL(1), > +}; > + > +static const struct iio_chan_spec adc102s021_channels[] = { > + ADC102_VOLTAGE_CHANNEL(0), > + ADC102_VOLTAGE_CHANNEL(1), > }; > > static const struct iio_chan_spec adc122s021_channels[] = { > @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = { > ADC128_VOLTAGE_CHANNEL(3), > }; > > +static const struct iio_chan_spec adc128s052_channels[] = { > + ADC128_VOLTAGE_CHANNEL(0), > + ADC128_VOLTAGE_CHANNEL(1), > + ADC128_VOLTAGE_CHANNEL(2), > + ADC128_VOLTAGE_CHANNEL(3), > + ADC128_VOLTAGE_CHANNEL(4), > + ADC128_VOLTAGE_CHANNEL(5), > + ADC128_VOLTAGE_CHANNEL(6), > + ADC128_VOLTAGE_CHANNEL(7), > +}; > + > +enum adc128_configuration_index { > + ADC128_CONFIG_INDEX_082S, > + ADC128_CONFIG_INDEX_102S, > + ADC128_CONFIG_INDEX_122S, > + ADC128_CONFIG_INDEX_124S, > + ADC128_CONFIG_INDEX_128S, > +}; > + > static const struct adc128_configuration adc128_config[] = { I would have rather removed the array here. Adding the enum just makes lots more code to read without any technical benefit. > - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) }, > - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) }, > - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) }, > + [ADC128_CONFIG_INDEX_082S] = { > + .channels = adc082s021_channels, > + .num_channels = ARRAY_SIZE(adc082s021_channels) > + }, > + [ADC128_CONFIG_INDEX_102S] = { > + .channels = adc102s021_channels, > + .num_channels = ARRAY_SIZE(adc102s021_channels) > + }, > + [ADC128_CONFIG_INDEX_122S] = { > + .channels = adc122s021_channels, > + .num_channels = ARRAY_SIZE(adc122s021_channels) > + }, > + [ADC128_CONFIG_INDEX_124S] = { > + .channels = adc124s021_channels, > + .num_channels = ARRAY_SIZE(adc124s021_channels) > + }, > + [ADC128_CONFIG_INDEX_128S] = { > + .channels = adc128s052_channels, > + .num_channels = ARRAY_SIZE(adc128s052_channels) > + }, > }; I.e. instead: static const struct adc128_configuration adc08s021_config = { .channels = adc082s021_channels, .num_channels = ARRAY_SIZE(adc082s021_channels), }; static const struct adc128_configuration adc10s021_config = { .channels = adc102s021_channels, .num_channels = ARRAY_SIZE(adc102s021_channels) }; ... > > static const struct iio_info adc128_info = { > @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi) > } > > static const struct of_device_id adc128_of_match[] = { > - { .compatible = "ti,adc128s052", .data = &adc128_config[0] }, > - { .compatible = "ti,adc122s021", .data = &adc128_config[1] }, > - { .compatible = "ti,adc122s051", .data = &adc128_config[1] }, > - { .compatible = "ti,adc122s101", .data = &adc128_config[1] }, > - { .compatible = "ti,adc124s021", .data = &adc128_config[2] }, > - { .compatible = "ti,adc124s051", .data = &adc128_config[2] }, > - { .compatible = "ti,adc124s101", .data = &adc128_config[2] }, > + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] }, > { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, adc128_of_match); It would be easier to see what is new and what is changed if we split out the "cleanup" to a separate patch. > > static const struct spi_device_id adc128_id[] = { > - { "adc128s052", (kernel_ulong_t)&adc128_config[0] }, > - { "adc122s021", (kernel_ulong_t)&adc128_config[1] }, > - { "adc122s051", (kernel_ulong_t)&adc128_config[1] }, > - { "adc122s101", (kernel_ulong_t)&adc128_config[1] }, > - { "adc124s021", (kernel_ulong_t)&adc128_config[2] }, > - { "adc124s051", (kernel_ulong_t)&adc128_config[2] }, > - { "adc124s101", (kernel_ulong_t)&adc128_config[2] }, > + { "adc082s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { "adc082s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { "adc082s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { "adc102s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { "adc102s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { "adc102s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { "adc122s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { "adc122s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { "adc122s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { "adc124s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { "adc124s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { "adc124s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { "adc128s052", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_128S] }, > { } > }; > MODULE_DEVICE_TABLE(spi, adc128_id); > > static const struct acpi_device_id adc128_acpi_match[] = { > - { "AANT1280", (kernel_ulong_t)&adc128_config[2] }, > + { "AANT1280", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > { } > }; > MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
On Tue, Apr 08, 2025 at 03:57:32PM -0500, David Lechner wrote: > On 4/8/25 8:21 AM, Sukrut Bellary wrote: > > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > interface. The device family responds with 12-bit data, of which the LSB > > bits are transmitted by the lower resolution devices as 0. > > The unavailable bits are 0 in LSB. > > Shift is calculated per resolution and used in scaling and > > raw data read. > > Could improve the line wrapping in the commit message if there is a v4. Thanks for the review. Ok, I will improve in v4. > > > > Lets reuse the driver to support the family of devices with name > > ADC<bb><c>S<sss>, where > > * bb is the resolution in number of bits (8, 10, 12) > > * c is the number of channels (1, 2, 4, 8) > > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > and 101 for 1 MSPS) > > > > Complete datasheets are available at TI's website here: > > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > > > Tested only with ti-adc102s051 on BegalePlay SBC. > > https://www.beagleboard.org/boards/beagleplay > > > > Co-developed-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > > --- > > I didn't see any serious issues, just some room for more polish... > > > Changes in v3: > > - used be16_to_cpu() for the endian conversion. > > - used config index enum while setting up the adc128_config[] > > > > - Link to v2: > > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > > > Changes in v2: > > - Arranged of_device_id and spi_device_id in numeric order. > > - Used enum to index into adc128_config. > > - Reorder adc128_config in alphabetical. > > - Include channel resolution information. > > - Shift is calculated per resolution and used in scaling and > > raw data read. > > > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > > --- > > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > > 1 file changed, 112 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > > index a456ea78462f..d4b76fd85abd 100644 > > --- a/drivers/iio/adc/ti-adc128s052.c > > +++ b/drivers/iio/adc/ti-adc128s052.c > > @@ -7,6 +7,22 @@ > > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > > + * > > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > + * interface. This driver supports the whole family of devices with a name > > + * ADC<bb><c>S<sss>, where > > + * bb is the resolution in number of bits (8, 10, 12) > > + * c is the number of channels (1, 2, 4, 8) > > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > + * and 101 for 1 MSPS) > > Looks like odd line wrapping. I assume bullet points were meant here? * were part of comments. I will fix line wrapping. > * ... where: > * - bb is ... > * - c is ... > * - sss is ... > > > + * > > + * Complete datasheets are available at TI's website here: > > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > + * > > + * 8, 10, and 12 bits converters send 12-bit data with > > + * unavailable bits set to 0 in LSB. > > + * Shift is calculated per resolution and used in scaling and > > + * raw data read. > > */ > > > > #include <linux/err.h> > > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > > if (ret < 0) > > return ret; > > > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > > + return be16_to_cpu(*((__be16 *)adc->buffer)); > > } > > > > static int adc128_read_raw(struct iio_dev *indio_dev, > > @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > if (ret < 0) > > return ret; > > > > - *val = ret; > > + *val = (ret >> channel->scan_type.shift) & > > + GENMASK(channel->scan_type.realbits - 1, 0); > > It's a bit odd to do this here instead of in the helper function since > the helper function is doing some rearranging of bits already. > > Could pass scan_type to the helper function and do it all in one > place. Ok, I will try with helper in v4. > > return IIO_VAL_INT; > > > > case IIO_CHAN_INFO_SCALE: > > @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > return ret; > > > > *val = ret / 1000; > > - *val2 = 12; > > + *val2 = channel->scan_type.realbits; > > return IIO_VAL_FRACTIONAL_LOG2; > > > > default: > > @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > > > } > > > > -#define ADC128_VOLTAGE_CHANNEL(num) \ > > - { \ > > - .type = IIO_VOLTAGE, \ > > - .indexed = 1, \ > > - .channel = (num), \ > > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > > +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \ > > + { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = (num), \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .scan_index = (num), \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = (real_bits), \ > > + .storagebits = (store_bits), \ > > It looks like storagebits is always 16, so we could drop that parameter. Sure, I will drop storagebits. > > + .shift = (12 - real_bits), \ > > + }, \ > > } > > > > -static const struct iio_chan_spec adc128s052_channels[] = { > > - ADC128_VOLTAGE_CHANNEL(0), > > - ADC128_VOLTAGE_CHANNEL(1), > > - ADC128_VOLTAGE_CHANNEL(2), > > - ADC128_VOLTAGE_CHANNEL(3), > > - ADC128_VOLTAGE_CHANNEL(4), > > - ADC128_VOLTAGE_CHANNEL(5), > > - ADC128_VOLTAGE_CHANNEL(6), > > - ADC128_VOLTAGE_CHANNEL(7), > > +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16) > > +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16) > > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16) > > + > > +static const struct iio_chan_spec adc082s021_channels[] = { > > + ADC082_VOLTAGE_CHANNEL(0), > > + ADC082_VOLTAGE_CHANNEL(1), > > +}; > > + > > +static const struct iio_chan_spec adc102s021_channels[] = { > > + ADC102_VOLTAGE_CHANNEL(0), > > + ADC102_VOLTAGE_CHANNEL(1), > > }; > > > > static const struct iio_chan_spec adc122s021_channels[] = { > > @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = { > > ADC128_VOLTAGE_CHANNEL(3), > > }; > > > > +static const struct iio_chan_spec adc128s052_channels[] = { > > + ADC128_VOLTAGE_CHANNEL(0), > > + ADC128_VOLTAGE_CHANNEL(1), > > + ADC128_VOLTAGE_CHANNEL(2), > > + ADC128_VOLTAGE_CHANNEL(3), > > + ADC128_VOLTAGE_CHANNEL(4), > > + ADC128_VOLTAGE_CHANNEL(5), > > + ADC128_VOLTAGE_CHANNEL(6), > > + ADC128_VOLTAGE_CHANNEL(7), > > +}; > > + > > +enum adc128_configuration_index { > > + ADC128_CONFIG_INDEX_082S, > > + ADC128_CONFIG_INDEX_102S, > > + ADC128_CONFIG_INDEX_122S, > > + ADC128_CONFIG_INDEX_124S, > > + ADC128_CONFIG_INDEX_128S, > > +}; > > + > > static const struct adc128_configuration adc128_config[] = { > > I would have rather removed the array here. Adding the enum just > makes lots more code to read without any technical benefit. > > > - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) }, > > - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) }, > > - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) }, > > + [ADC128_CONFIG_INDEX_082S] = { > > + .channels = adc082s021_channels, > > + .num_channels = ARRAY_SIZE(adc082s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_102S] = { > > + .channels = adc102s021_channels, > > + .num_channels = ARRAY_SIZE(adc102s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_122S] = { > > + .channels = adc122s021_channels, > > + .num_channels = ARRAY_SIZE(adc122s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_124S] = { > > + .channels = adc124s021_channels, > > + .num_channels = ARRAY_SIZE(adc124s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_128S] = { > > + .channels = adc128s052_channels, > > + .num_channels = ARRAY_SIZE(adc128s052_channels) > > + }, > > }; > > I.e. instead: > > static const struct adc128_configuration adc08s021_config = { > .channels = adc082s021_channels, > .num_channels = ARRAY_SIZE(adc082s021_channels), > }; > > static const struct adc128_configuration adc10s021_config = { > .channels = adc102s021_channels, > .num_channels = ARRAY_SIZE(adc102s021_channels) > }; > > ... OK, I will remove enum. > > > > static const struct iio_info adc128_info = { > > @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi) > > } > > > > static const struct of_device_id adc128_of_match[] = { > > - { .compatible = "ti,adc128s052", .data = &adc128_config[0] }, > > - { .compatible = "ti,adc122s021", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc122s051", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc122s101", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc124s021", .data = &adc128_config[2] }, > > - { .compatible = "ti,adc124s051", .data = &adc128_config[2] }, > > - { .compatible = "ti,adc124s101", .data = &adc128_config[2] }, > > + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] }, > > { /* sentinel */ }, > > }; > > MODULE_DEVICE_TABLE(of, adc128_of_match); > > It would be easier to see what is new and what is changed if we split out the > "cleanup" to a separate patch. This is not a cleanup. Addressed the review comments on v1 i.e., to arrange of_device_id and spi_device_id in a numeric order. And added more device ids used enum. > > > > static const struct spi_device_id adc128_id[] = { > > - { "adc128s052", (kernel_ulong_t)&adc128_config[0] }, > > - { "adc122s021", (kernel_ulong_t)&adc128_config[1] }, > > - { "adc122s051", (kernel_ulong_t)&adc128_config[1] }, > > - { "adc122s101", (kernel_ulong_t)&adc128_config[1] }, > > - { "adc124s021", (kernel_ulong_t)&adc128_config[2] }, > > - { "adc124s051", (kernel_ulong_t)&adc128_config[2] }, > > - { "adc124s101", (kernel_ulong_t)&adc128_config[2] }, > > + { "adc082s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { "adc082s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { "adc082s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { "adc102s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { "adc102s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { "adc102s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { "adc122s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { "adc122s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { "adc122s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { "adc124s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { "adc124s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { "adc124s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { "adc128s052", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_128S] }, > > { } > > }; > > MODULE_DEVICE_TABLE(spi, adc128_id); > > > > static const struct acpi_device_id adc128_acpi_match[] = { > > - { "AANT1280", (kernel_ulong_t)&adc128_config[2] }, > > + { "AANT1280", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, > > { } > > }; > > MODULE_DEVICE_TABLE(acpi, adc128_acpi_match); >
On Tue, 8 Apr 2025 06:21:20 -0700 Sukrut Bellary <sbellary@baylibre.com> wrote: > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > interface. The device family responds with 12-bit data, of which the LSB > bits are transmitted by the lower resolution devices as 0. > The unavailable bits are 0 in LSB. > Shift is calculated per resolution and used in scaling and > raw data read. > > Lets reuse the driver to support the family of devices with name > ADC<bb><c>S<sss>, where > * bb is the resolution in number of bits (8, 10, 12) > * c is the number of channels (1, 2, 4, 8) > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > and 101 for 1 MSPS) > > Complete datasheets are available at TI's website here: > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > Tested only with ti-adc102s051 on BegalePlay SBC. > https://www.beagleboard.org/boards/beagleplay > > Co-developed-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > --- > Changes in v3: > - used be16_to_cpu() for the endian conversion. > - used config index enum while setting up the adc128_config[] > > - Link to v2: > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > Changes in v2: > - Arranged of_device_id and spi_device_id in numeric order. > - Used enum to index into adc128_config. > - Reorder adc128_config in alphabetical. > - Include channel resolution information. > - Shift is calculated per resolution and used in scaling and > raw data read. > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > --- > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > 1 file changed, 112 insertions(+), 37 deletions(-) > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > index a456ea78462f..d4b76fd85abd 100644 > --- a/drivers/iio/adc/ti-adc128s052.c > +++ b/drivers/iio/adc/ti-adc128s052.c > @@ -7,6 +7,22 @@ > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > + * > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > + * interface. This driver supports the whole family of devices with a name > + * ADC<bb><c>S<sss>, where > + * bb is the resolution in number of bits (8, 10, 12) > + * c is the number of channels (1, 2, 4, 8) > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > + * and 101 for 1 MSPS) > + * > + * Complete datasheets are available at TI's website here: > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > + * > + * 8, 10, and 12 bits converters send 12-bit data with > + * unavailable bits set to 0 in LSB. > + * Shift is calculated per resolution and used in scaling and > + * raw data read. > */ > > #include <linux/err.h> > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > if (ret < 0) > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > + return be16_to_cpu(*((__be16 *)adc->buffer)); I think we now have a convenient adc->buffer16 (probably introduced in Matti's series. Sorry I missed the overlap of the two series until now. These part numbers are too long and confusing to stick in my head! Matti took on maintaining that driver because he wanted to see any changes that might affect the Rohm part it now supports. If anyone wants to volunteer from the TI side of things that would be ideal - just send a patch adding to the new MAINTAINERS entry. Jonathan > }
On 08/04/2025 16:21, Sukrut Bellary wrote: > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > interface. The device family responds with 12-bit data, of which the LSB > bits are transmitted by the lower resolution devices as 0. > The unavailable bits are 0 in LSB. > Shift is calculated per resolution and used in scaling and > raw data read. > > Lets reuse the driver to support the family of devices with name > ADC<bb><c>S<sss>, where > * bb is the resolution in number of bits (8, 10, 12) > * c is the number of channels (1, 2, 4, 8) > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > and 101 for 1 MSPS) > > Complete datasheets are available at TI's website here: > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf I tried looking up: https://www.ti.com/lit/gpn/adc102s051.pdf > > Tested only with ti-adc102s051 on BegalePlay SBC. > https://www.beagleboard.org/boards/beagleplay > > Co-developed-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > --- > Changes in v3: > - used be16_to_cpu() for the endian conversion. > - used config index enum while setting up the adc128_config[] > > - Link to v2: > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > Changes in v2: > - Arranged of_device_id and spi_device_id in numeric order. > - Used enum to index into adc128_config. > - Reorder adc128_config in alphabetical. > - Include channel resolution information. > - Shift is calculated per resolution and used in scaling and > raw data read. > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > --- > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > 1 file changed, 112 insertions(+), 37 deletions(-) > Hi dee Ho, Thanks for improving this! It's always nice to be able to support more devices with small(ish) changes! This looks good to me. I will take another, hopefully more in-depth look at the rebased version when available though. I have just one comment for now, but it's not strictly related to this change. If you wish to go the extra mile, then I'd appreciated it. If not, then it can be re-worked later. Anyways, please, see below. > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > index a456ea78462f..d4b76fd85abd 100644 > --- a/drivers/iio/adc/ti-adc128s052.c > +++ b/drivers/iio/adc/ti-adc128s052.c > @@ -7,6 +7,22 @@ > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > + * > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > + * interface. This driver supports the whole family of devices with a name > + * ADC<bb><c>S<sss>, where > + * bb is the resolution in number of bits (8, 10, 12) > + * c is the number of channels (1, 2, 4, 8) > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > + * and 101 for 1 MSPS) > + * > + * Complete datasheets are available at TI's website here: > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > + * > + * 8, 10, and 12 bits converters send 12-bit data with > + * unavailable bits set to 0 in LSB. > + * Shift is calculated per resolution and used in scaling and > + * raw data read. > */ > > #include <linux/err.h> > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > if (ret < 0) > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > + return be16_to_cpu(*((__be16 *)adc->buffer)); > } > > static int adc128_read_raw(struct iio_dev *indio_dev, > @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > if (ret < 0) > return ret; > > - *val = ret; > + *val = (ret >> channel->scan_type.shift) & > + GENMASK(channel->scan_type.realbits - 1, 0); > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > return ret; > > *val = ret / 1000; > - *val2 = 12; > + *val2 = channel->scan_type.realbits; > return IIO_VAL_FRACTIONAL_LOG2; > > default: > @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > } > > -#define ADC128_VOLTAGE_CHANNEL(num) \ > - { \ > - .type = IIO_VOLTAGE, \ > - .indexed = 1, \ > - .channel = (num), \ > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (num), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_index = (num), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = (real_bits), \ > + .storagebits = (store_bits), \ > + .shift = (12 - real_bits), \ > + }, \ > } > > -static const struct iio_chan_spec adc128s052_channels[] = { > - ADC128_VOLTAGE_CHANNEL(0), > - ADC128_VOLTAGE_CHANNEL(1), > - ADC128_VOLTAGE_CHANNEL(2), > - ADC128_VOLTAGE_CHANNEL(3), > - ADC128_VOLTAGE_CHANNEL(4), > - ADC128_VOLTAGE_CHANNEL(5), > - ADC128_VOLTAGE_CHANNEL(6), > - ADC128_VOLTAGE_CHANNEL(7), > +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16) > +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16) > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16) > + > +static const struct iio_chan_spec adc082s021_channels[] = { > + ADC082_VOLTAGE_CHANNEL(0), > + ADC082_VOLTAGE_CHANNEL(1), > +}; > + > +static const struct iio_chan_spec adc102s021_channels[] = { > + ADC102_VOLTAGE_CHANNEL(0), > + ADC102_VOLTAGE_CHANNEL(1), > }; > > static const struct iio_chan_spec adc122s021_channels[] = { > @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = { > ADC128_VOLTAGE_CHANNEL(3), > }; > > +static const struct iio_chan_spec adc128s052_channels[] = { > + ADC128_VOLTAGE_CHANNEL(0), > + ADC128_VOLTAGE_CHANNEL(1), > + ADC128_VOLTAGE_CHANNEL(2), > + ADC128_VOLTAGE_CHANNEL(3), > + ADC128_VOLTAGE_CHANNEL(4), > + ADC128_VOLTAGE_CHANNEL(5), > + ADC128_VOLTAGE_CHANNEL(6), > + ADC128_VOLTAGE_CHANNEL(7), > +}; > + > +enum adc128_configuration_index { > + ADC128_CONFIG_INDEX_082S, > + ADC128_CONFIG_INDEX_102S, > + ADC128_CONFIG_INDEX_122S, > + ADC128_CONFIG_INDEX_124S, > + ADC128_CONFIG_INDEX_128S, > +}; I like the fact you added these indexes as it makes this a lot clearer. But... > + > static const struct adc128_configuration adc128_config[] = { > - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) }, > - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) }, > - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) }, > + [ADC128_CONFIG_INDEX_082S] = { > + .channels = adc082s021_channels, > + .num_channels = ARRAY_SIZE(adc082s021_channels) > + }, > + [ADC128_CONFIG_INDEX_102S] = { > + .channels = adc102s021_channels, > + .num_channels = ARRAY_SIZE(adc102s021_channels) > + }, > + [ADC128_CONFIG_INDEX_122S] = { > + .channels = adc122s021_channels, > + .num_channels = ARRAY_SIZE(adc122s021_channels) > + }, > + [ADC128_CONFIG_INDEX_124S] = { > + .channels = adc124s021_channels, > + .num_channels = ARRAY_SIZE(adc124s021_channels) > + }, > + [ADC128_CONFIG_INDEX_128S] = { > + .channels = adc128s052_channels, > + .num_channels = ARRAY_SIZE(adc128s052_channels) > + }, > }; ... I don't really love this array. I believe the code would be clearer if this array was changed to individual structs because ... > > static const struct iio_info adc128_info = { > @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi) > } > > static const struct of_device_id adc128_of_match[] = { > - { .compatible = "ti,adc128s052", .data = &adc128_config[0] }, > - { .compatible = "ti,adc122s021", .data = &adc128_config[1] }, > - { .compatible = "ti,adc122s051", .data = &adc128_config[1] }, > - { .compatible = "ti,adc122s101", .data = &adc128_config[1] }, > - { .compatible = "ti,adc124s021", .data = &adc128_config[2] }, > - { .compatible = "ti,adc124s051", .data = &adc128_config[2] }, > - { .compatible = "ti,adc124s101", .data = &adc128_config[2] }, > + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] }, ... here we could then directly refer to individual structs. That way we would not need to define the names for the array indexes (for clarity), or look up the individual array members based on magic numbers. > { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, adc128_of_match); Yours, -- Matti
On 09:40-20250414, Matti Vaittinen wrote: > On 08/04/2025 16:21, Sukrut Bellary wrote: > > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > interface. The device family responds with 12-bit data, of which the LSB > > bits are transmitted by the lower resolution devices as 0. > > The unavailable bits are 0 in LSB. > > Shift is calculated per resolution and used in scaling and > > raw data read. > > > > Lets reuse the driver to support the family of devices with name > > ADC<bb><c>S<sss>, where > > * bb is the resolution in number of bits (8, 10, 12) > > * c is the number of channels (1, 2, 4, 8) > > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > and 101 for 1 MSPS) > > > > Complete datasheets are available at TI's website here: > > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > I tried looking up: > https://www.ti.com/lit/gpn/adc102s051.pdf Gaah - the IT folks keep messing with the URLs :( -> dropping .pdf in the url should get you to the pdf #facepalm :( (sidenote: that wasn't the case a year back :( - hopefully this will stay).
On Sat, Apr 12, 2025 at 02:12:53PM +0100, Jonathan Cameron wrote: > On Tue, 8 Apr 2025 06:21:20 -0700 > Sukrut Bellary <sbellary@baylibre.com> wrote: > > > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > interface. The device family responds with 12-bit data, of which the LSB > > bits are transmitted by the lower resolution devices as 0. > > The unavailable bits are 0 in LSB. > > Shift is calculated per resolution and used in scaling and > > raw data read. > > > > Lets reuse the driver to support the family of devices with name > > ADC<bb><c>S<sss>, where > > * bb is the resolution in number of bits (8, 10, 12) > > * c is the number of channels (1, 2, 4, 8) > > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > and 101 for 1 MSPS) > > > > Complete datasheets are available at TI's website here: > > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > > > Tested only with ti-adc102s051 on BegalePlay SBC. > > https://www.beagleboard.org/boards/beagleplay > > > > Co-developed-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > > --- > > Changes in v3: > > - used be16_to_cpu() for the endian conversion. > > - used config index enum while setting up the adc128_config[] > > > > - Link to v2: > > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > > > Changes in v2: > > - Arranged of_device_id and spi_device_id in numeric order. > > - Used enum to index into adc128_config. > > - Reorder adc128_config in alphabetical. > > - Include channel resolution information. > > - Shift is calculated per resolution and used in scaling and > > raw data read. > > > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > > --- > > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > > 1 file changed, 112 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > > index a456ea78462f..d4b76fd85abd 100644 > > --- a/drivers/iio/adc/ti-adc128s052.c > > +++ b/drivers/iio/adc/ti-adc128s052.c > > @@ -7,6 +7,22 @@ > > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > > + * > > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > + * interface. This driver supports the whole family of devices with a name > > + * ADC<bb><c>S<sss>, where > > + * bb is the resolution in number of bits (8, 10, 12) > > + * c is the number of channels (1, 2, 4, 8) > > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > + * and 101 for 1 MSPS) > > + * > > + * Complete datasheets are available at TI's website here: > > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > + * > > + * 8, 10, and 12 bits converters send 12-bit data with > > + * unavailable bits set to 0 in LSB. > > + * Shift is calculated per resolution and used in scaling and > > + * raw data read. > > */ > > > > #include <linux/err.h> > > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > > if (ret < 0) > > return ret; > > > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > > + return be16_to_cpu(*((__be16 *)adc->buffer)); > > I think we now have a convenient adc->buffer16 (probably introduced in Matti's > series. Sorry I missed the overlap of the two series until now. These part numbers > are too long and confusing to stick in my head! > > Matti took on maintaining that driver because he wanted to see any changes > that might affect the Rohm part it now supports. If anyone wants to volunteer > from the TI side of things that would be ideal - just send a patch adding to > the new MAINTAINERS entry. Thanks for the review. Sure, I can work on the TI side of things. > Jonathan > > > > }
On Mon, Apr 14, 2025 at 09:40:03AM +0300, Matti Vaittinen wrote: > On 08/04/2025 16:21, Sukrut Bellary wrote: > > The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > interface. The device family responds with 12-bit data, of which the LSB > > bits are transmitted by the lower resolution devices as 0. > > The unavailable bits are 0 in LSB. > > Shift is calculated per resolution and used in scaling and > > raw data read. > > > > Lets reuse the driver to support the family of devices with name > > ADC<bb><c>S<sss>, where > > * bb is the resolution in number of bits (8, 10, 12) > > * c is the number of channels (1, 2, 4, 8) > > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > and 101 for 1 MSPS) > > > > Complete datasheets are available at TI's website here: > > https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > I tried looking up: > https://www.ti.com/lit/gpn/adc102s051.pdf Sorry about that. I missed to check the link before submitting the patch series v3. > > > > Tested only with ti-adc102s051 on BegalePlay SBC. > > https://www.beagleboard.org/boards/beagleplay > > > > Co-developed-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com> > > --- > > Changes in v3: > > - used be16_to_cpu() for the endian conversion. > > - used config index enum while setting up the adc128_config[] > > > > - Link to v2: > > https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/ > > > > Changes in v2: > > - Arranged of_device_id and spi_device_id in numeric order. > > - Used enum to index into adc128_config. > > - Reorder adc128_config in alphabetical. > > - Include channel resolution information. > > - Shift is calculated per resolution and used in scaling and > > raw data read. > > > > - Link to v1: https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/ > > --- > > drivers/iio/adc/ti-adc128s052.c | 149 ++++++++++++++++++++++++-------- > > 1 file changed, 112 insertions(+), 37 deletions(-) > > > > Hi dee Ho, > > Thanks for improving this! It's always nice to be able to support more > devices with small(ish) changes! > > This looks good to me. I will take another, hopefully more in-depth look at > the rebased version when available though. > > I have just one comment for now, but it's not strictly related to this > change. If you wish to go the extra mile, then I'd appreciated it. If not, > then it can be re-worked later. Anyways, please, see below. > > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > > index a456ea78462f..d4b76fd85abd 100644 > > --- a/drivers/iio/adc/ti-adc128s052.c > > +++ b/drivers/iio/adc/ti-adc128s052.c > > @@ -7,6 +7,22 @@ > > * https://www.ti.com/lit/ds/symlink/adc128s052.pdf > > * https://www.ti.com/lit/ds/symlink/adc122s021.pdf > > * https://www.ti.com/lit/ds/symlink/adc124s021.pdf > > + * > > + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus > > + * interface. This driver supports the whole family of devices with a name > > + * ADC<bb><c>S<sss>, where > > + * bb is the resolution in number of bits (8, 10, 12) > > + * c is the number of channels (1, 2, 4, 8) > > + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS > > + * and 101 for 1 MSPS) > > + * > > + * Complete datasheets are available at TI's website here: > > + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf > > + * > > + * 8, 10, and 12 bits converters send 12-bit data with > > + * unavailable bits set to 0 in LSB. > > + * Shift is calculated per resolution and used in scaling and > > + * raw data read. > > */ > > #include <linux/err.h> > > @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > > if (ret < 0) > > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > > + return be16_to_cpu(*((__be16 *)adc->buffer)); > > } > > static int adc128_read_raw(struct iio_dev *indio_dev, > > @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > if (ret < 0) > > return ret; > > - *val = ret; > > + *val = (ret >> channel->scan_type.shift) & > > + GENMASK(channel->scan_type.realbits - 1, 0); > > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > > @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > return ret; > > *val = ret / 1000; > > - *val2 = 12; > > + *val2 = channel->scan_type.realbits; > > return IIO_VAL_FRACTIONAL_LOG2; > > default: > > @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev, > > } > > -#define ADC128_VOLTAGE_CHANNEL(num) \ > > - { \ > > - .type = IIO_VOLTAGE, \ > > - .indexed = 1, \ > > - .channel = (num), \ > > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > > +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \ > > + { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = (num), \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .scan_index = (num), \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = (real_bits), \ > > + .storagebits = (store_bits), \ > > + .shift = (12 - real_bits), \ > > + }, \ > > } > > -static const struct iio_chan_spec adc128s052_channels[] = { > > - ADC128_VOLTAGE_CHANNEL(0), > > - ADC128_VOLTAGE_CHANNEL(1), > > - ADC128_VOLTAGE_CHANNEL(2), > > - ADC128_VOLTAGE_CHANNEL(3), > > - ADC128_VOLTAGE_CHANNEL(4), > > - ADC128_VOLTAGE_CHANNEL(5), > > - ADC128_VOLTAGE_CHANNEL(6), > > - ADC128_VOLTAGE_CHANNEL(7), > > +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16) > > +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16) > > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16) > > + > > +static const struct iio_chan_spec adc082s021_channels[] = { > > + ADC082_VOLTAGE_CHANNEL(0), > > + ADC082_VOLTAGE_CHANNEL(1), > > +}; > > + > > +static const struct iio_chan_spec adc102s021_channels[] = { > > + ADC102_VOLTAGE_CHANNEL(0), > > + ADC102_VOLTAGE_CHANNEL(1), > > }; > > static const struct iio_chan_spec adc122s021_channels[] = { > > @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = { > > ADC128_VOLTAGE_CHANNEL(3), > > }; > > +static const struct iio_chan_spec adc128s052_channels[] = { > > + ADC128_VOLTAGE_CHANNEL(0), > > + ADC128_VOLTAGE_CHANNEL(1), > > + ADC128_VOLTAGE_CHANNEL(2), > > + ADC128_VOLTAGE_CHANNEL(3), > > + ADC128_VOLTAGE_CHANNEL(4), > > + ADC128_VOLTAGE_CHANNEL(5), > > + ADC128_VOLTAGE_CHANNEL(6), > > + ADC128_VOLTAGE_CHANNEL(7), > > +}; > > + > > +enum adc128_configuration_index { > > + ADC128_CONFIG_INDEX_082S, > > + ADC128_CONFIG_INDEX_102S, > > + ADC128_CONFIG_INDEX_122S, > > + ADC128_CONFIG_INDEX_124S, > > + ADC128_CONFIG_INDEX_128S, > > +}; > > I like the fact you added these indexes as it makes this a lot clearer. > But... > > > + > > static const struct adc128_configuration adc128_config[] = { > > - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) }, > > - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) }, > > - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) }, > > + [ADC128_CONFIG_INDEX_082S] = { > > + .channels = adc082s021_channels, > > + .num_channels = ARRAY_SIZE(adc082s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_102S] = { > > + .channels = adc102s021_channels, > > + .num_channels = ARRAY_SIZE(adc102s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_122S] = { > > + .channels = adc122s021_channels, > > + .num_channels = ARRAY_SIZE(adc122s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_124S] = { > > + .channels = adc124s021_channels, > > + .num_channels = ARRAY_SIZE(adc124s021_channels) > > + }, > > + [ADC128_CONFIG_INDEX_128S] = { > > + .channels = adc128s052_channels, > > + .num_channels = ARRAY_SIZE(adc128s052_channels) > > + }, > > }; > > ... I don't really love this array. I believe the code would be clearer if > this array was changed to individual structs because ... > > > static const struct iio_info adc128_info = { > > @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi) > > } > > static const struct of_device_id adc128_of_match[] = { > > - { .compatible = "ti,adc128s052", .data = &adc128_config[0] }, > > - { .compatible = "ti,adc122s021", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc122s051", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc122s101", .data = &adc128_config[1] }, > > - { .compatible = "ti,adc124s021", .data = &adc128_config[2] }, > > - { .compatible = "ti,adc124s051", .data = &adc128_config[2] }, > > - { .compatible = "ti,adc124s101", .data = &adc128_config[2] }, > > + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, > > + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, > > + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, > > + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, > > + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] }, > > ... here we could then directly refer to individual structs. That way we > would not need to define the names for the array indexes (for clarity), or > look up the individual array members based on magic numbers. Thanks for the review. yes, I will take care of this in v4. > > { /* sentinel */ }, > > }; > > MODULE_DEVICE_TABLE(of, adc128_of_match); > > Yours, > -- Matti
On 16/04/2025 01:20, Sukrut Bellary wrote: > On Sat, Apr 12, 2025 at 02:12:53PM +0100, Jonathan Cameron wrote: >> On Tue, 8 Apr 2025 06:21:20 -0700 >> Sukrut Bellary <sbellary@baylibre.com> wrote: >> >> Matti took on maintaining that driver because he wanted to see any changes >> that might affect the Rohm part it now supports. If anyone wants to volunteer >> from the TI side of things that would be ideal - just send a patch adding to >> the new MAINTAINERS entry. > > Thanks for the review. > Sure, I can work on the TI side of things. Thanks Sukrut! That's great as I have no TI devices to run tests with. :) Yours, -- Matti
diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c index a456ea78462f..d4b76fd85abd 100644 --- a/drivers/iio/adc/ti-adc128s052.c +++ b/drivers/iio/adc/ti-adc128s052.c @@ -7,6 +7,22 @@ * https://www.ti.com/lit/ds/symlink/adc128s052.pdf * https://www.ti.com/lit/ds/symlink/adc122s021.pdf * https://www.ti.com/lit/ds/symlink/adc124s021.pdf + * + * The adcxx4s communicates with a host processor via an SPI/Microwire Bus + * interface. This driver supports the whole family of devices with a name + * ADC<bb><c>S<sss>, where + * bb is the resolution in number of bits (8, 10, 12) + * c is the number of channels (1, 2, 4, 8) + * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS + * and 101 for 1 MSPS) + * + * Complete datasheets are available at TI's website here: + * https://www.ti.com/lit/gpn/adc<bb><c>s<sss>.pdf + * + * 8, 10, and 12 bits converters send 12-bit data with + * unavailable bits set to 0 in LSB. + * Shift is calculated per resolution and used in scaling and + * raw data read. */ #include <linux/err.h> @@ -53,7 +69,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) if (ret < 0) return ret; - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); + return be16_to_cpu(*((__be16 *)adc->buffer)); } static int adc128_read_raw(struct iio_dev *indio_dev, @@ -70,7 +86,8 @@ static int adc128_read_raw(struct iio_dev *indio_dev, if (ret < 0) return ret; - *val = ret; + *val = (ret >> channel->scan_type.shift) & + GENMASK(channel->scan_type.realbits - 1, 0); return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: @@ -80,7 +97,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev, return ret; *val = ret / 1000; - *val2 = 12; + *val2 = channel->scan_type.realbits; return IIO_VAL_FRACTIONAL_LOG2; default: @@ -89,24 +106,34 @@ static int adc128_read_raw(struct iio_dev *indio_dev, } -#define ADC128_VOLTAGE_CHANNEL(num) \ - { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .channel = (num), \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits, store_bits) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (num), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = (num), \ + .scan_type = { \ + .sign = 'u', \ + .realbits = (real_bits), \ + .storagebits = (store_bits), \ + .shift = (12 - real_bits), \ + }, \ } -static const struct iio_chan_spec adc128s052_channels[] = { - ADC128_VOLTAGE_CHANNEL(0), - ADC128_VOLTAGE_CHANNEL(1), - ADC128_VOLTAGE_CHANNEL(2), - ADC128_VOLTAGE_CHANNEL(3), - ADC128_VOLTAGE_CHANNEL(4), - ADC128_VOLTAGE_CHANNEL(5), - ADC128_VOLTAGE_CHANNEL(6), - ADC128_VOLTAGE_CHANNEL(7), +#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8, 16) +#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10, 16) +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12, 16) + +static const struct iio_chan_spec adc082s021_channels[] = { + ADC082_VOLTAGE_CHANNEL(0), + ADC082_VOLTAGE_CHANNEL(1), +}; + +static const struct iio_chan_spec adc102s021_channels[] = { + ADC102_VOLTAGE_CHANNEL(0), + ADC102_VOLTAGE_CHANNEL(1), }; static const struct iio_chan_spec adc122s021_channels[] = { @@ -121,10 +148,46 @@ static const struct iio_chan_spec adc124s021_channels[] = { ADC128_VOLTAGE_CHANNEL(3), }; +static const struct iio_chan_spec adc128s052_channels[] = { + ADC128_VOLTAGE_CHANNEL(0), + ADC128_VOLTAGE_CHANNEL(1), + ADC128_VOLTAGE_CHANNEL(2), + ADC128_VOLTAGE_CHANNEL(3), + ADC128_VOLTAGE_CHANNEL(4), + ADC128_VOLTAGE_CHANNEL(5), + ADC128_VOLTAGE_CHANNEL(6), + ADC128_VOLTAGE_CHANNEL(7), +}; + +enum adc128_configuration_index { + ADC128_CONFIG_INDEX_082S, + ADC128_CONFIG_INDEX_102S, + ADC128_CONFIG_INDEX_122S, + ADC128_CONFIG_INDEX_124S, + ADC128_CONFIG_INDEX_128S, +}; + static const struct adc128_configuration adc128_config[] = { - { adc128s052_channels, ARRAY_SIZE(adc128s052_channels) }, - { adc122s021_channels, ARRAY_SIZE(adc122s021_channels) }, - { adc124s021_channels, ARRAY_SIZE(adc124s021_channels) }, + [ADC128_CONFIG_INDEX_082S] = { + .channels = adc082s021_channels, + .num_channels = ARRAY_SIZE(adc082s021_channels) + }, + [ADC128_CONFIG_INDEX_102S] = { + .channels = adc102s021_channels, + .num_channels = ARRAY_SIZE(adc102s021_channels) + }, + [ADC128_CONFIG_INDEX_122S] = { + .channels = adc122s021_channels, + .num_channels = ARRAY_SIZE(adc122s021_channels) + }, + [ADC128_CONFIG_INDEX_124S] = { + .channels = adc124s021_channels, + .num_channels = ARRAY_SIZE(adc124s021_channels) + }, + [ADC128_CONFIG_INDEX_128S] = { + .channels = adc128s052_channels, + .num_channels = ARRAY_SIZE(adc128s052_channels) + }, }; static const struct iio_info adc128_info = { @@ -177,31 +240,43 @@ static int adc128_probe(struct spi_device *spi) } static const struct of_device_id adc128_of_match[] = { - { .compatible = "ti,adc128s052", .data = &adc128_config[0] }, - { .compatible = "ti,adc122s021", .data = &adc128_config[1] }, - { .compatible = "ti,adc122s051", .data = &adc128_config[1] }, - { .compatible = "ti,adc122s101", .data = &adc128_config[1] }, - { .compatible = "ti,adc124s021", .data = &adc128_config[2] }, - { .compatible = "ti,adc124s051", .data = &adc128_config[2] }, - { .compatible = "ti,adc124s101", .data = &adc128_config[2] }, + { .compatible = "ti,adc082s021", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, + { .compatible = "ti,adc082s051", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, + { .compatible = "ti,adc082s101", .data = &adc128_config[ADC128_CONFIG_INDEX_082S] }, + { .compatible = "ti,adc102s021", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, + { .compatible = "ti,adc102s051", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, + { .compatible = "ti,adc102s101", .data = &adc128_config[ADC128_CONFIG_INDEX_102S] }, + { .compatible = "ti,adc122s021", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, + { .compatible = "ti,adc122s051", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, + { .compatible = "ti,adc122s101", .data = &adc128_config[ADC128_CONFIG_INDEX_122S] }, + { .compatible = "ti,adc124s021", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, + { .compatible = "ti,adc124s051", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, + { .compatible = "ti,adc124s101", .data = &adc128_config[ADC128_CONFIG_INDEX_124S] }, + { .compatible = "ti,adc128s052", .data = &adc128_config[ADC128_CONFIG_INDEX_128S] }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, adc128_of_match); static const struct spi_device_id adc128_id[] = { - { "adc128s052", (kernel_ulong_t)&adc128_config[0] }, - { "adc122s021", (kernel_ulong_t)&adc128_config[1] }, - { "adc122s051", (kernel_ulong_t)&adc128_config[1] }, - { "adc122s101", (kernel_ulong_t)&adc128_config[1] }, - { "adc124s021", (kernel_ulong_t)&adc128_config[2] }, - { "adc124s051", (kernel_ulong_t)&adc128_config[2] }, - { "adc124s101", (kernel_ulong_t)&adc128_config[2] }, + { "adc082s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, + { "adc082s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, + { "adc082s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_082S] }, + { "adc102s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, + { "adc102s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, + { "adc102s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_102S] }, + { "adc122s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, + { "adc122s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, + { "adc122s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_122S] }, + { "adc124s021", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, + { "adc124s051", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, + { "adc124s101", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, + { "adc128s052", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_128S] }, { } }; MODULE_DEVICE_TABLE(spi, adc128_id); static const struct acpi_device_id adc128_acpi_match[] = { - { "AANT1280", (kernel_ulong_t)&adc128_config[2] }, + { "AANT1280", (kernel_ulong_t)&adc128_config[ADC128_CONFIG_INDEX_124S] }, { } }; MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);