On 1/28/22 4:04 PM, Miquel Raynal wrote:
> Hi Jonathan,
>
> jic23@xxxxxxxxxx wrote on Sat, 15 Jan 2022 16:06:19 +0000:
>
>> On Thu, 16 Dec 2021 09:22:35 +0100
>> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>>
>>> Hi Alexandru,
>>>
>>> ardeleanalex@xxxxxxxxx wrote on Thu, 16 Dec 2021 08:47:02 +0200:
>>>
>>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
>>>> <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxx/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
>
