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

[Alexandru Ardelean]

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.

> 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
>