On Mon, 16 Sep 2019 13:47:34 +0200
Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote:
> On 9/15/19 12:05 PM, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2019 15:21:30 +0200
> > Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote:
> >
> >> End of conversion may be handled by using IRQ or DMA. There may be a
> >> race when two conversions complete at the same time on several ADCs.
> >> EOC can be read as 'set' for several ADCs, with:
> >> - an ADC configured to use IRQs. EOCIE bit is set. The handler is normally
> >> called in this case.
> >> - an ADC configured to use DMA. EOCIE bit isn't set. EOC triggers the DMA
> >> request instead. It's then automatically cleared by DMA read. But the
> >> handler gets called due to status bit is temporarily set (IRQ triggered
> >> by the other ADC).
> >> So both EOC status bit in CSR and EOCIE control bit must be checked
> >> before invoking the interrupt handler (e.g. call ISR only for
> >> IRQ-enabled ADCs).
> >>
> >> Fixes: 2763ea0585c9 ("iio: adc: stm32: add optional dma support")
> >>
> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx>
> > Fix looks fine to me, but I'm not keen on splitting out individual bits from
> > register defines. That's a long term readability nightmare.
> >
> > See below,
> >
> > Jonathan
> >
> >> ---
> >> drivers/iio/adc/stm32-adc-core.c | 43 +++++++++++++++++++++++++++++++++++++---
> >> drivers/iio/adc/stm32-adc-core.h | 13 ++++++++++++
> >> drivers/iio/adc/stm32-adc.c | 6 ------
> >> 3 files changed, 53 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> >> index 9b85fef..7297396 100644
> >> --- a/drivers/iio/adc/stm32-adc-core.c
> >> +++ b/drivers/iio/adc/stm32-adc-core.c
> >> @@ -71,6 +71,8 @@
> >> * @eoc1: adc1 end of conversion flag in @csr
> >> * @eoc2: adc2 end of conversion flag in @csr
> >> * @eoc3: adc3 end of conversion flag in @csr
> >> + * @ier: interrupt enable register offset for each adc
> >> + * @eocie_msk: end of conversion interrupt enable mask in @ier
> >> */
> >> struct stm32_adc_common_regs {
> >> u32 csr;
> >> @@ -78,6 +80,8 @@ struct stm32_adc_common_regs {
> >> u32 eoc1_msk;
> >> u32 eoc2_msk;
> >> u32 eoc3_msk;
> >> + u32 ier;
> >> + u32 eocie_msk;
> >> };
> >>
> >> struct stm32_adc_priv;
> >> @@ -303,6 +307,8 @@ static const struct stm32_adc_common_regs stm32f4_adc_common_regs = {
> >> .eoc1_msk = STM32F4_EOC1,
> >> .eoc2_msk = STM32F4_EOC2,
> >> .eoc3_msk = STM32F4_EOC3,
> >> + .ier = STM32F4_ADC_CR1,
> >> + .eocie_msk = STM32F4_EOCIE,
> >> };
> >>
> >> /* STM32H7 common registers definitions */
> >> @@ -311,8 +317,24 @@ static const struct stm32_adc_common_regs stm32h7_adc_common_regs = {
> >> .ccr = STM32H7_ADC_CCR,
> >> .eoc1_msk = STM32H7_EOC_MST,
> >> .eoc2_msk = STM32H7_EOC_SLV,
> >> + .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,
> >> +};
> >> +
> >> +static unsigned int stm32_adc_eoc_enabled(struct stm32_adc_priv *priv,
> >> + unsigned int adc)
> >> +{
> >> + u32 ier, offset = stm32_adc_offset[adc];
> >> +
> >> + ier = readl_relaxed(priv->common.base + offset + priv->cfg->regs->ier);
> >> +
> >> + return ier & priv->cfg->regs->eocie_msk;
> >> +}
> >> +
> >> /* ADC common interrupt for all instances */
> >> static void stm32_adc_irq_handler(struct irq_desc *desc)
> >> {
> >> @@ -323,13 +345,28 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
> >> chained_irq_enter(chip, desc);
> >> status = readl_relaxed(priv->common.base + priv->cfg->regs->csr);
> >>
> >> - if (status & priv->cfg->regs->eoc1_msk)
> >> + /*
> >> + * End of conversion may be handled by using IRQ or DMA. There may be a
> >> + * race here when two conversions complete at the same time on several
> >> + * ADCs. EOC may be read 'set' for several ADCs, with:
> >> + * - an ADC configured to use DMA (EOC triggers the DMA request, and
> >> + * is then automatically cleared by DR read in hardware)
> >> + * - an ADC configured to use IRQs (EOCIE bit is set. The handler must
> >> + * be called in this case)
> >> + * So both EOC status bit in CSR and EOCIE control bit must be checked
> >> + * before invoking the interrupt handler (e.g. call ISR only for
> >> + * IRQ-enabled ADCs).
> >> + */
> >> + if (status & priv->cfg->regs->eoc1_msk &&
> >> + stm32_adc_eoc_enabled(priv, 0))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 0));
> >>
> >> - if (status & priv->cfg->regs->eoc2_msk)
> >> + if (status & priv->cfg->regs->eoc2_msk &&
> >> + stm32_adc_eoc_enabled(priv, 1))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 1));
> >>
> >> - if (status & priv->cfg->regs->eoc3_msk)
> >> + if (status & priv->cfg->regs->eoc3_msk &&
> >> + stm32_adc_eoc_enabled(priv, 2))
> >> generic_handle_irq(irq_find_mapping(priv->domain, 2));
> >>
> >> chained_irq_exit(chip, desc);
> >> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> >> index 8af507b..8dc936b 100644
> >> --- a/drivers/iio/adc/stm32-adc-core.h
> >> +++ b/drivers/iio/adc/stm32-adc-core.h
> >> @@ -25,8 +25,21 @@
> >> * --------------------------------------------------------
> >> */
> >> #define STM32_ADC_MAX_ADCS 3
> >> +#define STM32_ADC_OFFSET 0x100
> >> #define STM32_ADCX_COMN_OFFSET 0x300
> >>
> >> +/* STM32F4 - registers for each ADC instance */
> >> +#define STM32F4_ADC_CR1 0x04
> >> +
> >> +/* STM32F4_ADC_CR1 - bit fields */
> >> +#define STM32F4_EOCIE BIT(5)
> >> +
> >> +/* STM32H7 - registers for each instance */
> >> +#define STM32H7_ADC_IER 0x04
> >> +
> >> +/* STM32H7_ADC_IER - bit fields */
> >> +#define STM32H7_EOCIE BIT(2)
> >> +
> >> /**
> >> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
> >> * @base: control registers base cpu addr
> >> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> >> index 6a7dd08..3c9f456 100644
> >> --- a/drivers/iio/adc/stm32-adc.c
> >> +++ b/drivers/iio/adc/stm32-adc.c
> >> @@ -30,7 +30,6 @@
> >>
> >> /* STM32F4 - Registers for each ADC instance */
> >> #define STM32F4_ADC_SR 0x00
> >> -#define STM32F4_ADC_CR1 0x04
> >> #define STM32F4_ADC_CR2 0x08
> >> #define STM32F4_ADC_SMPR1 0x0C
> >> #define STM32F4_ADC_SMPR2 0x10
> >> @@ -54,7 +53,6 @@
> >> #define STM32F4_RES_SHIFT 24
> >> #define STM32F4_RES_MASK GENMASK(25, 24)
> >> #define STM32F4_SCAN BIT(8)
> >> -#define STM32F4_EOCIE BIT(5)
> > Hmm. This is breaking up the definitions of bits in a single register.
> > That is rather nasty from a code readability point of view.
> >
> > I am as keen as the next person on only exposing definitions where
> > we need to, but in this case we either need to provide an access path
> > to it here, or we need to move the whole block to the header.
>
> Hi Jonathan,
>
> I think I'll add a precursor patch in v2 to move the whole block to the
> header file. This way, the access path is easy (e.g. readl).
> I'm only wondering about the Fixes tag... this will probably not be
> straight forward to apply the fix on the maintenance releases ?
> Or do I need to add it to the precursor patch as well ?
The precursor is a simple move of definitions. Even if it's large, I don't
think it will be a problem applying it to stable.
Just make it clear in the patch description why it is needed for the fix.
Thanks,
Jonathan
>
> Thanks for reviewing,
> Best regards,
> Fabrice
>
> >
> >>
> >> /* STM32F4_ADC_CR2 - bit fields */
> >> #define STM32F4_SWSTART BIT(30)
> >> @@ -69,7 +67,6 @@
> >>
> >> /* STM32H7 - Registers for each ADC instance */
> >> #define STM32H7_ADC_ISR 0x00
> >> -#define STM32H7_ADC_IER 0x04
> >> #define STM32H7_ADC_CR 0x08
> >> #define STM32H7_ADC_CFGR 0x0C
> >> #define STM32H7_ADC_SMPR1 0x14
> >> @@ -89,9 +86,6 @@
> >> #define STM32H7_EOC BIT(2)
> >> #define STM32H7_ADRDY BIT(0)
> >>
> >> -/* STM32H7_ADC_IER - bit fields */
> >> -#define STM32H7_EOCIE STM32H7_EOC
> >> -
> >> /* STM32H7_ADC_CR - bit fields */
> >> #define STM32H7_ADCAL BIT(31)
> >> #define STM32H7_ADCALDIF BIT(30)
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
