[PATCH v1 0/3] iio: adc: meson: a few improvements

[PATCH v1 0/3] iio: adc: meson: a few improvements

[Martin Blumenstingl]

This series contains three improvements to the meson SAR ADC driver.
None of them are meant to change the existing behavior. The goal is
to make the driver code easier to read and understand.


Martin Blumenstingl (3):
  iio: adc: meson: fix voltage reference selection field name typo
  iio: adc: meson: consistently use bool/enum in struct
    meson_sar_adc_param
  iio: adc: meson: simplify MESON_SAR_ADC_REG11 register access

 drivers/iio/adc/meson_saradc.c | 78 ++++++++++++++++------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] iio: adc: meson: fix voltage reference selection field name typo

[Martin Blumenstingl]

The field should be called "vref_voltage", without a typo in the word
voltage. No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 13b473d8c6c7..2615d74534df 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -327,7 +327,7 @@ struct meson_sar_adc_param {
 	u8					vref_select;
 	u8					cmv_select;
 	u8					adc_eoc;
-	enum meson_sar_adc_vref_sel		vref_volatge;
+	enum meson_sar_adc_vref_sel		vref_voltage;
 };
 
 struct meson_sar_adc_data {
@@ -1001,7 +1001,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 		}
 
 		regval = FIELD_PREP(MESON_SAR_ADC_REG11_VREF_VOLTAGE,
-				    priv->param->vref_volatge);
+				    priv->param->vref_voltage);
 		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
 				   MESON_SAR_ADC_REG11_VREF_VOLTAGE, regval);
 
@@ -1225,7 +1225,7 @@ 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_volatge = 1,
+	.vref_voltage = 1,
 	.cmv_select = 1,
 };
 
@@ -1237,7 +1237,7 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 	.resolution = 12,
 	.disable_ring_counter = 1,
 	.has_reg11 = true,
-	.vref_volatge = 1,
+	.vref_voltage = 1,
 	.cmv_select = 1,
 };
 
@@ -1249,7 +1249,7 @@ static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
 	.resolution = 12,
 	.disable_ring_counter = 1,
 	.has_reg11 = true,
-	.vref_volatge = 1,
+	.vref_voltage = 1,
 	.has_vref_select = true,
 	.vref_select = VREF_VDDA,
 	.cmv_select = 1,
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] iio: adc: meson: fix voltage reference selection field name typo

[George Stark]

Hello Martin

Thanks for the patch

Should the tag
Fixes: 90c6241860bf ("iio: adc: meson: init voltage control bits")
be added?

On 3/24/24 02:13, Martin Blumenstingl wrote:
> The field should be called "vref_voltage", without a typo in the word
> voltage. No functional changes intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>   drivers/iio/adc/meson_saradc.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 13b473d8c6c7..2615d74534df 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -327,7 +327,7 @@ struct meson_sar_adc_param {
>   	u8					vref_select;
>   	u8					cmv_select;
>   	u8					adc_eoc;
> -	enum meson_sar_adc_vref_sel		vref_volatge;
> +	enum meson_sar_adc_vref_sel		vref_voltage;
>   };
>   
>   struct meson_sar_adc_data {
> @@ -1001,7 +1001,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>   		}
>   
>   		regval = FIELD_PREP(MESON_SAR_ADC_REG11_VREF_VOLTAGE,
> -				    priv->param->vref_volatge);
> +				    priv->param->vref_voltage);
>   		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
>   				   MESON_SAR_ADC_REG11_VREF_VOLTAGE, regval);
>   
> @@ -1225,7 +1225,7 @@ 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_volatge = 1,
> +	.vref_voltage = 1,
>   	.cmv_select = 1,
>   };
>   
> @@ -1237,7 +1237,7 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
>   	.resolution = 12,
>   	.disable_ring_counter = 1,
>   	.has_reg11 = true,
> -	.vref_volatge = 1,
> +	.vref_voltage = 1,
>   	.cmv_select = 1,
>   };
>   
> @@ -1249,7 +1249,7 @@ static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
>   	.resolution = 12,
>   	.disable_ring_counter = 1,
>   	.has_reg11 = true,
> -	.vref_volatge = 1,
> +	.vref_voltage = 1,
>   	.has_vref_select = true,
>   	.vref_select = VREF_VDDA,
>   	.cmv_select = 1,

-- 
Best regards
George

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] iio: adc: meson: fix voltage reference selection field name typo

[Martin Blumenstingl]

Hi George,

On Tue, Mar 26, 2024 at 12:45 AM George Stark <gnstark@salutedevices.com> wrote:
>
> Hello Martin
>
> Thanks for the patch
you're welcome

> Should the tag
> Fixes: 90c6241860bf ("iio: adc: meson: init voltage control bits")
> be added?
My understanding is that "Fixes" is for actual bugfixes. There's no
harm done by a type that's only visible in the driver source (it's not
exposed to DT, userspace or elsewhere).


Best regards,
Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in struct meson_sar_adc_param

[Martin Blumenstingl]

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)
 
 #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,
 };
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in struct meson_sar_adc_param

[Jonathan Cameron]

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,
>  };


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in struct meson_sar_adc_param

[George Stark]

Hello Martin

On 3/24/24 02:13, Martin Blumenstingl 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)
>   
>   #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;


The reason to choose u8 type over bool was that those are not actually
bool values but direct values of hw register bits. We have little 
information about real meaning of these bits so it won't help much to
add bool layer and keep real values in the init code instead of param 
section (adc_eoc, cmv_select).
bool disable_ring_counter will look deceptive too because it doesn't
say whether disable ring_counter or not (we always disable it) but
how to disable it (write 0 or 1)

I think the poor choice was not the type of variables but their names:
u8 adc_eoc_bit;
u8 cmv_select_bit;
u8 disable_ring_counter_bit;

would be clearer.


> -	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,
>   };

-- 
Best regards
George

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in struct meson_sar_adc_param

[Martin Blumenstingl]

Hi George,

On Tue, Mar 26, 2024 at 1:41 AM George Stark <gnstark@salutedevices.com> wrote:
[...]
> >   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;
>
>
> The reason to choose u8 type over bool was that those are not actually
> bool values but direct values of hw register bits. We have little
> information about real meaning of these bits so it won't help much to
> add bool layer and keep real values in the init code instead of param
> section (adc_eoc, cmv_select).
> bool disable_ring_counter will look deceptive too because it doesn't
> say whether disable ring_counter or not (we always disable it) but
> how to disable it (write 0 or 1)
Do you have any contact at Amlogic that can help explain the purpose
of these bits?
I tried reaching out to my contact off-list but did not get any answers.

I also have more questions about the SARADC IP block on the GXLX SoC.
We (Christian and I) have a patch prepared to fix audio (or rather the
MPLL clocks) on the GXLX SoC (see [0]). Unfortunately there seems to
be nobody that's answering me with enough info so we can upstream that
patch.

So if you have any contact or insights into datasheets - please check
and see if you can provide information so we can improve the naming of
these bits and hopefully get GXLX SoC support fixed.

Also +Cc Xingyu Chen and Zelong Dong - I found your names in
downstream patches to the SARADC driver [1].


Thank you and best regards,
Martin


[0] https://lore.kernel.org/linux-amlogic/20240604055431.3313961-2-christianshewitt@gmail.com/
[1] https://github.com/khadas/linux/commit/13321d23231a7a4d08f4fefebcef11b379475a37

Re: [PATCH v1 2/3] iio: adc: meson: consistently use bool/enum in struct meson_sar_adc_param

[George Stark]

Hello Martin

On 12/24/24 17:37, Martin Blumenstingl wrote:
> Hi George,
> 
> On Tue, Mar 26, 2024 at 1:41 AM George Stark <gnstark@salutedevices.com> wrote:
> [...]
>>>    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;
>>
>>
>> The reason to choose u8 type over bool was that those are not actually
>> bool values but direct values of hw register bits. We have little
>> information about real meaning of these bits so it won't help much to
>> add bool layer and keep real values in the init code instead of param
>> section (adc_eoc, cmv_select).
>> bool disable_ring_counter will look deceptive too because it doesn't
>> say whether disable ring_counter or not (we always disable it) but
>> how to disable it (write 0 or 1)
> Do you have any contact at Amlogic that can help explain the purpose
> of these bits?

I remember I was digging thru vendor kernel and uboot code to understand 
the meaning of those bits when prepared my adc patches. I'll check it 
again about these and bellow bits and will let you know if I find 
anything worthwhile.

> I tried reaching out to my contact off-list but did not get any answers.
> 
> I also have more questions about the SARADC IP block on the GXLX SoC.
> We (Christian and I) have a patch prepared to fix audio (or rather the
> MPLL clocks) on the GXLX SoC (see [0]). Unfortunately there seems to
> be nobody that's answering me with enough info so we can upstream that
> patch.
> 
> So if you have any contact or insights into datasheets - please check
> and see if you can provide information so we can improve the naming of
> these bits and hopefully get GXLX SoC support fixed.
> 
> Also +Cc Xingyu Chen and Zelong Dong - I found your names in
> downstream patches to the SARADC driver [1].
> 
> 
> Thank you and best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-amlogic/20240604055431.3313961-2-christianshewitt@gmail.com/
> [1] https://github.com/khadas/linux/commit/13321d23231a7a4d08f4fefebcef11b379475a37

-- 
Best regards
George

[PATCH v1 3/3] iio: adc: meson: simplify MESON_SAR_ADC_REG11 register access

[Martin Blumenstingl]

Simply check the max_register value to decide whether
MESON_SAR_ADC_REG11 is present on the current IP revision. This allows
dropping two additional bool fields from struct meson_sar_adc_param
which previously had to be manually kept in sync. No functional changes
intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 6b2af0c2bbc7..8c1e542c0ab7 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -320,14 +320,12 @@ static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
 struct meson_sar_adc_param {
 	bool					has_bl30_integration;
 	unsigned long				clock_rate;
-	u32					bandgap_reg;
 	unsigned int				resolution;
 	const struct regmap_config		*regmap_config;
 	u8					temperature_trimming_bits;
 	unsigned int				temperature_multiplier;
 	unsigned int				temperature_divider;
 	bool					disable_ring_counter;
-	bool					has_reg11;
 	bool					has_vref_select;
 	bool					cmv_select;
 	bool					adc_eoc;
@@ -995,7 +993,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 			   MESON_SAR_ADC_REG3_CTRL_CONT_RING_COUNTER_EN,
 			   regval);
 
-	if (priv->param->has_reg11) {
+	if (priv->param->regmap_config->max_register >= MESON_SAR_ADC_REG11) {
 		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);
@@ -1031,16 +1029,15 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 static void meson_sar_adc_set_bandgap(struct iio_dev *indio_dev, bool on_off)
 {
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
-	const struct meson_sar_adc_param *param = priv->param;
-	u32 enable_mask;
 
-	if (param->bandgap_reg == MESON_SAR_ADC_REG11)
-		enable_mask = MESON_SAR_ADC_REG11_BANDGAP_EN;
+	if (priv->param->regmap_config->max_register >= MESON_SAR_ADC_REG11)
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
+				   MESON_SAR_ADC_REG11_BANDGAP_EN,
+				   on_off ? MESON_SAR_ADC_REG11_BANDGAP_EN : 0);
 	else
-		enable_mask = MESON_SAR_ADC_DELTA_10_TS_VBG_EN;
-
-	regmap_update_bits(priv->regmap, param->bandgap_reg, enable_mask,
-			   on_off ? enable_mask : 0);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_VBG_EN,
+				   on_off ? MESON_SAR_ADC_DELTA_10_TS_VBG_EN : 0);
 }
 
 static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
@@ -1205,7 +1202,6 @@ static const struct iio_info meson_sar_adc_iio_info = {
 static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
 	.has_bl30_integration = false,
 	.clock_rate = 1150000,
-	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
 	.regmap_config = &meson_sar_adc_regmap_config_meson8,
 	.resolution = 10,
 	.temperature_trimming_bits = 4,
@@ -1216,7 +1212,6 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
 static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
 	.has_bl30_integration = false,
 	.clock_rate = 1150000,
-	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
 	.regmap_config = &meson_sar_adc_regmap_config_meson8,
 	.resolution = 10,
 	.temperature_trimming_bits = 5,
@@ -1227,10 +1222,8 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
 static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
 	.has_bl30_integration = true,
 	.clock_rate = 1200000,
-	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 10,
-	.has_reg11 = true,
 	.vref_voltage = VREF_VOLTAGE_1V8,
 	.cmv_select = true,
 };
@@ -1238,11 +1231,9 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
 static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 	.has_bl30_integration = true,
 	.clock_rate = 1200000,
-	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 12,
 	.disable_ring_counter = 1,
-	.has_reg11 = true,
 	.vref_voltage = VREF_VOLTAGE_1V8,
 	.cmv_select = true,
 };
@@ -1250,11 +1241,9 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
 	.has_bl30_integration = true,
 	.clock_rate = 1200000,
-	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 12,
 	.disable_ring_counter = 1,
-	.has_reg11 = true,
 	.vref_voltage = VREF_VOLTAGE_1V8,
 	.has_vref_select = true,
 	.vref_select = VREF_VDDA,
@@ -1264,11 +1253,9 @@ static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
 static const struct meson_sar_adc_param meson_sar_adc_g12a_param = {
 	.has_bl30_integration = false,
 	.clock_rate = 1200000,
-	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 12,
 	.disable_ring_counter = 1,
-	.has_reg11 = true,
 	.vref_voltage = VREF_VOLTAGE_0V9,
 	.adc_eoc = true,
 	.has_vref_select = true,
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/3] iio: adc: meson: a few improvements

[Jonathan Cameron]

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

> This series contains three improvements to the meson SAR ADC driver.
> None of them are meant to change the existing behavior. The goal is
> to make the driver code easier to read and understand.
> 
> 
> Martin Blumenstingl (3):
>   iio: adc: meson: fix voltage reference selection field name typo
>   iio: adc: meson: consistently use bool/enum in struct
>     meson_sar_adc_param
>   iio: adc: meson: simplify MESON_SAR_ADC_REG11 register access
> 
>  drivers/iio/adc/meson_saradc.c | 78 ++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 42 deletions(-)
> 

Hi Martin,

Please resend +CC linux-iio@vger.kernel.org

I'll take a quick look but I won't pick up anything that hasn't been on that
list (as I use patchwork to track status etc).

Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel