by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 3/8] iio: adc: stm32-adc: add stm32mp13 support

On Wed, 28 Sep 2022 18:41:09 +0200
Olivier Moysan <[email protected]> wrote:

> Add STM32 ADC support for STM32MP13x SOCs family.
>
> On STM32MP13x, each ADC peripheral has a single ADC block.

Wrap this a bit nearer 80 chars. Personally I go with 75, but anywhere around
that is fine.

> These ADC peripherals, ADC1 and ADC2, are fully independent.
> This introduces changes in common registers handling.
>
> Some features such as boost mode, channel preselection and
> linear calibration are not supported by the STM32MP13x ADC.
> Add diversity management for these features.
>
> The STM32MP13x ADC introduces registers and bitfield variants
> on existing features such as calibration factors and internal
> channels. Add register diversity management.
>
> Add also support of new internal channels VDDCPU and VDDQ_DDR.
>
> Signed-off-by: Olivier Moysan <[email protected]>
Hi Oliver

A few really trivial things inline,

Jonathan

> ---
> drivers/iio/adc/stm32-adc-core.c | 21 +++
> drivers/iio/adc/stm32-adc-core.h | 32 +++++
> drivers/iio/adc/stm32-adc.c | 212 +++++++++++++++++++++++++++----
> 3 files changed, 237 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> index 81d5db91c67b..6564aa61b595 100644
> --- a/drivers/iio/adc/stm32-adc-core.c
> +++ b/drivers/iio/adc/stm32-adc-core.c
> @@ -322,6 +322,16 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
> .eocie_msk = STM32H7_EOCIE,
> };
>
> +/* STM32MP13 common registers definitions */
> +static const struct stm32_adc_common_regs stm32mp13_adc_common_regs = {
> + .csr = STM32H7_ADC_CSR,
> + .ccr = STM32H7_ADC_CCR,
> + .eoc_msk = { STM32H7_EOC_MST},

space before },
ouch. I see this odd balance is common in this driver. Don't carry it on and
if you fancy adding the missing spaces to the rest of the driver that would be
great!

> + .ovr_msk = { STM32H7_OVR_MST},
> + .ier = STM32H7_ADC_IER,
> + .eocie_msk = STM32H7_EOCIE,
> +};
> +
> static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = {
> 0, STM32_ADC_OFFSET, STM32_ADC_OFFSET * 2,
> };
> @@ -868,6 +878,14 @@ static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = {
> .num_irqs = 2,
> };
>
> +static const struct stm32_adc_priv_cfg stm32mp13_adc_priv_cfg = {
> + .regs = &stm32mp13_adc_common_regs,
> + .clk_sel = stm32h7_adc_clk_sel,
> + .max_clk_rate_hz = 75000000,
> + .ipid = STM32MP13_IPIDR_NUMBER,
> + .num_irqs = 1,
> +};
> +
> static const struct of_device_id stm32_adc_of_match[] = {
> {
> .compatible = "st,stm32f4-adc-core",
> @@ -878,6 +896,9 @@ static const struct of_device_id stm32_adc_of_match[] = {
> }, {
> .compatible = "st,stm32mp1-adc-core",
> .data = (void *)&stm32mp1_adc_priv_cfg
> + }, {
> + .compatible = "st,stm32mp13-adc-core",
> + .data = (void *)&stm32mp13_adc_priv_cfg
> }, {
> },
> };
> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> index 2118ef63843d..658fef4308ac 100644
> --- a/drivers/iio/adc/stm32-adc-core.h
> +++ b/drivers/iio/adc/stm32-adc-core.h
> @@ -112,6 +112,11 @@
> #define STM32MP1_ADC_IPDR 0x3F8
> #define STM32MP1_ADC_SIDR 0x3FC
>
> +/* STM32MP13 - Registers for each ADC instance */
> +#define STM32MP13_ADC_DIFSEL 0xB0
> +#define STM32MP13_ADC_CALFACT 0xB4
> +#define STM32MP13_ADC2_OR 0xC8
> +
> /* STM32H7 - common registers for all ADC instances */
> #define STM32H7_ADC_CSR (STM32_ADCX_COMN_OFFSET + 0x00)
> #define STM32H7_ADC_CCR (STM32_ADCX_COMN_OFFSET + 0x08)
> @@ -161,6 +166,10 @@ enum stm32h7_adc_dmngt {
> STM32H7_DMNGT_DMA_CIRC, /* DMA circular mode */
> };
>
> +/* STM32H7_ADC_DIFSEL - bit fields */
> +#define STM32H7_DIFSEL_SHIFT 0

This shift is both unnecessary, as encoded in the mask, and unused
so you can definitely get rid of it.
Might be nice to clean the rest of these out as well in favour of
just using the masks and FIELD_PREP() etc for any places where the mask
is a compile time constant.

Please check the others are actually useful.

> +#define STM32H7_DIFSEL_MASK GENMASK(19, 0)
> +
> /* STM32H7_ADC_CALFACT - bit fields */
> #define STM32H7_CALFACT_D_SHIFT 16
> #define STM32H7_CALFACT_D_MASK GENMASK(26, 16)
> @@ -210,7 +219,30 @@ enum stm32h7_adc_dmngt {
> /* STM32MP1_ADC_SIDR - bit fields */
> #define STM32MP1_SIDR_MASK GENMASK(31, 0)
...