diff mbox series

[04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

Message ID 20211215151344.163036-5-miquel.raynal@bootlin.com (mailing list archive)
State Changes Requested
Headers show
Series Light core IIO enhancements | expand

Commit Message

Miquel Raynal Dec. 15, 2021, 3:13 p.m. UTC
This is an internal variable of the core, let's use the
iio_buffer_enabled() helper which is exported for the following purpose:
telling if the current mode is a buffered mode, which is precisely what
this driver looks for.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Alexandru Ardelean Dec. 16, 2021, 6:47 a.m. UTC | #1
On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> This is an internal variable of the core, let's use the
> iio_buffer_enabled() helper which is exported for the following purpose:
> telling if the current mode is a buffered mode, which is precisely what
> this driver looks for.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 1cfefb3b5e56..a3b8827d3bbf 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
>          * In continuous mode, use fast mode configuration,
>          * if it provides a better resolution.
>          */
> -       if (adc->nconv == 1 && !trig &&
> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {

This may become tricky if other modes get added later.
STM does a relatively good job in updating and re-using their drivers
(even if some of them do look quirky sometimes).

So, the question here would be: is "iio_buffer_enabled(indio_dev)"
going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
INDIO_BUFFER_HARDWARE get added?

I'd also ping some STM people for some feedback, acks or testing.

>                 if (fl->flo[1].res >= fl->flo[0].res) {
>                         fl->fast = 1;
>                         flo = &fl->flo[1];
> @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
>                 cr1 = DFSDM_CR1_RCH(chan->channel);
>
>                 /* Continuous conversions triggered by SPI clk in buffer mode */
> -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> +               if (iio_buffer_enabled(indio_dev))
>                         cr1 |= DFSDM_CR1_RCONT(1);
>
>                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> --
> 2.27.0
>
Miquel Raynal Dec. 16, 2021, 8:22 a.m. UTC | #2
Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:

> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > This is an internal variable of the core, let's use the
> > iio_buffer_enabled() helper which is exported for the following purpose:
> > telling if the current mode is a buffered mode, which is precisely what
> > this driver looks for.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> >          * In continuous mode, use fast mode configuration,
> >          * if it provides a better resolution.
> >          */
> > -       if (adc->nconv == 1 && !trig &&
> > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {  
> 
> This may become tricky if other modes get added later.
> STM does a relatively good job in updating and re-using their drivers
> (even if some of them do look quirky sometimes).
> 
> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> INDIO_BUFFER_HARDWARE get added?

I would argue, is this a real problem? Today iio_buffer_enabled() seem
to handle well what this driver is expecting. If tomorrow someone adds
another mode, that is his/her responsibility to state "okay, this
section is not common to all buffer styles *anymore*, so we need to do
a more fine grained check against ->currentmodes than
iio_buffer_enabled() does". In that case using the ->currentmodes
getter would be the right way to go, but only at that particular
moment, not today.

> 
> I'd also ping some STM people for some feedback, acks or testing.
> 
> >                 if (fl->flo[1].res >= fl->flo[0].res) {
> >                         fl->fast = 1;
> >                         flo = &fl->flo[1];
> > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> >
> >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > +               if (iio_buffer_enabled(indio_dev))
> >                         cr1 |= DFSDM_CR1_RCONT(1);
> >
> >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > --
> > 2.27.0
> >  


Thanks,
Miquèl
Jonathan Cameron Jan. 15, 2022, 4:06 p.m. UTC | #3
On Thu, 16 Dec 2021 09:22:35 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > This is an internal variable of the core, let's use the
> > > iio_buffer_enabled() helper which is exported for the following purpose:
> > > telling if the current mode is a buffered mode, which is precisely what
> > > this driver looks for.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> > >          * In continuous mode, use fast mode configuration,
> > >          * if it provides a better resolution.
> > >          */
> > > -       if (adc->nconv == 1 && !trig &&
> > > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {    
> > 
> > This may become tricky if other modes get added later.
> > STM does a relatively good job in updating and re-using their drivers
> > (even if some of them do look quirky sometimes).

Their hardware is crazy/complicated so tends to push the limits!

> > 
> > So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> > going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> > INDIO_BUFFER_HARDWARE get added?  
> 
> I would argue, is this a real problem? Today iio_buffer_enabled() seem
> to handle well what this driver is expecting. If tomorrow someone adds
> another mode, that is his/her responsibility to state "okay, this
> section is not common to all buffer styles *anymore*, so we need to do
> a more fine grained check against ->currentmodes than
> iio_buffer_enabled() does". In that case using the ->currentmodes
> getter would be the right way to go, but only at that particular
> moment, not today.

It should be isolated to this driver, so I think it is fine to use
the broader check today, but I'll leave this to the st folks as
it's their driver and I don't feel that strongly about it.

> 
> > 
> > I'd also ping some STM people for some feedback, acks or testing.

Definitely on this - they are an active bunch who do a great job of looking
after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
others are on v2 cc list).


> >   
> > >                 if (fl->flo[1].res >= fl->flo[0].res) {
> > >                         fl->fast = 1;
> > >                         flo = &fl->flo[1];
> > > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> > >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> > >
> > >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > > +               if (iio_buffer_enabled(indio_dev))
> > >                         cr1 |= DFSDM_CR1_RCONT(1);
> > >
> > >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > > --
> > > 2.27.0
> > >    
> 
> 
> Thanks,
> Miquèl
Miquel Raynal Jan. 28, 2022, 3:04 p.m. UTC | #4
Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:

> On Thu, 16 Dec 2021 09:22:35 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Alexandru,
> > 
> > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> >   
> > > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:    
> > > >
> > > > This is an internal variable of the core, let's use the
> > > > iio_buffer_enabled() helper which is exported for the following purpose:
> > > > telling if the current mode is a buffered mode, which is precisely what
> > > > this driver looks for.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > > > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> > > >          * In continuous mode, use fast mode configuration,
> > > >          * if it provides a better resolution.
> > > >          */
> > > > -       if (adc->nconv == 1 && !trig &&
> > > > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > > > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {      
> > > 
> > > This may become tricky if other modes get added later.
> > > STM does a relatively good job in updating and re-using their drivers
> > > (even if some of them do look quirky sometimes).  
> 
> Their hardware is crazy/complicated so tends to push the limits!
> 
> > > 
> > > So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> > > going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> > > INDIO_BUFFER_HARDWARE get added?    
> > 
> > I would argue, is this a real problem? Today iio_buffer_enabled() seem
> > to handle well what this driver is expecting. If tomorrow someone adds
> > another mode, that is his/her responsibility to state "okay, this
> > section is not common to all buffer styles *anymore*, so we need to do
> > a more fine grained check against ->currentmodes than
> > iio_buffer_enabled() does". In that case using the ->currentmodes
> > getter would be the right way to go, but only at that particular
> > moment, not today.  
> 
> It should be isolated to this driver, so I think it is fine to use
> the broader check today, but I'll leave this to the st folks as
> it's their driver and I don't feel that strongly about it.
> 
> >   
> > > 
> > > I'd also ping some STM people for some feedback, acks or testing.  
> 
> Definitely on this - they are an active bunch who do a great job of looking
> after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
> others are on v2 cc list).
> 

I'll add Olivier Moysan as well in the next version who has been quite
active on this driver as well according to git log.

> 
> > >     
> > > >                 if (fl->flo[1].res >= fl->flo[0].res) {
> > > >                         fl->fast = 1;
> > > >                         flo = &fl->flo[1];
> > > > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> > > >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> > > >
> > > >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > > > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > > > +               if (iio_buffer_enabled(indio_dev))
> > > >                         cr1 |= DFSDM_CR1_RCONT(1);
> > > >
> > > >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > > > --
> > > > 2.27.0
> > > >      
> > 
> > 
> > Thanks,
> > Miquèl  
> 


Thanks,
Miquèl
Fabrice Gasnier Feb. 1, 2022, 8:41 a.m. UTC | #5
On 1/28/22 4:04 PM, Miquel Raynal wrote:
> Hi Jonathan,
> 
> jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:
> 
>> On Thu, 16 Dec 2021 09:22:35 +0100
>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>> Hi Alexandru,
>>>
>>> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
>>>   
>>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
>>>> <miquel.raynal@bootlin.com> wrote:    
>>>>>
>>>>> This is an internal variable of the core, let's use the
>>>>> iio_buffer_enabled() helper which is exported for the following purpose:
>>>>> telling if the current mode is a buffered mode, which is precisely what
>>>>> this driver looks for.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
>>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> index 1cfefb3b5e56..a3b8827d3bbf 100644
>>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
>>>>>          * In continuous mode, use fast mode configuration,
>>>>>          * if it provides a better resolution.
>>>>>          */
>>>>> -       if (adc->nconv == 1 && !trig &&
>>>>> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
>>>>> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {      
>>>>
>>>> This may become tricky if other modes get added later.
>>>> STM does a relatively good job in updating and re-using their drivers
>>>> (even if some of them do look quirky sometimes).  
>>
>> Their hardware is crazy/complicated so tends to push the limits!
>>
>>>>
>>>> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
>>>> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
>>>> INDIO_BUFFER_HARDWARE get added?    
>>>
>>> I would argue, is this a real problem? Today iio_buffer_enabled() seem
>>> to handle well what this driver is expecting. If tomorrow someone adds
>>> another mode, that is his/her responsibility to state "okay, this
>>> section is not common to all buffer styles *anymore*, so we need to do
>>> a more fine grained check against ->currentmodes than
>>> iio_buffer_enabled() does". In that case using the ->currentmodes
>>> getter would be the right way to go, but only at that particular
>>> moment, not today.  
>>
>> It should be isolated to this driver, so I think it is fine to use
>> the broader check today, but I'll leave this to the st folks as
>> it's their driver and I don't feel that strongly about it.

Hi Miquel, Alexandru, Jonathan, all,

First, sorry for the delay.

Indeed, I don't expect any functional changes here by using
iio_buffer_enabled(indio_dev).
So it should be fine to use it. You're right, the driver looks for
buffer mode in both places where this gets used.

Just an additional statement is: the driver also checks for no trigger,
and single channel in both places (to select desired mode in the dfsdm).
As I see, only INDIO_BUFFER_SOFTWARE is expected then.

For my own understanding (I'm just asking), why not using the
currentmodes getter routine ?

I've had a look at the whole series [1], It seems used elsewhere. I may
miss something... It would be 100% equivalent to current code to use:
iio_get_internal_mode(indio_dev) & INDIO_BUFFER_SOFTWARE ?

This would be safe in case new modes gets introduced later ?
(another note: unless these new modes gets set by default in the 'modes'
field, this should have no impact here as well anyway ?)

[1]
https://lore.kernel.org/linux-iio/CA+U=DsoMLD1EpK7yDXaQ_HwTQgm_TeZvM_eykE6jWYgg6P3Y7w@mail.gmail.com/T/


>>
>>>   
>>>>
>>>> I'd also ping some STM people for some feedback, acks or testing.  
>>
>> Definitely on this - they are an active bunch who do a great job of looking
>> after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
>> others are on v2 cc list).
>>
> 
> I'll add Olivier Moysan as well in the next version who has been quite
> active on this driver as well according to git log.

Indeed, please add both Olivier and I next time.

Best Regards,
Thanks,
Fabrice

> 
>>
>>>>     
>>>>>                 if (fl->flo[1].res >= fl->flo[0].res) {
>>>>>                         fl->fast = 1;
>>>>>                         flo = &fl->flo[1];
>>>>> @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
>>>>>                 cr1 = DFSDM_CR1_RCH(chan->channel);
>>>>>
>>>>>                 /* Continuous conversions triggered by SPI clk in buffer mode */
>>>>> -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
>>>>> +               if (iio_buffer_enabled(indio_dev))
>>>>>                         cr1 |= DFSDM_CR1_RCONT(1);
>>>>>
>>>>>                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
>>>>> --
>>>>> 2.27.0
>>>>>      
>>>
>>>
>>> Thanks,
>>> Miquèl  
>>
> 
> 
> Thanks,
> Miquèl
>
Miquel Raynal Feb. 2, 2022, 9:33 a.m. UTC | #6
Hi Fabrice,

fabrice.gasnier@foss.st.com wrote on Tue, 1 Feb 2022 09:41:03 +0100:

> On 1/28/22 4:04 PM, Miquel Raynal wrote:
> > Hi Jonathan,
> > 
> > jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:
> >   
> >> On Thu, 16 Dec 2021 09:22:35 +0100
> >> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>  
> >>> Hi Alexandru,
> >>>
> >>> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> >>>     
> >>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> >>>> <miquel.raynal@bootlin.com> wrote:      
> >>>>>
> >>>>> This is an internal variable of the core, let's use the
> >>>>> iio_buffer_enabled() helper which is exported for the following purpose:
> >>>>> telling if the current mode is a buffered mode, which is precisely what
> >>>>> this driver looks for.
> >>>>>
> >>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> ---
> >>>>>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> >>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> index 1cfefb3b5e56..a3b8827d3bbf 100644
> >>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> >>>>>          * In continuous mode, use fast mode configuration,
> >>>>>          * if it provides a better resolution.
> >>>>>          */
> >>>>> -       if (adc->nconv == 1 && !trig &&
> >>>>> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> >>>>> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {        
> >>>>
> >>>> This may become tricky if other modes get added later.
> >>>> STM does a relatively good job in updating and re-using their drivers
> >>>> (even if some of them do look quirky sometimes).    
> >>
> >> Their hardware is crazy/complicated so tends to push the limits!
> >>  
> >>>>
> >>>> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> >>>> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> >>>> INDIO_BUFFER_HARDWARE get added?      
> >>>
> >>> I would argue, is this a real problem? Today iio_buffer_enabled() seem
> >>> to handle well what this driver is expecting. If tomorrow someone adds
> >>> another mode, that is his/her responsibility to state "okay, this
> >>> section is not common to all buffer styles *anymore*, so we need to do
> >>> a more fine grained check against ->currentmodes than
> >>> iio_buffer_enabled() does". In that case using the ->currentmodes
> >>> getter would be the right way to go, but only at that particular
> >>> moment, not today.    
> >>
> >> It should be isolated to this driver, so I think it is fine to use
> >> the broader check today, but I'll leave this to the st folks as
> >> it's their driver and I don't feel that strongly about it.  
> 
> Hi Miquel, Alexandru, Jonathan, all,
> 
> First, sorry for the delay.
> 
> Indeed, I don't expect any functional changes here by using
> iio_buffer_enabled(indio_dev).
> So it should be fine to use it. You're right, the driver looks for
> buffer mode in both places where this gets used.
> 
> Just an additional statement is: the driver also checks for no trigger,
> and single channel in both places (to select desired mode in the dfsdm).
> As I see, only INDIO_BUFFER_SOFTWARE is expected then.

Ok, thanks for the validation, do not hesitate to drop a Reviewed-by to
the next version of this series if you agree with the changes made here.

> For my own understanding (I'm just asking), why not using the
> currentmodes getter routine ?
> 
> I've had a look at the whole series [1], It seems used elsewhere. I may
> miss something... It would be 100% equivalent to current code to use:
> iio_get_internal_mode(indio_dev) & INDIO_BUFFER_SOFTWARE ?
> 
> This would be safe in case new modes gets introduced later ?
> (another note: unless these new modes gets set by default in the 'modes'
> field, this should have no impact here as well anyway ?)

I would argue that this is more a conceptual change. IMHO:
- currentmode is a variable that should have been kept internal
- checking against its value directly is kind of a hack and should be
  avoided when possible because we want the core to have full freedom
  over the way it manages these flags
- if you want to verify if buffers are enabled, then the core offers
  you a dedicated helper that does exactly this, and will do it better
  than if hardcoded by individual writers, generally

And it's not "used elsewhere" anymore thanks to this series :) only two
drivers _really_ need to check the actual current mode to do specific
actions, but that's all.

I hope it clarifies a bit.

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 1cfefb3b5e56..a3b8827d3bbf 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -466,8 +466,7 @@  static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
 	 * In continuous mode, use fast mode configuration,
 	 * if it provides a better resolution.
 	 */
-	if (adc->nconv == 1 && !trig &&
-	    (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
+	if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {
 		if (fl->flo[1].res >= fl->flo[0].res) {
 			fl->fast = 1;
 			flo = &fl->flo[1];
@@ -562,7 +561,7 @@  static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
 		cr1 = DFSDM_CR1_RCH(chan->channel);
 
 		/* Continuous conversions triggered by SPI clk in buffer mode */
-		if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
+		if (iio_buffer_enabled(indio_dev))
 			cr1 |= DFSDM_CR1_RCONT(1);
 
 		cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);