| Message ID | 20240323231309.415425-3-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: meson: a few improvements | expand |
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, > };
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, > };
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
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
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, };
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(-)