On Sun, Dec 15, 2019 at 12:43 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Wed, 11 Dec 2019 07:46:04 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>");
> > > MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@xxxxxx>");
> > > -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");
>
