Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support

[Jagath Jog J <jagathjog1996@gmail.com>]

On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote:
> On Wednesday, April 13, 2022, Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
> 
> >
> >
> > On Tuesday, April 12, 2022, Jagath Jog J <jagathjog1996@gmail.com> 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@gmail.com>
> >> 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