[PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock
Jonathan Cameron
Jonathan.Cameron at huawei.com
Fri Oct 14 08:21:23 PDT 2022
On Fri, 14 Oct 2022 09:25:59 +0200
Nuno Sá <noname.nuno at gmail.com> wrote:
> On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote:
> > Hi Nuno,
> >
> > nuno.sa at analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200:
> >
> > > 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.
> >
> > What about adding a TODO comment saying something like: "this comes
> > from static analysis and helped dropping mlock access, but someone
> > with
> > the device needs to figure out if we can simplify this dance"?
> > Because
> > the reason behind all this is that we don't want to risk breaking the
> > driver, but perhaps a simpler approach would work, right?
> >
>
> Hi Miquel,
>
> AFAIU, either the device is powered (when buffer mode enabled) and we
> can do the reading or it's not and we need to power it on/off
> "manually" while making sure we don't race against enable/disabling
> buffers. This "dance" is needed mainly to make sure that we grab
> 'mlock' one way or another... The other way would be to use some
> specific device lock together with a flag (as discussed) but as
> discussed with Jonathan we decided to go down this road... So,
> honestly, I don't really see the necessity of "marking" this code with
> a TODO but of course if someone comes in with something simpler, great
> :).
Agreed. I don't expect to see any improvement in this in the future
so a TODO would just be noise and might encourage people to propose
the 'get the lock on it's own function' that we are going through this
dance to avoid adding.
Jonathan
>
> Anyways, as I said, I'm not really keen in spinning a new version to
> add this comment so I will defer the decision to Jonathan :)
>
> Thanks for the help!
> - Nuno Sá
>
More information about the linux-amlogic
mailing list