On Mon, 16 Sep 2019 13:47:34 +0200 Fabrice Gasnier <[email protected]> wrote:
> On 9/15/19 12:05 PM, Jonathan Cameron wrote: > > On Fri, 13 Sep 2019 15:21:30 +0200 > > Fabrice Gasnier <[email protected]> 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 <[email protected]> > > 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 > [email protected] > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
