[PATCH v2 15/16] iio: health: max30102: do not use internal iio_dev lock

[Jonathan Cameron]

On Sun, 9 Oct 2022 12:44:40 +0100
Jonathan Cameron <jic23 at kernel.org> wrote:

> On Wed, 05 Oct 2022 10:17:00 +0200
> Nuno Sá <noname.nuno at gmail.com> wrote:
> 
> > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote:  
> > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa at analog.com> wrote:    
> > > > 
> > > > The pattern used in this device does not quite fit in the
> > > > iio_device_claim_direct_mode() typical usage. In this case, we want
> > > > to
> > > > know if we are in buffered mode or not to know if the device is
> > > > powered
> > > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > > will
> > > > power on the device if needed. Hence, in order to keep the same
> > > > functionality, we try to:
> > > > 
> > > > 1. Claim Buffered mode;
> > > > 2: If 1) succeeds call max30102_get_temp() without powering on the
> > > >    device;
> > > > 3: Release Buffered mode;
> > > > 4: If 1) fails, Claim Direct mode;
> > > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > > device;
> > > > 6: Release Direct mode;
> > > > 7: If 4) fails, goto to 1) and try again.
> > > > 
> > > > This dance between buffered and direct mode is not particularly
> > > > pretty
> > > > (as well as the loop introduced by the goto statement) but it does
> > > > allow
> > > > us to get rid of the mlock usage while keeping the same behavior.    
> > > 
> > > ...
> > >     
> > > > +               if (iio_device_claim_buffer_mode(indio_dev)) {    
> > > 
> > > Why is ret not used here?
> > >     
> > > > +                       /*
> > > > +                        * This one is a *bit* hacky. If we cannot
> > > > claim buffer
> > > > +                        * mode, then try direct mode so that we
> > > > make sure
> > > > +                        * things cannot concurrently change. And
> > > > we just keep
> > > > +                        * trying until we get one of the modes...
> > > > +                        */
> > > > +                       if
> > > > (iio_device_claim_direct_mode(indio_dev))    
> > > 
> > > ...and here?
> > >     
> > > > +                               goto any_mode_retry;    
> > >     
> > > > +               } else {    
> > >     
> > > > +               }    
> > > 
> > > I.o.w. what error code will be visible to the caller and why.
> > >     
> > 
> > Note that we do not really care about the error code returned by both
> > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We
> > just loop until we get one of the modes (thus ret = 0) so that we can
> > safely call one of the max30102_get_temp() variants. And that will be
> > the visible error code (if any). That said, you can look at the first
> > version of the series about this (and the previous patch) and why this
> > is being done like this (note that I'm also not 100% convinced about
> > this ping pong between getting one of the IIO modes but it's also not
> > that bad and if there's no major complains, I'm fine with it).  
> 
> This is a vanishingly rare corner case and not in a remotely high performance
> path so I'm not keen on introducing a more complex API just to handle
> this corner. If we added a means to get the lock independent of mode
> we'd have an interface that is far to likely to get missused.
> What you have here does the job and looks nasty enough to put people off
> copying it unless they really need it!
> 
> Jonathan
> 
I should probably have said lgtm for how you have it here.

Jonathan