Re: [PATCH v4 1/3] iio: light: add driver for veml6030 ambient light sensor

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Oct 2019 18:02:51 +0530
rishi gupta <gupt21@gmail.com> 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);
> > > +}