Re: [PATCH v2 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

[Dmitry Rokosov]

Hello Jonathan,

Thank you for comments. Please find my replies below.

On Sat, May 28, 2022 at 07:33:34PM +0100, Jonathan Cameron wrote:
> On Wed, 25 May 2022 18:15:32 +0000
> Dmitry Rokosov <DDRokosov@xxxxxxxxxxxxxx> wrote:
> 
> > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > sensitivity consumer applications. It has dynamical user selectable full
> > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > with output data rates from 1Hz to 1000Hz.
> > 
> > Datasheet can be found at following URL:
> > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > 
> > This driver supports following MSA311 features:
> >     - IIO interface
> >     - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> >     - ODR (Output Data Rate) selection
> >     - Scale and samp_freq selection
> >     - IIO triggered buffer, IIO reg access
> >     - NEW_DATA interrupt + trigger
> > 
> > Below features to be done:
> >     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx>
> 
> Hi Dmitry,
> 
> Various comments inline. One thing to think about is which of the comments
> / function documentation is useful and which is just stating the obvious.
> If things are obvious it is usually better to not add documentation that
> doesn't provide additional insight because it provides a maintenance
> burden going forwards.
> 
> In a similar fashion, consider if a failure path that isn't already resulting
> in a print is remotely likely.  Error messages are something else that cause
> maintenance burden, so there has to be some advantage in having them to
> pay that cost.
> 
> Thanks,
> 
> Jonathan
> 

Sure, no problem. I'll send updated driver without obvious comments and
error messages in v3 patch series.

> > +/**
> > + * msa311_write_raw() - IIO interface function to write attr/accel data
> > + * @indio_dev: The IIO device associated with MSA311
> > + * @chan: IIO channel specification
> > + * @val: Input data value, first part
> > + * @val2: Input data value, second part
> > + * @mask: Value type selector
> > + *
> > + * Return: 0 on success,
> > + *         -EINVAL for unknown value type (bad mask),
> > + *         -ERRNO in other failures
> > + */
> > +static int msa311_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val, int val2, long mask)
> > +{
> > +	struct msa311_priv *msa311 = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (iio_buffer_enabled(indio_dev))
> 
> This races.  We have
> iio_device_claim_direct_mode() and the matching
> release to avoid such races.  They will ensure we are in
> a mode where the buffer isn't enabled for the duration of
> any action like this. 
> 
> 
> Mind you, why can't we write the scale while the buffer is turned on?
> It might be unwise given no way of knowing when it applies, but
> that is a problem for userspace dumb enough to do it ;)
> 
> If there is a reason to not do so, good to add a comment here
> to say why not.  Same obviously applies to sampling frequency below.

I've inserted such condition, because I used pm_runtime() calls inside
trig_set_state() callback, which was wrong behavior (as you correctly
mentioned before). In those situations, if userspace changed scale or freq
during a buffer reading, it was a time slot where device could go to sleep,
and this is a bad thing. Anyway, for now I'm using pm_runtime() callbacks
during buffer_enable and buffer_disable executions, so I can remove this
condition from scale/freq write operations and race will gone.

> > +					i2c->name,
> > +					indio_dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "failed to request irq (%d)\n", err);
> > +
> > +	trig = devm_iio_trigger_alloc(dev, "%s-new-data", i2c->name);
> > +	if (!trig)
> > +		return dev_err_probe(dev, -ENOMEM,
> > +				     "cannot allocate newdata trig\n");
> > +
> > +	msa311->new_data_trig = trig;
> > +	msa311->new_data_trig->dev.parent = dev;
> > +	msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
> > +	iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
> > +
> > +	err = devm_iio_trigger_register(dev, msa311->new_data_trig);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "can't register newdata trig (%d)\n", err);
> > +
> > +	indio_dev->trig = iio_trigger_get(msa311->new_data_trig);
> 
> Drop this.  Your driver now supports other triggers so leave
> this decision to userspace (thus avoiding the issue with remove discussed in
> v1).
> 

Okay, but many other drivers have such the same problem. May be it's
better to stay in the consistent state with them? What do you think?

> > +
> > +	/* Resume device if any */
> > +	pm_runtime_get_sync(dev);
> > +
> > +	/* Don't use autosuspend PM runtime feature anymore */
> > +	pm_runtime_dont_use_autosuspend(dev);
> 
> this is done for you in the unwind of
> devm_pm_runtime_enable() so If you need to repeat it here, explain why.
> 

As I understood, devm_pm_runtime_enable() executes pm_runtime_disable()
during resource utilization. I'm not sure that pm_runtime_disable()
switches off autosuspend feature. I'll take a look and remove this line
if needed.

> > +
> > +	/* Suspend device right now */
> > +	pm_runtime_put_sync_suspend(dev);
> 
> At this point is this any different from pm_runtime_put_sync?
> 

Yes. Function pm_runtime_put_sync() transfers device to IDLE state if
needed, we do not want it here. Using pm_runtime_put_sync_suspend() our
goal is for msa311 transition to SUSPEND state when driver unloading.

> > +}
> > +
> > +/**
> > + * msa311_probe() - MSA311 main I2C driver probe function
> > + * @i2c: I2C client associated with MSA311 device
> > + *
> > + * Return: 0 on success
> > + *         -ENOMEM due to memory allocation failures
> > + *         -ERRNO in other failures
> > + */
> > +static int msa311_probe(struct i2c_client *i2c)
> > +{
> > +	struct msa311_priv *msa311;
> > +	struct iio_dev *indio_dev;
> > +	struct device *dev = &i2c->dev;
> > +	int err;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*msa311));
> > +	if (!indio_dev)
> > +		return dev_err_probe(dev, -ENOMEM,
> > +				     "iio device allocation failed\n");
> > +
> > +	msa311 = iio_priv(indio_dev);
> > +	msa311->i2c = i2c;
> > +	i2c_set_clientdata(i2c, indio_dev);
> > +
> > +	err = msa311_regmap_init(msa311);
> > +	if (err)
> > +		return err;
> > +
> > +	mutex_init(&msa311->lock);
> > +
> > +	err = devm_pm_runtime_enable(dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "cannot enable runtime PM (%d)\n", err);
> > +
> > +	/* Resume msa311 logic before any interactions with registers */
> > +	err = pm_runtime_resume_and_get(dev);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "failed to resume runtime pm (%d)\n", err);
> 
> Given you already report an error message on the failure path in resume,
> having one here as well is probably excessive as any other failure case
> is very unlikely.
> 

This dev_err() message is located here, because
pm_runtime_resume_and_get() can contain internal errors which are not
dependent on driver logic. So I try to catch such errors in this place.

> > +
> > +	pm_runtime_set_autosuspend_delay(dev, MSA311_PWR_SLEEP_DELAY_MS);
> > +	pm_runtime_use_autosuspend(dev);
> > +
> > +	err = msa311_chip_init(msa311);
> > +	if (err)
> > +		return err;
> > +
> > +	indio_dev->modes = INDIO_DIRECT_MODE; /* setup buffered mode later */
> > +	indio_dev->channels = msa311_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(msa311_channels);
> > +	indio_dev->name = i2c->name;
> > +	indio_dev->info = &msa311_info;
> > +
> > +	err = devm_iio_triggered_buffer_setup(dev,
> > +					      indio_dev,
> > +					      iio_pollfunc_store_time,
> > +					      msa311_buffer_thread,
> > +					      &msa311_buffer_setup_ops);
> > +	if (err)
> > +		return dev_err_probe(dev, err,
> > +				     "cannot setup iio trig buf (%d)\n", err);
> > +
> > +	if (i2c->irq > 0) {
> > +		err = msa311_setup_interrupts(msa311);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +
> > +	err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
> 
> It's not immediately clear what this devm action corresponds to and hence
> why it is at this point in the probe.  Needs a comment.  I think it's
> a way of forcing suspend to have definitely occurred?
> 

Above devm action is needed to force driver to go to SUSPEND mode during
unloading. In other words, the device should be in SUSPEND mode in the two
cases:
1) When driver is loaded, but we do not have any data or configuration
requests to it (we are solving it using autosuspend feature)
2) When driver is unloaded and device is not used (devm action is used
in this case)

-- 
Thank you,
Dmitry