iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928
diff mbox series

Message ID 20191205155821.7441-1-djunho@gmail.com
State New
Headers show
Series
  • iio:adc:ad7923: Add support for the ad7908/ad7918/ad7928
Related show

Commit Message

Daniel Junho Dec. 5, 2019, 3:58 p.m. UTC
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(-)

Comments

Jonathan Cameron Dec. 7, 2019, 11:26 a.m. UTC | #1
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");
Daniel Junho Dec. 10, 2019, 12:47 a.m. UTC | #2
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

Patch
diff mbox series

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");