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

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

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@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".

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