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
>
