[PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[Fabrice Gasnier]

Some I/Os are connected to ADC input channels, when the corresponding bit
in PCSEL register are set on STM32H7 and STM32MP15. This is done in the
prepare routine of stm32-adc driver.
There are constraints here, as PCSEL shouldn't be set when VDDA supply
is disabled. Enabling/disabling of VDDA supply in done via stm32-adc-core
runtime PM routines (before/after ADC is enabled/disabled).

Currently, PCSEL remains set when disabling ADC. Later on, PM runtime
can disable the VDDA supply. This creates some conditions on I/Os that
can start to leak current.
So PCSEL needs to be cleared when disabling the ADC.

Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-adc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5088de8..e3e7541 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -975,6 +975,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
 {
 	struct stm32_adc *adc = iio_priv(indio_dev);
 
+	stm32_adc_writel(adc, STM32H7_ADC_PCSEL, 0);
 	stm32h7_adc_disable(indio_dev);
 	stm32h7_adc_enter_pwr_down(adc);
 }
-- 
2.7.4


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

Re: [PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[Olivier MOYSAN]

Hi Fabrice,

On 10/22/21 2:19 PM, Fabrice Gasnier wrote:
> Some I/Os are connected to ADC input channels, when the corresponding bit
> in PCSEL register are set on STM32H7 and STM32MP15. This is done in the
> prepare routine of stm32-adc driver.
> There are constraints here, as PCSEL shouldn't be set when VDDA supply
> is disabled. Enabling/disabling of VDDA supply in done via stm32-adc-core
> runtime PM routines (before/after ADC is enabled/disabled).
> 
> Currently, PCSEL remains set when disabling ADC. Later on, PM runtime
> can disable the VDDA supply. This creates some conditions on I/Os that
> can start to leak current.
> So PCSEL needs to be cleared when disabling the ADC.
> 
> Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
>   drivers/iio/adc/stm32-adc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 5088de8..e3e7541 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -975,6 +975,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
>   {
>   	struct stm32_adc *adc = iio_priv(indio_dev);
>   
> +	stm32_adc_writel(adc, STM32H7_ADC_PCSEL, 0);
>   	stm32h7_adc_disable(indio_dev);
>   	stm32h7_adc_enter_pwr_down(adc);
>   }
> 

Reviewed-by: Olivier Moysan <olivier.moysan@foss.st.com>

Thanks
Olivier

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

Re: [PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[Jonathan Cameron]

On Fri, 22 Oct 2021 14:38:52 +0200
Olivier MOYSAN <olivier.moysan@foss.st.com> wrote:

I'll probably reword the description here as 'leak' tends to mean memory
leak rather than current.

> Hi Fabrice,
> 
> On 10/22/21 2:19 PM, Fabrice Gasnier wrote:
> > Some I/Os are connected to ADC input channels, when the corresponding bit
> > in PCSEL register are set on STM32H7 and STM32MP15. This is done in the
> > prepare routine of stm32-adc driver.
> > There are constraints here, as PCSEL shouldn't be set when VDDA supply
> > is disabled. Enabling/disabling of VDDA supply in done via stm32-adc-core
> > runtime PM routines (before/after ADC is enabled/disabled).
> > 
> > Currently, PCSEL remains set when disabling ADC. Later on, PM runtime
> > can disable the VDDA supply. This creates some conditions on I/Os that
> > can start to leak current.
> > So PCSEL needs to be cleared when disabling the ADC.
> > 
> > Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
> > 

No line break here as Fixes forms part of the tag block.

> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Given timing wrt to being too near merge window, I'll let this it on
list a while longer as it'll be post rc1 material now anyway.

I can fix the above whilst applying if nothing else comes up.

Jonathan

> > ---
> >   drivers/iio/adc/stm32-adc.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 5088de8..e3e7541 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -975,6 +975,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> >   {
> >   	struct stm32_adc *adc = iio_priv(indio_dev);
> >   
> > +	stm32_adc_writel(adc, STM32H7_ADC_PCSEL, 0);
> >   	stm32h7_adc_disable(indio_dev);
> >   	stm32h7_adc_enter_pwr_down(adc);
> >   }
> >   
> 
> Reviewed-by: Olivier Moysan <olivier.moysan@foss.st.com>
> 
> Thanks
> Olivier


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

Re: [PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[Fabrice Gasnier]

On 10/24/21 6:07 PM, Jonathan Cameron wrote:
> On Fri, 22 Oct 2021 14:38:52 +0200
> Olivier MOYSAN <olivier.moysan@foss.st.com> wrote:
> 
> I'll probably reword the description here as 'leak' tends to mean memory
> leak rather than current.

Hi Jonathan,

Yes, please fell free to improve this. I had the same concern, but I
haven't found a more suitable description.

> 
>> Hi Fabrice,
>>
>> On 10/22/21 2:19 PM, Fabrice Gasnier wrote:
>>> Some I/Os are connected to ADC input channels, when the corresponding bit
>>> in PCSEL register are set on STM32H7 and STM32MP15. This is done in the
>>> prepare routine of stm32-adc driver.
>>> There are constraints here, as PCSEL shouldn't be set when VDDA supply
>>> is disabled. Enabling/disabling of VDDA supply in done via stm32-adc-core
>>> runtime PM routines (before/after ADC is enabled/disabled).
>>>
>>> Currently, PCSEL remains set when disabling ADC. Later on, PM runtime
>>> can disable the VDDA supply. This creates some conditions on I/Os that
>>> can start to leak current.
>>> So PCSEL needs to be cleared when disabling the ADC.
>>>
>>> Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
>>>
> 
> No line break here as Fixes forms part of the tag block.
> 

Sorry, will check this next time.

>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Given timing wrt to being too near merge window, I'll let this it on
> list a while longer as it'll be post rc1 material now anyway.
> 
> I can fix the above whilst applying if nothing else comes up.

That's fine for me.

Many thanks,
Fabrice
> 
> Jonathan
> 
>>> ---
>>>   drivers/iio/adc/stm32-adc.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>>> index 5088de8..e3e7541 100644
>>> --- a/drivers/iio/adc/stm32-adc.c
>>> +++ b/drivers/iio/adc/stm32-adc.c
>>> @@ -975,6 +975,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
>>>   {
>>>   	struct stm32_adc *adc = iio_priv(indio_dev);
>>>   
>>> +	stm32_adc_writel(adc, STM32H7_ADC_PCSEL, 0);
>>>   	stm32h7_adc_disable(indio_dev);
>>>   	stm32h7_adc_enter_pwr_down(adc);
>>>   }
>>>   
>>
>> Reviewed-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>
>> Thanks
>> Olivier
> 

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

Re: [PATCH] iio: adc: stm32: fix a leak by resetting pcsel before disabling vdda

[Jonathan Cameron]

On Mon, 25 Oct 2021 09:43:10 +0200
Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:

> On 10/24/21 6:07 PM, Jonathan Cameron wrote:
> > On Fri, 22 Oct 2021 14:38:52 +0200
> > Olivier MOYSAN <olivier.moysan@foss.st.com> wrote:
> > 
> > I'll probably reword the description here as 'leak' tends to mean memory
> > leak rather than current.  
> 
> Hi Jonathan,
> 
> Yes, please fell free to improve this. I had the same concern, but I
> haven't found a more suitable description.

Added the word current to the title which I think makes it clear.
> 
> >   
> >> Hi Fabrice,
> >>
> >> On 10/22/21 2:19 PM, Fabrice Gasnier wrote:  
> >>> Some I/Os are connected to ADC input channels, when the corresponding bit
> >>> in PCSEL register are set on STM32H7 and STM32MP15. This is done in the
> >>> prepare routine of stm32-adc driver.
> >>> There are constraints here, as PCSEL shouldn't be set when VDDA supply
> >>> is disabled. Enabling/disabling of VDDA supply in done via stm32-adc-core
> >>> runtime PM routines (before/after ADC is enabled/disabled).
> >>>
> >>> Currently, PCSEL remains set when disabling ADC. Later on, PM runtime
> >>> can disable the VDDA supply. This creates some conditions on I/Os that
> >>> can start to leak current.
> >>> So PCSEL needs to be cleared when disabling the ADC.
> >>>
> >>> Fixes: 95e339b6e85d ("iio: adc: stm32: add support for STM32H7")
> >>>  
> > 
> > No line break here as Fixes forms part of the tag block.
> >   
> 
> Sorry, will check this next time.
> 
> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>  
> > 
> > Given timing wrt to being too near merge window, I'll let this it on
> > list a while longer as it'll be post rc1 material now anyway.
> > 
> > I can fix the above whilst applying if nothing else comes up.  
> 
> That's fine for me.

Applied to the fixes-togreg branch of iio.git, targeting a pull request
some time shortly after rc1.

Thanks,

Jonathan

> 
> Many thanks,
> Fabrice
> > 
> > Jonathan
> >   
> >>> ---
> >>>   drivers/iio/adc/stm32-adc.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> >>> index 5088de8..e3e7541 100644
> >>> --- a/drivers/iio/adc/stm32-adc.c
> >>> +++ b/drivers/iio/adc/stm32-adc.c
> >>> @@ -975,6 +975,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> >>>   {
> >>>   	struct stm32_adc *adc = iio_priv(indio_dev);
> >>>   
> >>> +	stm32_adc_writel(adc, STM32H7_ADC_PCSEL, 0);
> >>>   	stm32h7_adc_disable(indio_dev);
> >>>   	stm32h7_adc_enter_pwr_down(adc);
> >>>   }
> >>>     
> >>
> >> Reviewed-by: Olivier Moysan <olivier.moysan@foss.st.com>
> >>
> >> Thanks
> >> Olivier  
> >   


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