* [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928
@ 2019-12-10 15:08 Daniel Junho
2019-12-10 15:08 ` [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines Daniel Junho
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Daniel Junho @ 2019-12-10 15:08 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Michael Hennerich, Stefan Popa,
linux-iio, linux-kernel, lkcamp
This patch adds support to AD7928 family on the implementation of the
AD7923.
The ad7928 is software compatible with the ad7923.
The ad7908 and ad7918 are the 8 and 10-bit versions of the ad7928.
Change log v1 -> v2
- Split the v1 into 4 commits.
- Removes a buggy line committed unintentionally
Daniel Junho (4):
iio: adc: ad7923: Remove the unused defines
iio: adc: ad7923: Fix checkpatch warning
iio: adc: ad7923: Add of_device_id table
iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928
drivers/iio/adc/ad7923.c | 78 +++++++++++++++++++++++++++++++---------
1 file changed, 62 insertions(+), 16 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines 2019-12-10 15:08 [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928 Daniel Junho @ 2019-12-10 15:08 ` Daniel Junho 2019-12-11 7:26 ` Ardelean, Alexandru 2019-12-10 15:08 ` [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning Daniel Junho ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Daniel Junho @ 2019-12-10 15:08 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp Removes the unused define AD7923_CHANNEL_x from the code. Signed-off-by: Daniel Junho <djunho@gmail.com> --- drivers/iio/adc/ad7923.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c index 3212eb4c0f25..969c06b7d2b7 100644 --- a/drivers/iio/adc/ad7923.c +++ b/drivers/iio/adc/ad7923.c @@ -29,10 +29,6 @@ #define AD7923_PM_MODE_AS (1) /* auto shutdown */ #define AD7923_PM_MODE_FS (2) /* full shutdown */ #define AD7923_PM_MODE_OPS (3) /* normal operation */ -#define AD7923_CHANNEL_0 (0) /* analog input 0 */ -#define AD7923_CHANNEL_1 (1) /* analog input 1 */ -#define AD7923_CHANNEL_2 (2) /* analog input 2 */ -#define AD7923_CHANNEL_3 (3) /* analog input 3 */ #define AD7923_SEQUENCE_OFF (0) /* no sequence fonction */ #define AD7923_SEQUENCE_PROTECT (2) /* no interrupt write cycle */ #define AD7923_SEQUENCE_ON (3) /* continuous sequence */ -- 2.24.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines 2019-12-10 15:08 ` [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines Daniel Junho @ 2019-12-11 7:26 ` Ardelean, Alexandru 2019-12-15 15:47 ` Jonathan Cameron 0 siblings, 1 reply; 12+ messages in thread From: Ardelean, Alexandru @ 2019-12-11 7:26 UTC (permalink / raw) To: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, jic23, lars On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > [External] > > Removes the unused define AD7923_CHANNEL_x from the code. > > Signed-off-by: Daniel Junho <djunho@gmail.com> > --- > drivers/iio/adc/ad7923.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > index 3212eb4c0f25..969c06b7d2b7 100644 > --- a/drivers/iio/adc/ad7923.c > +++ b/drivers/iio/adc/ad7923.c > @@ -29,10 +29,6 @@ > #define AD7923_PM_MODE_AS (1) /* auto shutdown */ > #define AD7923_PM_MODE_FS (2) /* full shutdown */ > #define AD7923_PM_MODE_OPS (3) /* normal operation */ > -#define AD7923_CHANNEL_0 (0) /* analog input 0 */ > -#define AD7923_CHANNEL_1 (1) /* analog input 1 */ > -#define AD7923_CHANNEL_2 (2) /* analog input 2 */ > -#define AD7923_CHANNEL_3 (3) /* analog input 3 */ The fact that is not used, is not a problem really. This can serve as a "shortcut" for the datasheet. We can leave this as-is [1], or remove it as you do [2], or an alternative would be [3]: #define AD7923_CHANNEL(x) (x) /* analog channel input */ I don't mind either of these 3 versions ([1], [2] or [3]). I'll leave it to Jonathan's preference. > #define AD7923_SEQUENCE_OFF (0) /* no sequence fonction > */ > #define AD7923_SEQUENCE_PROTECT (2) /* no interrupt > write cycle */ > #define AD7923_SEQUENCE_ON (3) /* continuous sequence */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines 2019-12-11 7:26 ` Ardelean, Alexandru @ 2019-12-15 15:47 ` Jonathan Cameron 0 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2019-12-15 15:47 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, lars On Wed, 11 Dec 2019 07:26:07 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > > [External] > > > > Removes the unused define AD7923_CHANNEL_x from the code. > > > > Signed-off-by: Daniel Junho <djunho@gmail.com> > > --- > > drivers/iio/adc/ad7923.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > > index 3212eb4c0f25..969c06b7d2b7 100644 > > --- a/drivers/iio/adc/ad7923.c > > +++ b/drivers/iio/adc/ad7923.c > > @@ -29,10 +29,6 @@ > > #define AD7923_PM_MODE_AS (1) /* auto shutdown */ > > #define AD7923_PM_MODE_FS (2) /* full shutdown */ > > #define AD7923_PM_MODE_OPS (3) /* normal operation */ > > -#define AD7923_CHANNEL_0 (0) /* analog input 0 */ > > -#define AD7923_CHANNEL_1 (1) /* analog input 1 */ > > -#define AD7923_CHANNEL_2 (2) /* analog input 2 */ > > -#define AD7923_CHANNEL_3 (3) /* analog input 3 */ > > The fact that is not used, is not a problem really. > This can serve as a "shortcut" for the datasheet. In some cases I would agree, but here adding a definition to go from a channel number in text to an actual number doesn't add anything. If there mapping was more complex then it would be reasonable to have defines, but here I can't see the point. So let's keep the patch to drop them. I'll pick it up whilst the comments on patch 4 have been addressed. Thanks, Jonathan > > We can leave this as-is [1], or remove it as you do [2], or an alternative > would be [3]: > #define AD7923_CHANNEL(x) (x) /* analog channel input */ > > I don't mind either of these 3 versions ([1], [2] or [3]). > > I'll leave it to Jonathan's preference. > > > #define AD7923_SEQUENCE_OFF (0) /* no sequence fonction > > */ > > #define AD7923_SEQUENCE_PROTECT (2) /* no interrupt > > write cycle */ > > #define AD7923_SEQUENCE_ON (3) /* continuous sequence */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning 2019-12-10 15:08 [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928 Daniel Junho 2019-12-10 15:08 ` [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines Daniel Junho @ 2019-12-10 15:08 ` Daniel Junho 2019-12-11 7:26 ` Ardelean, Alexandru 2019-12-10 15:08 ` [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table Daniel Junho 2019-12-10 15:08 ` [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho 3 siblings, 1 reply; 12+ messages in thread From: Daniel Junho @ 2019-12-10 15:08 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp Fix checkpatch warning: WARNING: Prefer 'unsigned int' to bare use of 'unsigned' +static int ad7923_scan_direct(struct ad7923_state *st, unsigned ch) Signed-off-by: Daniel Junho <djunho@gmail.com> --- drivers/iio/adc/ad7923.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c index 969c06b7d2b7..e535cec9fc02 100644 --- a/drivers/iio/adc/ad7923.c +++ b/drivers/iio/adc/ad7923.c @@ -184,7 +184,7 @@ static irqreturn_t ad7923_trigger_handler(int irq, void *p) return IRQ_HANDLED; } -static int ad7923_scan_direct(struct ad7923_state *st, unsigned ch) +static int ad7923_scan_direct(struct ad7923_state *st, unsigned int ch) { int ret, cmd; -- 2.24.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning 2019-12-10 15:08 ` [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning Daniel Junho @ 2019-12-11 7:26 ` Ardelean, Alexandru 0 siblings, 0 replies; 12+ messages in thread From: Ardelean, Alexandru @ 2019-12-11 7:26 UTC (permalink / raw) To: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, jic23, lars On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > [External] > > Fix checkpatch warning: > WARNING: Prefer 'unsigned int' to bare use of 'unsigned' > +static int ad7923_scan_direct(struct ad7923_state *st, unsigned ch) > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Signed-off-by: Daniel Junho <djunho@gmail.com> > --- > drivers/iio/adc/ad7923.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > index 969c06b7d2b7..e535cec9fc02 100644 > --- a/drivers/iio/adc/ad7923.c > +++ b/drivers/iio/adc/ad7923.c > @@ -184,7 +184,7 @@ static irqreturn_t ad7923_trigger_handler(int irq, > void *p) > return IRQ_HANDLED; > } > > -static int ad7923_scan_direct(struct ad7923_state *st, unsigned ch) > +static int ad7923_scan_direct(struct ad7923_state *st, unsigned int ch) > { > int ret, cmd; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table 2019-12-10 15:08 [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928 Daniel Junho 2019-12-10 15:08 ` [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines Daniel Junho 2019-12-10 15:08 ` [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning Daniel Junho @ 2019-12-10 15:08 ` Daniel Junho 2019-12-11 7:27 ` Ardelean, Alexandru 2019-12-10 15:08 ` [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho 3 siblings, 1 reply; 12+ messages in thread From: Daniel Junho @ 2019-12-10 15:08 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp Accomplish device tree compatibility to driver AD7923 by adding of_device_id table and making a subsequent call to MODULE_DEVICE_TABLE. Signed-off-by: Daniel Junho <djunho@gmail.com> --- drivers/iio/adc/ad7923.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c index e535cec9fc02..6d56fa0b9e30 100644 --- a/drivers/iio/adc/ad7923.c +++ b/drivers/iio/adc/ad7923.c @@ -348,9 +348,19 @@ static const struct spi_device_id ad7923_id[] = { }; MODULE_DEVICE_TABLE(spi, ad7923_id); +static const struct of_device_id ad7923_of_match[] = { + { .compatible = "adi,ad7904", }, + { .compatible = "adi,ad7914", }, + { .compatible = "adi,ad7923", }, + { .compatible = "adi,ad7924", }, + { }, +}; +MODULE_DEVICE_TABLE(of, ad7923_of_match); + static struct spi_driver ad7923_driver = { .driver = { .name = "ad7923", + .of_match_table = ad7923_of_match, }, .probe = ad7923_probe, .remove = ad7923_remove, -- 2.24.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table 2019-12-10 15:08 ` [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table Daniel Junho @ 2019-12-11 7:27 ` Ardelean, Alexandru 0 siblings, 0 replies; 12+ messages in thread From: Ardelean, Alexandru @ 2019-12-11 7:27 UTC (permalink / raw) To: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, jic23, lars On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > [External] > > Accomplish device tree compatibility to driver AD7923 > by adding of_device_id table and making a subsequent call to > MODULE_DEVICE_TABLE. > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Signed-off-by: Daniel Junho <djunho@gmail.com> > --- > drivers/iio/adc/ad7923.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > index e535cec9fc02..6d56fa0b9e30 100644 > --- a/drivers/iio/adc/ad7923.c > +++ b/drivers/iio/adc/ad7923.c > @@ -348,9 +348,19 @@ static const struct spi_device_id ad7923_id[] = { > }; > MODULE_DEVICE_TABLE(spi, ad7923_id); > > +static const struct of_device_id ad7923_of_match[] = { > + { .compatible = "adi,ad7904", }, > + { .compatible = "adi,ad7914", }, > + { .compatible = "adi,ad7923", }, > + { .compatible = "adi,ad7924", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, ad7923_of_match); > + > static struct spi_driver ad7923_driver = { > .driver = { > .name = "ad7923", > + .of_match_table = ad7923_of_match, > }, > .probe = ad7923_probe, > .remove = ad7923_remove, ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 2019-12-10 15:08 [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928 Daniel Junho ` (2 preceding siblings ...) 2019-12-10 15:08 ` [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table Daniel Junho @ 2019-12-10 15:08 ` Daniel Junho 2019-12-11 7:46 ` Ardelean, Alexandru 3 siblings, 1 reply; 12+ messages in thread From: Daniel Junho @ 2019-12-10 15:08 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp The ad7928 is software compatible with the ad7923. The ad7908 and ad7918 are the 8 and 10-bit versions of the ad7928. Signed-off-by: Daniel Junho <djunho@gmail.com> --- drivers/iio/adc/ad7923.c | 62 +++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 11 deletions(-) diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c index 6d56fa0b9e30..ae807fcaad76 100644 --- a/drivers/iio/adc/ad7923.c +++ b/drivers/iio/adc/ad7923.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * AD7904/AD7914/AD7923/AD7924 SPI ADC driver + * AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 SPI ADC driver * * Copyright 2011 Analog Devices Inc (from AD7923 Driver) * Copyright 2012 CS Systemes d'Information @@ -29,11 +29,14 @@ #define AD7923_PM_MODE_AS (1) /* auto shutdown */ #define AD7923_PM_MODE_FS (2) /* full shutdown */ #define AD7923_PM_MODE_OPS (3) /* normal operation */ -#define AD7923_SEQUENCE_OFF (0) /* no sequence fonction */ -#define AD7923_SEQUENCE_PROTECT (2) /* no interrupt write cycle */ -#define AD7923_SEQUENCE_ON (3) /* continuous sequence */ -#define AD7923_MAX_CHAN 4 +#define AD7923_SEQUENCE_OFF (0x00) /* no sequence function */ +#define AD7923_SEQUENCE_SHADOW (0x01) /* use the shadow register + * for programming + * (only AD7908/7918/7928) + */ +#define AD7923_SEQUENCE_PROTECT (0x10) /* no interrupt write cycle */ +#define AD7923_SEQUENCE_ON (0x11) /* continuous sequence */ #define AD7923_PM_MODE_WRITE(mode) ((mode) << 4) /* write mode */ #define AD7923_CHANNEL_WRITE(channel) ((channel) << 6) /* write channel */ @@ -74,6 +77,9 @@ enum ad7923_id { AD7904, AD7914, AD7924, + AD7908, + AD7918, + AD7928 }; #define AD7923_V_CHAN(index, bits) \ @@ -93,7 +99,7 @@ enum ad7923_id { }, \ } -#define DECLARE_AD7923_CHANNELS(name, bits) \ +#define DECLARE_AD7923_4CHANNELS(name, bits) \ const struct iio_chan_spec name ## _channels[] = { \ AD7923_V_CHAN(0, bits), \ AD7923_V_CHAN(1, bits), \ @@ -102,9 +108,25 @@ const struct iio_chan_spec name ## _channels[] = { \ IIO_CHAN_SOFT_TIMESTAMP(4), \ } -static DECLARE_AD7923_CHANNELS(ad7904, 8); -static DECLARE_AD7923_CHANNELS(ad7914, 10); -static DECLARE_AD7923_CHANNELS(ad7924, 12); +#define DECLARE_AD7923_8CHANNELS(name, bits) \ +const struct iio_chan_spec name ## _channels[] = { \ + AD7923_V_CHAN(0, bits), \ + AD7923_V_CHAN(1, bits), \ + AD7923_V_CHAN(2, bits), \ + AD7923_V_CHAN(3, bits), \ + AD7923_V_CHAN(4, bits), \ + AD7923_V_CHAN(5, bits), \ + AD7923_V_CHAN(6, bits), \ + AD7923_V_CHAN(7, bits), \ + IIO_CHAN_SOFT_TIMESTAMP(8), \ +} + +static DECLARE_AD7923_4CHANNELS(ad7904, 8); +static DECLARE_AD7923_4CHANNELS(ad7914, 10); +static DECLARE_AD7923_4CHANNELS(ad7924, 12); +static DECLARE_AD7923_8CHANNELS(ad7908, 8); +static DECLARE_AD7923_8CHANNELS(ad7918, 10); +static DECLARE_AD7923_8CHANNELS(ad7928, 12); static const struct ad7923_chip_info ad7923_chip_info[] = { [AD7904] = { @@ -119,6 +141,18 @@ static const struct ad7923_chip_info ad7923_chip_info[] = { .channels = ad7924_channels, .num_channels = ARRAY_SIZE(ad7924_channels), }, + [AD7908] = { + .channels = ad7908_channels, + .num_channels = ARRAY_SIZE(ad7908_channels), + }, + [AD7918] = { + .channels = ad7918_channels, + .num_channels = ARRAY_SIZE(ad7918_channels), + }, + [AD7928] = { + .channels = ad7928_channels, + .num_channels = ARRAY_SIZE(ad7928_channels), + }, }; /** @@ -131,7 +165,7 @@ static int ad7923_update_scan_mode(struct iio_dev *indio_dev, int i, cmd, len; len = 0; - for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN) { + for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) { cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) | AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | st->settings; @@ -344,6 +378,9 @@ static const struct spi_device_id ad7923_id[] = { {"ad7914", AD7914}, {"ad7923", AD7924}, {"ad7924", AD7924}, + {"ad7908", AD7908}, + {"ad7918", AD7918}, + {"ad7928", AD7928}, {} }; MODULE_DEVICE_TABLE(spi, ad7923_id); @@ -353,6 +390,9 @@ static const struct of_device_id ad7923_of_match[] = { { .compatible = "adi,ad7914", }, { .compatible = "adi,ad7923", }, { .compatible = "adi,ad7924", }, + { .compatible = "adi,ad7908", }, + { .compatible = "adi,ad7918", }, + { .compatible = "adi,ad7928", }, { }, }; MODULE_DEVICE_TABLE(of, ad7923_of_match); @@ -370,5 +410,5 @@ module_spi_driver(ad7923_driver); MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>"); -MODULE_DESCRIPTION("Analog Devices AD7904/AD7914/AD7923/AD7924 ADC"); +MODULE_DESCRIPTION("Analog Devices AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 ADC"); MODULE_LICENSE("GPL v2"); -- 2.24.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 2019-12-10 15:08 ` [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho @ 2019-12-11 7:46 ` Ardelean, Alexandru 2019-12-15 15:43 ` Jonathan Cameron 0 siblings, 1 reply; 12+ messages in thread From: Ardelean, Alexandru @ 2019-12-11 7:46 UTC (permalink / raw) To: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, jic23, lars On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > [External] > > The ad7928 is software compatible with the ad7923. > The ad7908 and ad7918 are the 8 and 10-bit versions of the ad7928. > The patch looks good. Comments mostly about namimg. One about channel numbers. > Signed-off-by: Daniel Junho <djunho@gmail.com> > --- > drivers/iio/adc/ad7923.c | 62 +++++++++++++++++++++++++++++++++------- > 1 file changed, 51 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > index 6d56fa0b9e30..ae807fcaad76 100644 > --- a/drivers/iio/adc/ad7923.c > +++ b/drivers/iio/adc/ad7923.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * AD7904/AD7914/AD7923/AD7924 SPI ADC driver > + * AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 SPI ADC driver > * > * Copyright 2011 Analog Devices Inc (from AD7923 Driver) > * Copyright 2012 CS Systemes d'Information > @@ -29,11 +29,14 @@ > #define AD7923_PM_MODE_AS (1) /* auto shutdown */ > #define AD7923_PM_MODE_FS (2) /* full shutdown */ > #define AD7923_PM_MODE_OPS (3) /* normal operation */ > -#define AD7923_SEQUENCE_OFF (0) /* no sequence fonction > */ > -#define AD7923_SEQUENCE_PROTECT (2) /* no interrupt > write cycle */ > -#define AD7923_SEQUENCE_ON (3) /* continuous sequence */ > > -#define AD7923_MAX_CHAN 4 > +#define AD7923_SEQUENCE_OFF (0x00) /* no sequence function > */ > +#define AD7923_SEQUENCE_SHADOW (0x01) /* use the shadow > register > + * for programming > + * (only AD7908/7918/7928) [1] Since this reg is only available on these 3 chips, you could rename this to AD7908_SEQUENCE_SHADOW and use it for these 3 chips only. That way, you don't need to mention via comment "(only AD7908/7918/7928)" > + */ > +#define AD7923_SEQUENCE_PROTECT (0x10) /* no interrupt > write cycle */ > +#define AD7923_SEQUENCE_ON (0x11) /* continuous sequence */ > > #define AD7923_PM_MODE_WRITE(mode) ((mode) << 4) /* write mode */ > #define AD7923_CHANNEL_WRITE(channel) ((channel) << 6) /* write > channel */ > @@ -74,6 +77,9 @@ enum ad7923_id { > AD7904, > AD7914, > AD7924, > + AD7908, > + AD7918, > + AD7928 > }; > > #define AD7923_V_CHAN(index, bits) \ > @@ -93,7 +99,7 @@ enum ad7923_id { > }, \ > } > > -#define DECLARE_AD7923_CHANNELS(name, bits) \ > +#define DECLARE_AD7923_4CHANNELS(name, bits) \ > const struct iio_chan_spec name ## _channels[] = { \ > AD7923_V_CHAN(0, bits), \ > AD7923_V_CHAN(1, bits), \ > @@ -102,9 +108,25 @@ const struct iio_chan_spec name ## _channels[] = { \ > IIO_CHAN_SOFT_TIMESTAMP(4), \ > } > > -static DECLARE_AD7923_CHANNELS(ad7904, 8); > -static DECLARE_AD7923_CHANNELS(ad7914, 10); > -static DECLARE_AD7923_CHANNELS(ad7924, 12); > +#define DECLARE_AD7923_8CHANNELS(name, bits) \ Same comments as [1], you could leave DECLARE_AD7923_CHANNELS and declare DECLARE_AD7908_CHANNELS which has 8 channels and use for the 3 new chips. > +const struct iio_chan_spec name ## _channels[] = { \ > + AD7923_V_CHAN(0, bits), \ > + AD7923_V_CHAN(1, bits), \ > + AD7923_V_CHAN(2, bits), \ > + AD7923_V_CHAN(3, bits), \ > + AD7923_V_CHAN(4, bits), \ > + AD7923_V_CHAN(5, bits), \ > + AD7923_V_CHAN(6, bits), \ > + AD7923_V_CHAN(7, bits), \ > + IIO_CHAN_SOFT_TIMESTAMP(8), \ > +} > + > +static DECLARE_AD7923_4CHANNELS(ad7904, 8); > +static DECLARE_AD7923_4CHANNELS(ad7914, 10); > +static DECLARE_AD7923_4CHANNELS(ad7924, 12); > +static DECLARE_AD7923_8CHANNELS(ad7908, 8); > +static DECLARE_AD7923_8CHANNELS(ad7918, 10); > +static DECLARE_AD7923_8CHANNELS(ad7928, 12); > > static const struct ad7923_chip_info ad7923_chip_info[] = { > [AD7904] = { > @@ -119,6 +141,18 @@ static const struct ad7923_chip_info > ad7923_chip_info[] = { > .channels = ad7924_channels, > .num_channels = ARRAY_SIZE(ad7924_channels), > }, > + [AD7908] = { > + .channels = ad7908_channels, > + .num_channels = ARRAY_SIZE(ad7908_channels), > + }, > + [AD7918] = { > + .channels = ad7918_channels, > + .num_channels = ARRAY_SIZE(ad7918_channels), > + }, > + [AD7928] = { > + .channels = ad7928_channels, > + .num_channels = ARRAY_SIZE(ad7928_channels), > + }, > }; > > /** > @@ -131,7 +165,7 @@ static int ad7923_update_scan_mode(struct iio_dev > *indio_dev, > int i, cmd, len; > > len = 0; > - for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN) { > + for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) I got the idea about 'indio_dev->num_channel - 1'. This makes the assumption that the number of IIO channels is always HW- channels + 1 timestamp channel [which for now is true]. I'm not sure if this is ok. I think Jonathan would be a bit better here to comment. > { > cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) | > AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | > st->settings; > @@ -344,6 +378,9 @@ static const struct spi_device_id ad7923_id[] = { > {"ad7914", AD7914}, > {"ad7923", AD7924}, > {"ad7924", AD7924}, > + {"ad7908", AD7908}, > + {"ad7918", AD7918}, > + {"ad7928", AD7928}, > {} > }; > MODULE_DEVICE_TABLE(spi, ad7923_id); > @@ -353,6 +390,9 @@ static const struct of_device_id ad7923_of_match[] = > { > { .compatible = "adi,ad7914", }, > { .compatible = "adi,ad7923", }, > { .compatible = "adi,ad7924", }, > + { .compatible = "adi,ad7908", }, > + { .compatible = "adi,ad7918", }, > + { .compatible = "adi,ad7928", }, > { }, > }; > MODULE_DEVICE_TABLE(of, ad7923_of_match); > @@ -370,5 +410,5 @@ module_spi_driver(ad7923_driver); > > MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>"); > -MODULE_DESCRIPTION("Analog Devices AD7904/AD7914/AD7923/AD7924 ADC"); > +MODULE_DESCRIPTION("Analog Devices > AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 ADC"); This description is becoming long. Maybe simplify to MODULE_DESCRIPTION("Analog Devices AD7923 and similar ADC"); > MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 2019-12-11 7:46 ` Ardelean, Alexandru @ 2019-12-15 15:43 ` Jonathan Cameron 2019-12-16 11:59 ` Daniel Junho 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Cameron @ 2019-12-15 15:43 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, lkcamp, linux-iio, djunho, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, lars On Wed, 11 Dec 2019 07:46:04 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > > [External] > > > > The ad7928 is software compatible with the ad7923. > > The ad7908 and ad7918 are the 8 and 10-bit versions of the ad7928. > > > > The patch looks good. > Comments mostly about namimg. > One about channel numbers. > > > > Signed-off-by: Daniel Junho <djunho@gmail.com> > > --- > > drivers/iio/adc/ad7923.c | 62 +++++++++++++++++++++++++++++++++------- > > 1 file changed, 51 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > > index 6d56fa0b9e30..ae807fcaad76 100644 > > --- a/drivers/iio/adc/ad7923.c > > +++ b/drivers/iio/adc/ad7923.c > > @@ -1,6 +1,6 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > /* > > - * AD7904/AD7914/AD7923/AD7924 SPI ADC driver > > + * AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 SPI ADC driver > > * > > * Copyright 2011 Analog Devices Inc (from AD7923 Driver) > > * Copyright 2012 CS Systemes d'Information > > @@ -29,11 +29,14 @@ > > #define AD7923_PM_MODE_AS (1) /* auto shutdown */ > > #define AD7923_PM_MODE_FS (2) /* full shutdown */ > > #define AD7923_PM_MODE_OPS (3) /* normal operation */ > > -#define AD7923_SEQUENCE_OFF (0) /* no sequence fonction > > */ > > -#define AD7923_SEQUENCE_PROTECT (2) /* no interrupt > > write cycle */ > > -#define AD7923_SEQUENCE_ON (3) /* continuous sequence */ > > > > -#define AD7923_MAX_CHAN 4 > > +#define AD7923_SEQUENCE_OFF (0x00) /* no sequence function > > */ > > +#define AD7923_SEQUENCE_SHADOW (0x01) /* use the shadow > > register > > + * for programming > > + * (only AD7908/7918/7928) > > > [1] Since this reg is only available on these 3 chips, you could rename > this to AD7908_SEQUENCE_SHADOW and use it for these 3 chips only. > That way, you don't need to mention via comment "(only AD7908/7918/7928)" > > > > + */ > > +#define AD7923_SEQUENCE_PROTECT (0x10) /* no interrupt > > write cycle */ > > +#define AD7923_SEQUENCE_ON (0x11) /* continuous sequence */ > > > > #define AD7923_PM_MODE_WRITE(mode) ((mode) << 4) /* write mode */ > > #define AD7923_CHANNEL_WRITE(channel) ((channel) << 6) /* write > > channel */ > > @@ -74,6 +77,9 @@ enum ad7923_id { > > AD7904, > > AD7914, > > AD7924, > > + AD7908, > > + AD7918, > > + AD7928 > > }; > > > > #define AD7923_V_CHAN(index, bits) \ > > @@ -93,7 +99,7 @@ enum ad7923_id { > > }, \ > > } > > > > -#define DECLARE_AD7923_CHANNELS(name, bits) \ > > +#define DECLARE_AD7923_4CHANNELS(name, bits) \ > > const struct iio_chan_spec name ## _channels[] = { \ > > AD7923_V_CHAN(0, bits), \ > > AD7923_V_CHAN(1, bits), \ > > @@ -102,9 +108,25 @@ const struct iio_chan_spec name ## _channels[] = { \ > > IIO_CHAN_SOFT_TIMESTAMP(4), \ > > } > > > > -static DECLARE_AD7923_CHANNELS(ad7904, 8); > > -static DECLARE_AD7923_CHANNELS(ad7914, 10); > > -static DECLARE_AD7923_CHANNELS(ad7924, 12); > > +#define DECLARE_AD7923_8CHANNELS(name, bits) \ > > Same comments as [1], you could leave DECLARE_AD7923_CHANNELS and declare > DECLARE_AD7908_CHANNELS which has 8 channels and use for the 3 new chips. > > > +const struct iio_chan_spec name ## _channels[] = { \ > > + AD7923_V_CHAN(0, bits), \ > > + AD7923_V_CHAN(1, bits), \ > > + AD7923_V_CHAN(2, bits), \ > > + AD7923_V_CHAN(3, bits), \ > > + AD7923_V_CHAN(4, bits), \ > > + AD7923_V_CHAN(5, bits), \ > > + AD7923_V_CHAN(6, bits), \ > > + AD7923_V_CHAN(7, bits), \ > > + IIO_CHAN_SOFT_TIMESTAMP(8), \ > > +} > > + > > +static DECLARE_AD7923_4CHANNELS(ad7904, 8); > > +static DECLARE_AD7923_4CHANNELS(ad7914, 10); > > +static DECLARE_AD7923_4CHANNELS(ad7924, 12); > > +static DECLARE_AD7923_8CHANNELS(ad7908, 8); > > +static DECLARE_AD7923_8CHANNELS(ad7918, 10); > > +static DECLARE_AD7923_8CHANNELS(ad7928, 12); > > > > static const struct ad7923_chip_info ad7923_chip_info[] = { > > [AD7904] = { > > @@ -119,6 +141,18 @@ static const struct ad7923_chip_info > > ad7923_chip_info[] = { > > .channels = ad7924_channels, > > .num_channels = ARRAY_SIZE(ad7924_channels), > > }, > > + [AD7908] = { > > + .channels = ad7908_channels, > > + .num_channels = ARRAY_SIZE(ad7908_channels), > > + }, > > + [AD7918] = { > > + .channels = ad7918_channels, > > + .num_channels = ARRAY_SIZE(ad7918_channels), > > + }, > > + [AD7928] = { > > + .channels = ad7928_channels, > > + .num_channels = ARRAY_SIZE(ad7928_channels), > > + }, > > }; > > > > /** > > @@ -131,7 +165,7 @@ static int ad7923_update_scan_mode(struct iio_dev > > *indio_dev, > > int i, cmd, len; > > > > len = 0; > > - for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN) { > > + for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) > > I got the idea about 'indio_dev->num_channel - 1'. > This makes the assumption that the number of IIO channels is always HW- > channels + 1 timestamp channel [which for now is true]. > I'm not sure if this is ok. > > I think Jonathan would be a bit better here to comment. It is a driver specific assumption so we don't need it to be true in general. Perhaps add a comment along the lines of /* * For this driver the last channel is always the software timestamp so * skip that one. */ > > > { > > cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) | > > AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | > > st->settings; > > @@ -344,6 +378,9 @@ static const struct spi_device_id ad7923_id[] = { > > {"ad7914", AD7914}, > > {"ad7923", AD7924}, > > {"ad7924", AD7924}, > > + {"ad7908", AD7908}, > > + {"ad7918", AD7918}, > > + {"ad7928", AD7928}, > > {} > > }; > > MODULE_DEVICE_TABLE(spi, ad7923_id); > > @@ -353,6 +390,9 @@ static const struct of_device_id ad7923_of_match[] = > > { > > { .compatible = "adi,ad7914", }, > > { .compatible = "adi,ad7923", }, > > { .compatible = "adi,ad7924", }, > > + { .compatible = "adi,ad7908", }, > > + { .compatible = "adi,ad7918", }, > > + { .compatible = "adi,ad7928", }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, ad7923_of_match); > > @@ -370,5 +410,5 @@ module_spi_driver(ad7923_driver); > > > > MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > > MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>"); > > -MODULE_DESCRIPTION("Analog Devices AD7904/AD7914/AD7923/AD7924 ADC"); > > +MODULE_DESCRIPTION("Analog Devices > > AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 ADC"); > > This description is becoming long. > Maybe simplify to > MODULE_DESCRIPTION("Analog Devices AD7923 and similar ADC"); > > > MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 2019-12-15 15:43 ` Jonathan Cameron @ 2019-12-16 11:59 ` Daniel Junho 0 siblings, 0 replies; 12+ messages in thread From: Daniel Junho @ 2019-12-16 11:59 UTC (permalink / raw) To: Jonathan Cameron Cc: Ardelean, Alexandru, Popa, Stefan Serban, lkcamp, linux-iio, linux-kernel, pmeerw, knaack.h, Hennerich, Michael, lars On Sun, Dec 15, 2019 at 12:43 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 11 Dec 2019 07:46:04 +0000 > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > > > On Tue, 2019-12-10 at 12:08 -0300, Daniel Junho wrote: > > > [External] > > > > > > The ad7928 is software compatible with the ad7923. > > > The ad7908 and ad7918 are the 8 and 10-bit versions of the ad7928. > > > > > > > The patch looks good. > > Comments mostly about namimg. > > One about channel numbers. > > > > > > > Signed-off-by: Daniel Junho <djunho@gmail.com> > > > --- > > > drivers/iio/adc/ad7923.c | 62 +++++++++++++++++++++++++++++++++------- > > > 1 file changed, 51 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c > > > index 6d56fa0b9e30..ae807fcaad76 100644 > > > --- a/drivers/iio/adc/ad7923.c > > > +++ b/drivers/iio/adc/ad7923.c > > > @@ -1,6 +1,6 @@ > > > // SPDX-License-Identifier: GPL-2.0-only > > > /* > > > - * AD7904/AD7914/AD7923/AD7924 SPI ADC driver > > > + * AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 SPI ADC driver > > > * > > > * Copyright 2011 Analog Devices Inc (from AD7923 Driver) > > > * Copyright 2012 CS Systemes d'Information > > > @@ -29,11 +29,14 @@ > > > #define AD7923_PM_MODE_AS (1) /* auto shutdown */ > > > #define AD7923_PM_MODE_FS (2) /* full shutdown */ > > > #define AD7923_PM_MODE_OPS (3) /* normal operation */ > > > -#define AD7923_SEQUENCE_OFF (0) /* no sequence fonction > > > */ > > > -#define AD7923_SEQUENCE_PROTECT (2) /* no interrupt > > > write cycle */ > > > -#define AD7923_SEQUENCE_ON (3) /* continuous sequence */ > > > > > > -#define AD7923_MAX_CHAN 4 > > > +#define AD7923_SEQUENCE_OFF (0x00) /* no sequence function > > > */ > > > +#define AD7923_SEQUENCE_SHADOW (0x01) /* use the shadow > > > register > > > + * for programming > > > + * (only AD7908/7918/7928) > > > > > > [1] Since this reg is only available on these 3 chips, you could rename > > this to AD7908_SEQUENCE_SHADOW and use it for these 3 chips only. > > That way, you don't need to mention via comment "(only AD7908/7918/7928)" > > > > > > > + */ > > > +#define AD7923_SEQUENCE_PROTECT (0x10) /* no interrupt > > > write cycle */ > > > +#define AD7923_SEQUENCE_ON (0x11) /* continuous sequence */ > > > > > > #define AD7923_PM_MODE_WRITE(mode) ((mode) << 4) /* write mode */ > > > #define AD7923_CHANNEL_WRITE(channel) ((channel) << 6) /* write > > > channel */ > > > @@ -74,6 +77,9 @@ enum ad7923_id { > > > AD7904, > > > AD7914, > > > AD7924, > > > + AD7908, > > > + AD7918, > > > + AD7928 > > > }; > > > > > > #define AD7923_V_CHAN(index, bits) \ > > > @@ -93,7 +99,7 @@ enum ad7923_id { > > > }, \ > > > } > > > > > > -#define DECLARE_AD7923_CHANNELS(name, bits) \ > > > +#define DECLARE_AD7923_4CHANNELS(name, bits) \ > > > const struct iio_chan_spec name ## _channels[] = { \ > > > AD7923_V_CHAN(0, bits), \ > > > AD7923_V_CHAN(1, bits), \ > > > @@ -102,9 +108,25 @@ const struct iio_chan_spec name ## _channels[] = { \ > > > IIO_CHAN_SOFT_TIMESTAMP(4), \ > > > } > > > > > > -static DECLARE_AD7923_CHANNELS(ad7904, 8); > > > -static DECLARE_AD7923_CHANNELS(ad7914, 10); > > > -static DECLARE_AD7923_CHANNELS(ad7924, 12); > > > +#define DECLARE_AD7923_8CHANNELS(name, bits) \ > > > > Same comments as [1], you could leave DECLARE_AD7923_CHANNELS and declare > > DECLARE_AD7908_CHANNELS which has 8 channels and use for the 3 new chips. > > > > > +const struct iio_chan_spec name ## _channels[] = { \ > > > + AD7923_V_CHAN(0, bits), \ > > > + AD7923_V_CHAN(1, bits), \ > > > + AD7923_V_CHAN(2, bits), \ > > > + AD7923_V_CHAN(3, bits), \ > > > + AD7923_V_CHAN(4, bits), \ > > > + AD7923_V_CHAN(5, bits), \ > > > + AD7923_V_CHAN(6, bits), \ > > > + AD7923_V_CHAN(7, bits), \ > > > + IIO_CHAN_SOFT_TIMESTAMP(8), \ > > > +} > > > + > > > +static DECLARE_AD7923_4CHANNELS(ad7904, 8); > > > +static DECLARE_AD7923_4CHANNELS(ad7914, 10); > > > +static DECLARE_AD7923_4CHANNELS(ad7924, 12); > > > +static DECLARE_AD7923_8CHANNELS(ad7908, 8); > > > +static DECLARE_AD7923_8CHANNELS(ad7918, 10); > > > +static DECLARE_AD7923_8CHANNELS(ad7928, 12); > > > > > > static const struct ad7923_chip_info ad7923_chip_info[] = { > > > [AD7904] = { > > > @@ -119,6 +141,18 @@ static const struct ad7923_chip_info > > > ad7923_chip_info[] = { > > > .channels = ad7924_channels, > > > .num_channels = ARRAY_SIZE(ad7924_channels), > > > }, > > > + [AD7908] = { > > > + .channels = ad7908_channels, > > > + .num_channels = ARRAY_SIZE(ad7908_channels), > > > + }, > > > + [AD7918] = { > > > + .channels = ad7918_channels, > > > + .num_channels = ARRAY_SIZE(ad7918_channels), > > > + }, > > > + [AD7928] = { > > > + .channels = ad7928_channels, > > > + .num_channels = ARRAY_SIZE(ad7928_channels), > > > + }, > > > }; > > > > > > /** > > > @@ -131,7 +165,7 @@ static int ad7923_update_scan_mode(struct iio_dev > > > *indio_dev, > > > int i, cmd, len; > > > > > > len = 0; > > > - for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN) { > > > + for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) > > > > I got the idea about 'indio_dev->num_channel - 1'. > > This makes the assumption that the number of IIO channels is always HW- > > channels + 1 timestamp channel [which for now is true]. > > I'm not sure if this is ok. > > > > I think Jonathan would be a bit better here to comment. > > It is a driver specific assumption so we don't need it to be true > in general. Perhaps add a comment along the lines of > /* > * For this driver the last channel is always the software timestamp so > * skip that one. > */ > Ok. I will add this comment to the code and send a v3. Thank you all. > > > > > > { > > > cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) | > > > AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | > > > st->settings; > > > @@ -344,6 +378,9 @@ static const struct spi_device_id ad7923_id[] = { > > > {"ad7914", AD7914}, > > > {"ad7923", AD7924}, > > > {"ad7924", AD7924}, > > > + {"ad7908", AD7908}, > > > + {"ad7918", AD7918}, > > > + {"ad7928", AD7928}, > > > {} > > > }; > > > MODULE_DEVICE_TABLE(spi, ad7923_id); > > > @@ -353,6 +390,9 @@ static const struct of_device_id ad7923_of_match[] = > > > { > > > { .compatible = "adi,ad7914", }, > > > { .compatible = "adi,ad7923", }, > > > { .compatible = "adi,ad7924", }, > > > + { .compatible = "adi,ad7908", }, > > > + { .compatible = "adi,ad7918", }, > > > + { .compatible = "adi,ad7928", }, > > > { }, > > > }; > > > MODULE_DEVICE_TABLE(of, ad7923_of_match); > > > @@ -370,5 +410,5 @@ module_spi_driver(ad7923_driver); > > > > > > MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > > > MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>"); > > > -MODULE_DESCRIPTION("Analog Devices AD7904/AD7914/AD7923/AD7924 ADC"); > > > +MODULE_DESCRIPTION("Analog Devices > > > AD7904/AD7914/AD7923/AD7924/AD7908/AD7918/AD7928 ADC"); > > > > This description is becoming long. > > Maybe simplify to > > MODULE_DESCRIPTION("Analog Devices AD7923 and similar ADC"); > > > > > MODULE_LICENSE("GPL v2"); > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-12-16 11:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-10 15:08 [PATCH v2 0/4] Add support for the ad7908/ad7918/ad7928 Daniel Junho 2019-12-10 15:08 ` [PATCH v2 1/4] iio: adc: ad7923: Remove the unused defines Daniel Junho 2019-12-11 7:26 ` Ardelean, Alexandru 2019-12-15 15:47 ` Jonathan Cameron 2019-12-10 15:08 ` [PATCH v2 2/4] iio: adc: ad7923: Fix checkpatch warning Daniel Junho 2019-12-11 7:26 ` Ardelean, Alexandru 2019-12-10 15:08 ` [PATCH v2 3/4] iio: adc: ad7923: Add of_device_id table Daniel Junho 2019-12-11 7:27 ` Ardelean, Alexandru 2019-12-10 15:08 ` [PATCH v2 4/4] iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho 2019-12-11 7:46 ` Ardelean, Alexandru 2019-12-15 15:43 ` Jonathan Cameron 2019-12-16 11:59 ` Daniel Junho
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.