Re: [PATCH v2 1/4] iio: chemical: scd30: add core driver

[Tomasz Duszynski <tomasz.duszynski@octakon.com>]

On Mon, Jun 01, 2020 at 02:41:20PM +0100, Jonathan Cameron wrote:
> ...
>
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > > > > > +		scd30_command_t command)
> > > > > > +{
> > > > > > +	static const unsigned long scd30_scan_masks[] = { 0x07, 0x00 };
> > > > > > +	struct scd30_state *state;
> > > > > > +	struct iio_dev *indio_dev;
> > > > > > +	int ret;
> > > > > > +	u16 val;
> > > > > > +
> > > > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > > > > > +	if (!indio_dev)
> > > > > > +		return -ENOMEM;
> > > > > > +
> > > > > > +	state = iio_priv(indio_dev);
> > > > > > +	state->dev = dev;
> > > > >
> > > > > Doesn't seem to be used.
> > > > >
> > > > > > +	state->priv = priv;
> > > > >
> > > > > What's this for?  At least at first glance I can't find it being used
> > > > > anywhere.
> > > > >
> > > > > > +	state->irq = irq;
> > > > > > +	state->pressure_comp = SCD30_PRESSURE_COMP_DEFAULT;
> > > > > > +	state->meas_interval = SCD30_MEAS_INTERVAL_DEFAULT;
> > > > > > +	state->command = command;
> > > > > > +	mutex_init(&state->lock);
> > > > > > +	init_completion(&state->meas_ready);
> > > > > > +
> > > > > > +	dev_set_drvdata(dev, indio_dev);
> > > > > > +
> > > > > > +	indio_dev->dev.parent = dev;
> > > > >
> > > > > Side note that there is a series moving this into the core under revision at
> > > > > the moment.  Hopefully I'll remember to fix this up when applying your patch
> > > > > if that one has gone in ahead of it.
> > > > >
> > > > > > +	indio_dev->info = &scd30_info;
> > > > > > +	indio_dev->name = name;
> > > > > > +	indio_dev->channels = scd30_channels;
> > > > > > +	indio_dev->num_channels = ARRAY_SIZE(scd30_channels);
> > > > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > > +	indio_dev->available_scan_masks = scd30_scan_masks;
> > > > > > +
> > > > > > +	state->vdd = devm_regulator_get(dev, "vdd");
> > > > > > +	if (IS_ERR(state->vdd)) {
> > > > > > +		if (PTR_ERR(state->vdd) == -EPROBE_DEFER)
> > > > > > +			return -EPROBE_DEFER;
> > > > > > +
> > > > > > +		dev_err(dev, "failed to get regulator\n");
> > > > > > +		return PTR_ERR(state->vdd);
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = regulator_enable(state->vdd);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = devm_add_action_or_reset(dev, scd30_disable_regulator, state);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > >
> > > > > A comment here on why it makes sense to register this here.  What
> > > > > started mesurement? It seems that happens well below here so
> > > > > we should really call this after that start all.
> > > > >
> > > >
> > > > Sensor after being powered up starts in mode it was left in.
> > > > Chances are it was continuous mode and we want to shut it down.
> > >
> > > That's fine.  The question is why 'here' as opposed to after the below where you
> > > put it into continuous mode.
> > >
> >
> > Let's suppose sensor got energized and started measuring. Then without
> > registering action which stops measurement we jump to device reset etc.
> >
> > Now if reset failed for whatever reason (same applies to everything
> > below reset) devm will gracefully unwind previous actions but sensor
> > will continue doing his job. But there's no point. Better to save some
> > milliaps for later.
>
> I'm not convinced. Note that in your example, the sensor was already on.
> If we never loaded the driver it would stay on.  We should
> deal with only problems we have potentially created.
>

Fair enough.

> If reset fails, it shouldn't 'enable' the sensor hence we are just in
> the same state as if the driver hadn't loaded at all.  Also if reset
> fails the chance of a disable succeeding is very low.
>
> The code is much more logical with this being done immediately after
> the action to enable thing we are disabling.  So please put it down
> there.
>
>
>
> >
> > In case we have real regulator then there's no real issue because
> > power gets cut off during cleanup.
> >
> > Quite often though there's only a dummy one which does nothing useful
> > except making regulator framework happy.
> >
> > So my thinking here is that we're slightly better off registering
> > scd30_stop_meas() action earlier to prevent such scenario from happening.
> >
> > > >
> > > > > > +	ret = devm_add_action_or_reset(dev, scd30_stop_meas, state);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = scd30_reset(state);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "failed to reset device: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (state->irq > 0) {
> > > > > > +		ret = scd30_setup_trigger(indio_dev);
> > > > > > +		if (ret) {
> > > > > > +			dev_err(dev, "failed to setup trigger: %d\n", ret);
> > > > > > +			return ret;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > > > > +					      scd30_trigger_handler, NULL);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = scd30_command_read(state, CMD_FW_VERSION, &val);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "failed to read firmware version: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +	dev_info(dev, "firmware version: %d.%d\n", val >> 8, (char)val);
> > > > > > +
> > > > > > +	ret = scd30_command_write(state, CMD_MEAS_INTERVAL,
> > > > > > +				  state->meas_interval);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "failed to set measurement interval: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = scd30_command_write(state, CMD_START_MEAS, state->pressure_comp);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "failed to start measurement: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> Here is where we should register that cleanup handler.
>
> Jonathan
>
> > > > > > +
> > > > > > +	return devm_iio_device_register(dev, indio_dev);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(scd30_probe);
> > > > > > +
> > > > > > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
> > > > > > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > > > > > +MODULE_LICENSE("GPL v2");
> > > > > > --
> > > > > > 2.26.2
> > > > > >
> > > > >
> > >
> > >
>
>