On Tue, 22 Oct 2019 18:02:51 +0530
rishi gupta <gupt21@xxxxxxxxx> wrote:
> Thanks Jonathan, sorry for deep thread, learnt will keep in mind.
>
> All suggested changes done except re-ordering devm_add_action_or_reset.
> Please see inline and suggest if I missed something.
>
...
> > > +static int veml6030_probe(struct i2c_client *client,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + int ret;
> > > + struct veml6030_data *data;
> > > + struct iio_dev *indio_dev;
> > > + struct regmap *regmap;
> > > +
> > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > > + dev_err(&client->dev, "i2c adapter doesn't support plain i2c\n");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + regmap = devm_regmap_init_i2c(client, &veml6030_regmap_config);
> > > + if (IS_ERR(regmap)) {
> > > + dev_err(&client->dev, "can't setup regmap\n");
> > > + return PTR_ERR(regmap);
> > > + }
> > > +
> > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + data = iio_priv(indio_dev);
> > > + i2c_set_clientdata(client, indio_dev);
> > > + data->client = client;
> > > + data->regmap = regmap;
> > > +
> > > + indio_dev->dev.parent = &client->dev;
> > > + indio_dev->name = "veml6030";
> > > + indio_dev->channels = veml6030_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(veml6030_channels);
> > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > + if (client->irq) {
> > > + ret = devm_request_threaded_irq(&client->dev, client->irq,
> > > + NULL, veml6030_event_handler,
> > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > > + "veml6030", indio_dev);
> > > + if (ret < 0) {
> > > + dev_err(&client->dev,
> > > + "irq %d request failed\n", client->irq);
> > > + return ret;
> > > + }
> > > + indio_dev->info = &veml6030_info;
> > > + } else {
> > > + indio_dev->info = &veml6030_info_no_irq;
> > > + }
> > > +
> > > + ret = devm_add_action_or_reset(&client->dev,
> > > + veml6030_als_shut_down_action, data);
> >
> > What is this reversing? It should be immediately after whatever that is, thus
> > ensuring we only undo whatever we need to on failure and the ordering is correct
> > for remove. I am guessing it should be after hw_init.
> >
> This just disables active measurements (this is the only thing we need
> to do when failure happens).
>
> Suppose hw initialisation succeeds but call to
> devm_add_action_or_reset() fails. In this case sensor will be left
> turned on as veml6030_als_shut_down_action() will never be executed.
> Therefore I kept it before veml6030_hw_init().
> Does this sounds correct to you ?
Nope, that's the point of the _or_reset part of that call. Note that we used
to manually handle the result of devm_add_action, but this little wrapper
does that for us.
In all failure cases it will run the callback provided to it.
https://elixir.bootlin.com/linux/v4.8/source/include/linux/device.h#L688
So it should always be called 'after' the thing it is setting up the
unwinding function for.
Jonathan
>
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = veml6030_hw_init(indio_dev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return devm_iio_device_register(&client->dev, indio_dev);
> > > +}
