* [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
* [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
* [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
* [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 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 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
* 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
* 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 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
* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).