Re: [PATCH v2 3/8] iio: accel: kx022a: Use cleanup.h helpers

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 26/11/2024 19:55, Jonathan Cameron wrote:
> On Mon, 25 Nov 2024 11:34:36 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hello Jonathan,
>>
>> Thanks again!
>>
>> On 23/11/2024 18:42, Jonathan Cameron wrote:
>>> On Thu, 21 Nov 2024 10:20:23 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> A few functions in KX022A need to use mutex for protecting the
>>>> enabling/disabling of the measurement while configurations are being
>>>> made. Some of the functions can be slightly simplified by using the
>>>> __cleanup based scoped mutexes, which allows dropping the goto based
>>>> unlocking at error path.
>>>>
>>>> Simplify error paths using guard(mutex).
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>> Now we have guard(), the main reason (I think) for the
>>> combined on + lock and off + unlock paths is gone. So can
>>> we just flatten those and do the locking at caller.
>>
>> I did consider this too :)
>>
>> Why I decided to keep it as it is, (even though we need the extra
>> mutex_unlock() at certain error path) is because I kind of like the
>> lock+off and unlock+on functions. This locking does not protect data,
>> but really a sequence of operations that needs to be done while sensor
>> is OFF state. It's almost like a doc saying that "please, ensure the
>> sensor is OFF for the following operations" :)
> 
> hmm.  I really don't like them because they are 'unusual'  :)

I could argue these aren't totally unusual, perhaps unusual in IIO. I 
fell in love with this type of functions when Guenter suggested this 
approach for me in the wdg. Well, IIO is your territory so I'll mutilate 
this file accordingly.

> I'd argue they just ensure a sequence of writes go in as an atomic thing.
> Two of those writes happen to be turn it off and turn it on.

Well, the data-sheet is very clear what comes to clearing the PC1 bit 
when the various CNTL register are touched:
https://fscdn.rohm.com/kionix/en/datasheet/kx022acr-z-e.pdf
(at the beginning of various CNTL register descriptions). So, the on/off 
thing is not something that just happens - and this is what these 
functions did try to underline :)

> So the data the are protecting is the device internal state data.
> 
>>
>> (Another thing is that we do claim the direct mode in write_raw, and
>> goto is still handy for releasing it. Scoped guards won't play nicely
>> with goto. Yes, we could probably use the __cleanup for direct mode, but
>> I still like the lock+off, unlock+on for the reason above)
> There is a nice new cleanup that David did to make the direct mode
> handling much cleaner.
> 
> 	if_not_cond_guard(iio_claim_direct_try, indio_dev)
> 		return -EBUSY;

Ah. Nice. This is not yet in the iio_testing though. I'll add this 'drop 
the off+lock, on+unlock -functions change as an individual patch. It'll 
depend on the if_not_cond_guard() while the rest of the patches should 
have no dependencies to any "not yet in iio_testing" stuff.

I do have the patches ready for sending but I don't have sensors to test 
this at home. I'll give this a try at the office tomorrow and send it 
out then.

Yours,
     -- Matti