Hi Andy,
On Wed, Apr 13, 2022 at 07:53:46PM +0530, Jagath Jog J wrote:
> 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".
Sorry, I got my mistake about mutex lock and unlocking and I will correct those
in the next patch series.
Thank you
Jagath
>
>
> > >
> > >
> > >>
> > >> 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
