On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@xxxxxxxxxxx> wrote:
>
> The st_sensors_core driver hardcodes the content of the
> iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> helpers. Let's get rid of this handcrafted implementation and use the
> proper core helpers instead. Additionally, this lowers the tab level
> (which is always good) and prevents the use of the ->currentmode
> variable which is not supposed to be used like this anyway.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
> .../iio/common/st_sensors/st_sensors_core.c | 28 +++++++++----------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 1de395bda03e..e57e85c06f4b 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> int err;
> struct st_sensor_data *sdata = iio_priv(indio_dev);
>
> - mutex_lock(&indio_dev->mlock);
> - if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> - err = -EBUSY;
> + err = iio_device_claim_direct_mode(indio_dev);
I'm afraid, for this driver, we would first need a cleanup of
indio_dev->mlock usage.
Or at least that's how I would start it.
i.e. remove the indio_dev->mlock and replace it with it's own
mutex/lock in all places (except this one).
The whole story about mlock is a bit old.
As I was told, it was initially defined in the iio_dev object, but not
very strictly controlled during review [of drivers].
Drivers kept using it (as a convenience lock).
It was later defined to be an IIO framework lock.
Now, there's a (slow) ongoing work to move mlock inside the
iio_dev_opaque struct, and make each driver use it's own lock, OR use
iio_device_{claim,release}_direct_mode() where appropriate.
FWIW: this change could go in as-is.
But there's still the point of implementing another lock on the
st_sensor_data type.
I would try to split this work into another [parallel] series, because
otherwise [if fitted into this series] it would just grow and be
slow-to-review series.
But ¯\_(ツ)_/¯
> + if (err)
> + return err;
> +
> + err = st_sensors_set_enable(indio_dev, true);
> + if (err < 0)
> goto out;
> - } else {
> - err = st_sensors_set_enable(indio_dev, true);
> - if (err < 0)
> - goto out;
>
> - msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> - err = st_sensors_read_axis_data(indio_dev, ch, val);
> - if (err < 0)
> - goto out;
> + msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> + err = st_sensors_read_axis_data(indio_dev, ch, val);
> + if (err < 0)
> + goto out;
>
> - *val = *val >> ch->scan_type.shift;
> + *val = *val >> ch->scan_type.shift;
>
> - err = st_sensors_set_enable(indio_dev, false);
> - }
> + err = st_sensors_set_enable(indio_dev, false);
> out:
> - mutex_unlock(&indio_dev->mlock);
> + iio_device_release_direct_mode(indio_dev);
>
> return err;
> }
> --
> 2.27.0
>
