Received-SPF: pass (google.com: domain of linux-kernel+bounces-112706-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 24 Mar 2024 14:07:56 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org, gnstark@salutedevices.com,
 neil.armstrong@linaro.org, lars@metafoo.de
Subject: Re: [PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in
 struct meson_sar_adc_param
Message-ID: <20240324140756.5d001387@jic23-huawei>
In-Reply-To: <20240323231309.415425-3-martin.blumenstingl@googlemail.com>
References: <20240323231309.415425-1-martin.blumenstingl@googlemail.com>
	<20240323231309.415425-3-martin.blumenstingl@googlemail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 24 Mar 2024 00:13:08 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Consistently use bool for any register bit that enables/disables
> functionality and enum for register values where there's a choice
> between different settings. The aim is to make the code easier to read
> and understand by being more consistent. No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/iio/adc/meson_saradc.c | 47 +++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 2615d74534df..6b2af0c2bbc7 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -156,9 +156,9 @@
>  #define MESON_SAR_ADC_REG11					0x2c
>  	#define MESON_SAR_ADC_REG11_BANDGAP_EN			BIT(13)
>  	#define MESON_SAR_ADC_REG11_CMV_SEL                     BIT(6)
> -	#define MESON_SAR_ADC_REG11_VREF_VOLTAGE                BIT(5)
> -	#define MESON_SAR_ADC_REG11_EOC                         BIT(1)
> -	#define MESON_SAR_ADC_REG11_VREF_SEL                    BIT(0)
> +	#define MESON_SAR_ADC_REG11_VREF_VOLTAGE		BIT(5)
> +	#define MESON_SAR_ADC_REG11_EOC				BIT(1)
> +	#define MESON_SAR_ADC_REG11_VREF_SEL			BIT(0)

Looks like a spaces to tab conversion.  Not related to rest of patch so
put them back as spaces.  If you really want to tidy that up with tabs
later, separate patch please.

Otherwise LGTM

>  
>  #define MESON_SAR_ADC_REG13					0x34
>  	#define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK	GENMASK(13, 8)
> @@ -224,6 +224,11 @@ enum meson_sar_adc_vref_sel {
>  	VREF_VDDA = 1,
>  };
>  
> +enum meson_sar_adc_vref_voltage {
> +	VREF_VOLTAGE_0V9 = 0,
> +	VREF_VOLTAGE_1V8 = 1,
> +};
> +
>  enum meson_sar_adc_avg_mode {
>  	NO_AVERAGING = 0x0,
>  	MEAN_AVERAGING = 0x1,
> @@ -321,13 +326,13 @@ struct meson_sar_adc_param {
>  	u8					temperature_trimming_bits;
>  	unsigned int				temperature_multiplier;
>  	unsigned int				temperature_divider;
> -	u8					disable_ring_counter;
> +	bool					disable_ring_counter;
>  	bool					has_reg11;
>  	bool					has_vref_select;
> -	u8					vref_select;
> -	u8					cmv_select;
> -	u8					adc_eoc;
> -	enum meson_sar_adc_vref_sel		vref_voltage;
> +	bool					cmv_select;
> +	bool					adc_eoc;
> +	enum meson_sar_adc_vref_sel		vref_select;
> +	enum meson_sar_adc_vref_voltage		vref_voltage;
>  };
>  
>  struct meson_sar_adc_data {
> @@ -982,14 +987,16 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  				   MESON_SAR_ADC_DELTA_10_TS_REVE0, 0);
>  	}
>  
> -	regval = FIELD_PREP(MESON_SAR_ADC_REG3_CTRL_CONT_RING_COUNTER_EN,
> -			    priv->param->disable_ring_counter);
> +	if (priv->param->disable_ring_counter)
> +		regval = MESON_SAR_ADC_REG3_CTRL_CONT_RING_COUNTER_EN;
> +	else
> +		regval = 0;
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
>  			   MESON_SAR_ADC_REG3_CTRL_CONT_RING_COUNTER_EN,
>  			   regval);
>  
>  	if (priv->param->has_reg11) {
> -		regval = FIELD_PREP(MESON_SAR_ADC_REG11_EOC, priv->param->adc_eoc);
> +		regval = priv->param->adc_eoc ? MESON_SAR_ADC_REG11_EOC : 0;
>  		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
>  				   MESON_SAR_ADC_REG11_EOC, regval);
>  
> @@ -1005,8 +1012,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
>  				   MESON_SAR_ADC_REG11_VREF_VOLTAGE, regval);
>  
> -		regval = FIELD_PREP(MESON_SAR_ADC_REG11_CMV_SEL,
> -				    priv->param->cmv_select);
> +		regval = priv->param->cmv_select ? MESON_SAR_ADC_REG11_CMV_SEL : 0;
>  		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
>  				   MESON_SAR_ADC_REG11_CMV_SEL, regval);
>  	}
> @@ -1225,8 +1231,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
>  	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
>  	.resolution = 10,
>  	.has_reg11 = true,
> -	.vref_voltage = 1,
> -	.cmv_select = 1,
> +	.vref_voltage = VREF_VOLTAGE_1V8,
> +	.cmv_select = true,
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
> @@ -1237,8 +1243,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
>  	.resolution = 12,
>  	.disable_ring_counter = 1,
>  	.has_reg11 = true,
> -	.vref_voltage = 1,
> -	.cmv_select = 1,
> +	.vref_voltage = VREF_VOLTAGE_1V8,
> +	.cmv_select = true,
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
> @@ -1249,10 +1255,10 @@ static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
>  	.resolution = 12,
>  	.disable_ring_counter = 1,
>  	.has_reg11 = true,
> -	.vref_voltage = 1,
> +	.vref_voltage = VREF_VOLTAGE_1V8,
>  	.has_vref_select = true,
>  	.vref_select = VREF_VDDA,
> -	.cmv_select = 1,
> +	.cmv_select = true,
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_g12a_param = {
> @@ -1263,7 +1269,8 @@ static const struct meson_sar_adc_param meson_sar_adc_g12a_param = {
>  	.resolution = 12,
>  	.disable_ring_counter = 1,
>  	.has_reg11 = true,
> -	.adc_eoc = 1,
> +	.vref_voltage = VREF_VOLTAGE_0V9,
> +	.adc_eoc = true,
>  	.has_vref_select = true,
>  	.vref_select = VREF_VDDA,
>  };