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

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

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, regmap is not configured with locking. 
In the regmap_config structure variable below these members are not defined
in the driver.

struct regmap_config {
	regmap_lock lock;
	regmap_unlock unlock;
	void *lock_arg;

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?

regmap_lock lock;
regmap_unlock unlock;
void *lock_arg;


> 
> -- 
> With Best Regards,
> Andy Shevchenko