Re: [PATCH 1/4] staging: iio: ad7150: organize registers definition

[Jonathan Cameron]

On Sat, 4 May 2019 13:13:55 +0300
Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote:

> On Sat, May 4, 2019 at 1:25 AM Melissa Wen <melissa.srw@xxxxxxxxx> wrote:
> >
> > Use the suffix REG to make the register addresses clear
> > and indentation to highlight field names.
> >  
> 
> I'm inclined to say that this change is a bit too much noise versus added value.
> While the REG suffix does make sense (generally), since it hasn't been
> added from the beginning, it doesn't make much sense to add it now.
> 
> It is sufficiently clear (as-is) that these macros refer to registers.
> They should be easy to match with the datasheet as well.
I'd agree for a driver that was outside staging that this is more noise than
it is worth, but one of the aims of moving drivers out is to produce
very clean and nice code. In this particular case there are very few fields
so it's not nearly as much of a mess as some other drivers where it really
hasn't been clear and the _REG suffix added much greater benefits.

Some of the arguments for minimizing noise typically don't apply on
the basis we 'probably' won't backport fixes across the move out of staging.

So I'm a bit on the fence on this one, but probably fall slightly
more in favour of the change.

Hence I would say up to you Melissa on whether you want to keep this
one or not given the arguments in favour and against that Alex
has laid out.

Given there are a few bits needed in later patches, I'll not pick this
one up for now either way!

Jonathan

> 
> > Signed-off-by: Melissa Wen <melissa.srw@xxxxxxxxx>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 75 ++++++++++++++++----------------
> >  1 file changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index dd7fcab8e19e..24601ba7db88 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -15,35 +15,34 @@
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/events.h>
> > -/*
> > - * AD7150 registers definition
> > - */
> >
> > -#define AD7150_STATUS              0
> > -#define AD7150_STATUS_OUT1         BIT(3)
> > -#define AD7150_STATUS_OUT2         BIT(5)
> > -#define AD7150_CH1_DATA_HIGH       1
> > -#define AD7150_CH2_DATA_HIGH       3
> > -#define AD7150_CH1_AVG_HIGH        5
> > -#define AD7150_CH2_AVG_HIGH        7
> > -#define AD7150_CH1_SENSITIVITY     9
> > -#define AD7150_CH1_THR_HOLD_H      9
> > -#define AD7150_CH1_TIMEOUT         10
> > -#define AD7150_CH1_SETUP           11
> > -#define AD7150_CH2_SENSITIVITY     12
> > -#define AD7150_CH2_THR_HOLD_H      12
> > -#define AD7150_CH2_TIMEOUT         13
> > -#define AD7150_CH2_SETUP           14
> > -#define AD7150_CFG                 15
> > -#define AD7150_CFG_FIX             BIT(7)
> > -#define AD7150_PD_TIMER            16
> > -#define AD7150_CH1_CAPDAC          17
> > -#define AD7150_CH2_CAPDAC          18
> > -#define AD7150_SN3                 19
> > -#define AD7150_SN2                 20
> > -#define AD7150_SN1                 21
> > -#define AD7150_SN0                 22
> > -#define AD7150_ID                  23
> > +/* AD7150 registers */
> > +
> > +#define AD7150_STATUS_REG                      0x00
> > +#define         AD7150_STATUS_OUT1                     BIT(3)
> > +#define         AD7150_STATUS_OUT2                     BIT(5)
> > +#define AD7150_CH1_DATA_HIGH_REG               0x01
> > +#define AD7150_CH2_DATA_HIGH_REG               0x03
> > +#define AD7150_CH1_AVG_HIGH_REG                        0x05
> > +#define AD7150_CH2_AVG_HIGH_REG                        0x07
> > +#define AD7150_CH1_SENSITIVITY_REG             0x09
> > +#define AD7150_CH1_THR_HOLD_H_REG              0x09
> > +#define AD7150_CH2_SENSITIVITY_REG             0x0C
> > +#define AD7150_CH1_TIMEOUT_REG                 0x0A
> > +#define AD7150_CH1_SETUP_REG                   0x0B
> > +#define AD7150_CH2_THR_HOLD_H_REG              0x0C
> > +#define AD7150_CH2_TIMEOUT_REG                 0x0D
> > +#define AD7150_CH2_SETUP_REG                   0x0E
> > +#define AD7150_CFG_REG                         0x0F
> > +#define         AD7150_CFG_FIX                         BIT(7)
> > +#define AD7150_PD_TIMER_REG                    0x10
> > +#define AD7150_CH1_CAPDAC_REG                  0x11
> > +#define AD7150_CH2_CAPDAC_REG                  0x12
> > +#define AD7150_SN3_REG                         0x13
> > +#define AD7150_SN2_REG                         0x14
> > +#define AD7150_SN1_REG                         0x15
> > +#define AD7150_SN0_REG                         0x16
> > +#define AD7150_ID_REG                          0x17
> >
> >  /**
> >   * struct ad7150_chip_info - instance specific chip data
> > @@ -85,12 +84,12 @@ struct ad7150_chip_info {
> >   */
> >
> >  static const u8 ad7150_addresses[][6] = {
> > -       { AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
> > -         AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
> > -         AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
> > -       { AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
> > -         AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
> > -         AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
> > +       { AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
> > +         AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
> > +         AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
> > +       { AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
> > +         AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
> > +         AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
> >  };
> >
> >  static int ad7150_read_raw(struct iio_dev *indio_dev,
> > @@ -133,7 +132,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> >         bool adaptive;
> >         struct ad7150_chip_info *chip = iio_priv(indio_dev);
> >
> > -       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> > +       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> >         if (ret < 0)
> >                 return ret;
> >
> > @@ -229,7 +228,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> >         if (event_code == chip->current_event)
> >                 return 0;
> >         mutex_lock(&chip->state_lock);
> > -       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> > +       ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> >         if (ret < 0)
> >                 goto error_ret;
> >
> > @@ -264,7 +263,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> >
> >         cfg |= (!adaptive << 7) | (thresh_type << 5);
> >
> > -       ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
> > +       ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG, cfg);
> >         if (ret < 0)
> >                 goto error_ret;
> >
> > @@ -497,7 +496,7 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
> >         s64 timestamp = iio_get_time_ns(indio_dev);
> >         int ret;
> >
> > -       ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
> > +       ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
> >         if (ret < 0)
> >                 return IRQ_HANDLED;
> >
> > --
> > 2.20.1
> >