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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 9 Jun 2022 18:09:05 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

> Hello Jonathan,
> 
> Thank you for comments. Please find my replies below.
> 
...

> > > +					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?

There are special cases:
- only one trigger supported.
- originally only one trigger was supported, but that got relaxed later
  and we need to maintain the default to avoid ABI changes.
- maybe one or two that slipped through.

We didn't start setting default triggers until fairly recently so lots
of drivers don't set one.  That doesn't mean we shoudn't fix the
issue you identified but in this case it's a policy decision so probably
belongs in userspace anyway.


...
> > > +
> > > +	/* 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.

It's a trade off, but generally we don't spend too much effort printing
errors for things that aren't reasonably likely to happen. Obviously
we do return the error though so that we know some debugging is needed
if it happens!

> 
> > > +
> > > +	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)
> 
That's fine, but add a comment to explain 2.
As a general rule, devm_ actions clearly match against setup path actions
so when they don't it's useful to provide a little more info.

Jonathan