On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote:
> On Wednesday, April 13, 2022, Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> wrote:
>
> >
> >
> > On Tuesday, April 12, 2022, Jagath Jog J <jagathjog1996@xxxxxxxxx> wrote:
> >
> >> Hello Andy,
> >>
> >> On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote:
> >> > On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@xxxxxxxxx>
> >> wrote:
> >> > >
> >> > > Added trigger buffer support to read continuous acceleration
> >> > > data from device with data ready interrupt which is mapped
> >> > > to INT1 pin.
> >> >
> >> > Can you explain the locking schema in this driver?
> >> >
> >> > > + /* Configure INT1 pin to open drain */
> >> > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG,
> >> 0x06);
> >> > > + if (ret)
> >> > > + return ret;
> >> >
> >> > No locking (or regmap only locking).
> >>
> >> This is bma400_init() function which will run when probe runs so there is
> >> no
> >> locking in this bma400_init().
> >>
> >> >
> >> > ...
> >> >
> >> > > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger
> >> *trig,
> >> > > + bool state)
> >> > > +{
> >> > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> >> > > + struct bma400_data *data = iio_priv(indio_dev);
> >> > > + int ret;
> >> > > +
> >> > > + ret = regmap_update_bits(data->regmap,
> >> BMA400_INT_CONFIG0_REG,
> >> > > + BMA400_INT_DRDY_MSK,
> >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> >> state));
> >> > > + if (ret)
> >> > > + return ret;
> >> > > +
> >> > > + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> >> > > + BMA400_INT_DRDY_MSK,
> >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> >> state));
> >> > > +}
> >> >
> >> > Ditto.
> >>
> >> Sorry, I missed this.
> >> I will add lock and unlocking in the next patch.
> >>
> >> >
> >> > ...
> >> >
> >> > > + mutex_lock(&data->mutex);
> >> > > +
> >> > > + /* bulk read six registers, with the base being the LSB
> >> register */
> >> > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> >> > > + &data->buffer.buff,
> >> sizeof(data->buffer.buff));
> >> > > + mutex_unlock(&data->mutex);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > But here only above with locking...
> >> >
> >> > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > ...followed by no locking.
> >>
> >> Okay I will add lock in the next patch.
> >>
> >> >
> >> > ...
> >> >
> >> > > + mutex_lock(&data->mutex);
> >> > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG,
> >> &status,
> >> > > + sizeof(status));
> >> > > + mutex_unlock(&data->mutex);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > And again with locking.
> >> >
> >> > ...
> >> >
> >> > So,
> >> > 1) Does regmap is configured with locking? What for?
> >> > 2) What's the role of data->mutex?
> >>
> >> 1.
> >> NO,
> >
> >
> > Are you sure?
> >
> >
> >> regmap is not configured with locking.
> >> In the remap_config structure variable below these members are not defined
> >> in the driver.
> >>
> >> struct regmap_config {
> >> regmap_lock lock;
> >> regmap_unlock unlock;
> >> void *lock_arg;
> >>
> >>
> > It means that default may be used.
> >
> >
> >> 2.
> >> data->mutex is used to protect the register read, write access from the
> >> device.
> >>
> >> Is the regmap functions handle locking and unlocking internally if these
> >> below
> >> struct members are not defined?
> >
> >
> > Yes. Look at this: https://elixir.bootlin.com/linux/latest/C/ident/
> > disable_locking
Please your advise will be very helpful for this.
I'm going through the same documentation. In this driver, disable_locking is
not initialized.
The functions which are called in the bma400_init() are not protected by mutex
for regmap since bma400_init() will run when the probe runs.
The functions which are called by read_raw() and write_raw() are protected by
mutex for regmap access.
There are some members in the device's private data structure and they are being
accessed in multiple places in the driver.
struct bma400_data {
enum bma400_power_mode power_mode;
struct bma400_sample_freq sample_freq;
int oversampling_ratio;
int scale;
.....
I think mutex is used to protect these above struct members since they are
critical resource, but in the struct bma400_data comment for mutex
is "data register lock".
> >
> >
> >>
> >> regmap_lock lock;
> >> regmap_unlock unlock;
> >> void *lock_arg;
> >>
> >>
> >> >
> >> > --
> >> > With Best Regards,
> >> > Andy Shevchenko
> >>
> >
> >>
>
> You may read the kernel documentation what those fields mean:
> https://elixir.bootlin.com/linux/latest/source/include/linux/regmap.h#L278
>
>
>
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >
>
> --
> With Best Regards,
> Andy Shevchenko
Thank you,
Jagath
