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@xxxxxxxxxx> wrote: > > On Thu, 5 Dec 2019 12:58:21 -0300 > Daniel Junho <djunho@xxxxxxxxx> 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@xxxxxxxxx> > 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@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"); > > 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