* [PATCH] iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928
@ 2019-12-05 15:58 Daniel Junho
2019-12-07 11:26 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Junho @ 2019-12-05 15:58 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 version of the ad7928.
Signed-off-by: Daniel Junho <djunho@gmail.com>
---
drivers/iio/adc/Kconfig | 3 +-
drivers/iio/adc/ad7923.c | 96 +++++++++++++++++++++++++++++++---------
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 976567d4dbef..40aa05fbf85e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -205,7 +205,8 @@ config AD7923
select IIO_TRIGGERED_BUFFER
help
Say yes here to build support for Analog Devices
- AD7904, AD7914, AD7923, AD7924 4 Channel ADCs.
+ AD7904, AD7914, AD7923, AD7924 4 Channel ADCs, and
+ AD7908, AD7918, AD7928 8 Channels ADCs.
To compile this driver as a module, choose M here: the
module will be called ad7923.
diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index 3212eb4c0f25..f08a5218dad1 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
@@ -26,23 +26,32 @@
#define AD7923_WRITE_CR BIT(11) /* write control register */
#define AD7923_RANGE BIT(1) /* range to REFin */
#define AD7923_CODING BIT(0) /* coding is straight binary */
+
#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 */
-
-#define AD7923_MAX_CHAN 4
+#define AD7923_CHANNEL_4 (4) /* analog input 4 */
+#define AD7923_CHANNEL_5 (5) /* analog input 5 */
+#define AD7923_CHANNEL_6 (6) /* analog input 6 */
+#define AD7923_CHANNEL_7 (7) /* analog input 7 */
+
+#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 */
-#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 1) << 3) \
- + (((sequence) & 2) << 9))
+#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 0x01) << 3) \
+ + (((sequence) & 0x10) << 9))
/* write sequence fonction */
/* left shift for CR : bit 11 transmit in first */
#define AD7923_SHIFT_REGISTER 4
@@ -78,6 +87,9 @@ enum ad7923_id {
AD7904,
AD7914,
AD7924,
+ AD7908,
+ AD7918,
+ AD7928
};
#define AD7923_V_CHAN(index, bits) \
@@ -97,7 +109,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), \
@@ -106,9 +118,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] = {
@@ -123,6 +151,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),
+ },
};
/**
@@ -135,10 +175,10 @@ 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;
+ AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
+ st->settings;
cmd <<= AD7923_SHIFT_REGISTER;
st->tx_buf[len++] = cpu_to_be16(cmd);
}
@@ -188,13 +228,13 @@ 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;
cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(ch) |
- AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
- st->settings;
+ AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
+ st->settings;
cmd <<= AD7923_SHIFT_REGISTER;
st->tx_buf[0] = cpu_to_be16(cmd);
@@ -280,7 +320,7 @@ static int ad7923_probe(struct spi_device *spi)
st->spi = spi;
st->settings = AD7923_CODING | AD7923_RANGE |
- AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
+ AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
info = &ad7923_chip_info[spi_get_device_id(spi)->driver_data];
@@ -348,13 +388,29 @@ 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);
+static const struct of_device_id ad7923_of_match[] = {
+ { .compatible = "adi,ad7904", },
+ { .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);
+
static struct spi_driver ad7923_driver = {
.driver = {
.name = "ad7923",
+ .of_match_table = ad7923_of_match,
},
.probe = ad7923_probe,
.remove = ad7923_remove,
@@ -364,5 +420,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] 3+ messages in thread
* Re: [PATCH] iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928
2019-12-05 15:58 [PATCH] iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho
@ 2019-12-07 11:26 ` Jonathan Cameron
[not found] ` <CAKxs2cDmycwOD6CE2UiSb18GkLgmRK=m7jkBq2+rBc8UGosStw@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2019-12-07 11:26 UTC (permalink / raw)
To: Daniel Junho
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp
On Thu, 5 Dec 2019 12:58:21 -0300
Daniel Junho <djunho@gmail.com> wrote:
> The ad7928 is software compatible with the ad7923. The ad7908 and ad7918 are the
> 8 and 10 bit version of the ad7928.
>
> Signed-off-by: Daniel Junho <djunho@gmail.com>
Hi Daniel,
Code itself looks good to me, but please be very careful to separate out
non function cleanup from a patch adding something new. It makes
reviewing harder. We'd much rather see a series with the cleanup all clearly
marked as such (and getting a most a few seconds review) and the real code
being just that.
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 3 +-
> drivers/iio/adc/ad7923.c | 96 +++++++++++++++++++++++++++++++---------
> 2 files changed, 78 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 976567d4dbef..40aa05fbf85e 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -205,7 +205,8 @@ config AD7923
> select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build support for Analog Devices
> - AD7904, AD7914, AD7923, AD7924 4 Channel ADCs.
> + AD7904, AD7914, AD7923, AD7924 4 Channel ADCs, and
> + AD7908, AD7918, AD7928 8 Channels ADCs.
>
> To compile this driver as a module, choose M here: the
> module will be called ad7923.
> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> index 3212eb4c0f25..f08a5218dad1 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
> @@ -26,23 +26,32 @@
> #define AD7923_WRITE_CR BIT(11) /* write control register */
> #define AD7923_RANGE BIT(1) /* range to REFin */
> #define AD7923_CODING BIT(0) /* coding is straight binary */
> +
No white space changes in a patch doing real changes to the driver.
> #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 */
> -
> -#define AD7923_MAX_CHAN 4
> +#define AD7923_CHANNEL_4 (4) /* analog input 4 */
> +#define AD7923_CHANNEL_5 (5) /* analog input 5 */
> +#define AD7923_CHANNEL_6 (6) /* analog input 6 */
> +#define AD7923_CHANNEL_7 (7) /* analog input 7 */
These made me wonder. Seems they aren't actually used in the driver anyway.
Ideally add a precursor to your main patch just dropping these particular
defines entirely.
> +
> +#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 */
> -#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 1) << 3) \
> - + (((sequence) & 2) << 9))
> +#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 0x01) << 3) \
> + + (((sequence) & 0x10) << 9))
This change is interesting. Was original code just buggy? we were masking
against 2nd bit, and now we are masking against the 4th?
> /* write sequence fonction */
> /* left shift for CR : bit 11 transmit in first */
> #define AD7923_SHIFT_REGISTER 4
> @@ -78,6 +87,9 @@ enum ad7923_id {
> AD7904,
> AD7914,
> AD7924,
> + AD7908,
> + AD7918,
> + AD7928
> };
>
> #define AD7923_V_CHAN(index, bits) \
> @@ -97,7 +109,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), \
> @@ -106,9 +118,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] = {
> @@ -123,6 +151,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),
> + },
> };
>
> /**
> @@ -135,10 +175,10 @@ 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;
> + AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> + st->settings;
> cmd <<= AD7923_SHIFT_REGISTER;
> st->tx_buf[len++] = cpu_to_be16(cmd);
> }
> @@ -188,13 +228,13 @@ 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)
Should be in a precursor patch tidying this up. Never mix new features
with cleanup as it's harder to review.
> {
> int ret, cmd;
>
> cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(ch) |
> - AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> - st->settings;
> + AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> + st->settings;
Clean out this noise from the patch. We just want to see
the new stuff rather than stare at a line to figure if it
is just whitespace, or if there is a real change ;)
> cmd <<= AD7923_SHIFT_REGISTER;
> st->tx_buf[0] = cpu_to_be16(cmd);
>
> @@ -280,7 +320,7 @@ static int ad7923_probe(struct spi_device *spi)
>
> st->spi = spi;
> st->settings = AD7923_CODING | AD7923_RANGE |
> - AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
> + AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
Please clean out an noise like this bit of realignment.
>
> info = &ad7923_chip_info[spi_get_device_id(spi)->driver_data];
>
> @@ -348,13 +388,29 @@ 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);
>
> +static const struct of_device_id ad7923_of_match[] = {
> + { .compatible = "adi,ad7904", },
> + { .compatible = "adi,ad7914", },
> + { .compatible = "adi,ad7923", },
> + { .compatible = "adi,ad7924", },
> + { .compatible = "adi,ad7908", },
> + { .compatible = "adi,ad7918", },
> + { .compatible = "adi,ad7928", },
> + { },
Really minor but I would have preferred a precursor patch
adding the of_device_id table for existing parts.
Makes a clear separation between new device support and tidying
up the fact that table should be there to allow the specific
adi,* matching.
> +};
> +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,
> @@ -364,5 +420,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");
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928
[not found] ` <CAKxs2cDmycwOD6CE2UiSb18GkLgmRK=m7jkBq2+rBc8UGosStw@mail.gmail.com>
@ 2019-12-10 0:47 ` Daniel Junho
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Junho @ 2019-12-10 0:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michael Hennerich, Stefan Popa, linux-iio, linux-kernel, lkcamp
Hi Jonathan,
First of all, thank you so much for the feedback and the time spent.
On Sat, Dec 7, 2019 at 8:26 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 5 Dec 2019 12:58:21 -0300
> Daniel Junho <djunho@gmail.com> wrote:
>
> > The ad7928 is software compatible with the ad7923. The ad7908 and ad7918 are the
> > 8 and 10 bit version of the ad7928.
> >
> > Signed-off-by: Daniel Junho <djunho@gmail.com>
> Hi Daniel,
>
> Code itself looks good to me, but please be very careful to separate out
> non function cleanup from a patch adding something new. It makes
> reviewing harder. We'd much rather see a series with the cleanup all clearly
> marked as such (and getting a most a few seconds review) and the real code
> being just that.
Ok. I understand and I will make these changes in a v2 patch.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/Kconfig | 3 +-
> > drivers/iio/adc/ad7923.c | 96 +++++++++++++++++++++++++++++++---------
> > 2 files changed, 78 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 976567d4dbef..40aa05fbf85e 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -205,7 +205,8 @@ config AD7923
> > select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to build support for Analog Devices
> > - AD7904, AD7914, AD7923, AD7924 4 Channel ADCs.
> > + AD7904, AD7914, AD7923, AD7924 4 Channel ADCs, and
> > + AD7908, AD7918, AD7928 8 Channels ADCs.
> >
> > To compile this driver as a module, choose M here: the
> > module will be called ad7923.
> > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> > index 3212eb4c0f25..f08a5218dad1 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
> > @@ -26,23 +26,32 @@
> > #define AD7923_WRITE_CR BIT(11) /* write control register */
> > #define AD7923_RANGE BIT(1) /* range to REFin */
> > #define AD7923_CODING BIT(0) /* coding is straight binary */
> > +
>
> No white space changes in a patch doing real changes to the driver.
Ok
>
> > #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 */
> > -
> > -#define AD7923_MAX_CHAN 4
> > +#define AD7923_CHANNEL_4 (4) /* analog input 4 */
> > +#define AD7923_CHANNEL_5 (5) /* analog input 5 */
> > +#define AD7923_CHANNEL_6 (6) /* analog input 6 */
> > +#define AD7923_CHANNEL_7 (7) /* analog input 7 */
> These made me wonder. Seems they aren't actually used in the driver anyway.
> Ideally add a precursor to your main patch just dropping these particular
> defines entirely.
Ok. I will remove them on a cleanup commit.
>
>
> > +
> > +#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 */
> > -#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 1) << 3) \
> > - + (((sequence) & 2) << 9))
> > +#define AD7923_SEQUENCE_WRITE(sequence) ((((sequence) & 0x01) << 3) \
> > + + (((sequence) & 0x10) << 9))
>
> This change is interesting. Was original code just buggy? we were masking
> against 2nd bit, and now we are masking against the 4th?
Sorry about that. My bad. I will return to 2.
It was just a quick test I did, and commit it unintentionally =(
>
> > /* write sequence fonction */
> > /* left shift for CR : bit 11 transmit in first */
> > #define AD7923_SHIFT_REGISTER 4
> > @@ -78,6 +87,9 @@ enum ad7923_id {
> > AD7904,
> > AD7914,
> > AD7924,
> > + AD7908,
> > + AD7918,
> > + AD7928
> > };
> >
> > #define AD7923_V_CHAN(index, bits) \
> > @@ -97,7 +109,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), \
> > @@ -106,9 +118,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] = {
> > @@ -123,6 +151,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),
> > + },
> > };
> >
> > /**
> > @@ -135,10 +175,10 @@ 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;
> > + AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> > + st->settings;
> > cmd <<= AD7923_SHIFT_REGISTER;
> > st->tx_buf[len++] = cpu_to_be16(cmd);
> > }
> > @@ -188,13 +228,13 @@ 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)
>
> Should be in a precursor patch tidying this up. Never mix new features
> with cleanup as it's harder to review.
Ok.
>
> > {
> > int ret, cmd;
> >
> > cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(ch) |
> > - AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> > - st->settings;
> > + AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> > + st->settings;
>
> Clean out this noise from the patch. We just want to see
> the new stuff rather than stare at a line to figure if it
> is just whitespace, or if there is a real change ;)
Ok
>
> > cmd <<= AD7923_SHIFT_REGISTER;
> > st->tx_buf[0] = cpu_to_be16(cmd);
> >
> > @@ -280,7 +320,7 @@ static int ad7923_probe(struct spi_device *spi)
> >
> > st->spi = spi;
> > st->settings = AD7923_CODING | AD7923_RANGE |
> > - AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
> > + AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS);
>
> Please clean out an noise like this bit of realignment.
Ok
>
> >
> > info = &ad7923_chip_info[spi_get_device_id(spi)->driver_data];
> >
> > @@ -348,13 +388,29 @@ 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);
> >
> > +static const struct of_device_id ad7923_of_match[] = {
> > + { .compatible = "adi,ad7904", },
> > + { .compatible = "adi,ad7914", },
> > + { .compatible = "adi,ad7923", },
> > + { .compatible = "adi,ad7924", },
> > + { .compatible = "adi,ad7908", },
> > + { .compatible = "adi,ad7918", },
> > + { .compatible = "adi,ad7928", },
> > + { },
> Really minor but I would have preferred a precursor patch
> adding the of_device_id table for existing parts.
>
> Makes a clear separation between new device support and tidying
> up the fact that table should be there to allow the specific
> adi,* matching.
OK.
>
> > +};
> > +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,
> > @@ -364,5 +420,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");
>
I will split this commit into 4.
- Remove the unused defines (AD7923_CHANNEL_x);
- Fix checkpatch warning using 'unsigned int' instead of 'unsigned';
- Add of_device_id table;
- Add support for the ad7908/ad7918/ad7928.
Thanks,
Daniel Junho
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-10 0:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05 15:58 [PATCH] iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928 Daniel Junho
2019-12-07 11:26 ` Jonathan Cameron
[not found] ` <CAKxs2cDmycwOD6CE2UiSb18GkLgmRK=m7jkBq2+rBc8UGosStw@mail.gmail.com>
2019-12-10 0:47 ` 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.