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

[Matti Vaittinen]

On 26/11/2024 19:55, Jonathan Cameron wrote:
> On Mon, 25 Nov 2024 11:34:36 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > 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