| Message ID | 20240923-veml6035-v2-8-58c72a0df31c@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: veml6030: fix issues and add support for veml6035 | expand |
On Mon, 23 Sep 2024 00:17:56 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > Move devm_add_action_or_reset() with a device shut down action above the > hardware initialization function to ensure that any error path after > powering on the device leads to a power off. > > The power off action is carried out by setting the VEML6030_ALS_SD bit > of the VEML6030_REG_ALS_CONF, which is harmless in error paths were the > device is already off. On the other hand, making use of the registered > action in all error paths makes them more homogeneous by avoiding > special action depending on the current power state of the device. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> As per my very late reply, I'd move the devm_add_action_or_reset() into the hw_init function so it's closely coupled with the thing it is undoing rather than with a function that does lots of other things. You'll want to pass dev into the init function though so it is easy to see what device the devm_ call is against. Don't use the parent of the IIO dev as that's an implementation detail! Jonathan > --- > drivers/iio/light/veml6030.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c > index 861bdf2edd4d..19c69bfad8cb 100644 > --- a/drivers/iio/light/veml6030.c > +++ b/drivers/iio/light/veml6030.c > @@ -853,12 +853,12 @@ static int veml6030_probe(struct i2c_client *client) > indio_dev->info = &veml6030_info_no_irq; > } > > - ret = veml6030_hw_init(indio_dev); > + ret = devm_add_action_or_reset(&client->dev, > + veml6030_als_shut_down_action, data); > if (ret < 0) > return ret; > > - ret = devm_add_action_or_reset(&client->dev, > - veml6030_als_shut_down_action, data); > + ret = veml6030_hw_init(indio_dev); > if (ret < 0) > return ret; > >
diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c index 861bdf2edd4d..19c69bfad8cb 100644 --- a/drivers/iio/light/veml6030.c +++ b/drivers/iio/light/veml6030.c @@ -853,12 +853,12 @@ static int veml6030_probe(struct i2c_client *client) indio_dev->info = &veml6030_info_no_irq; } - ret = veml6030_hw_init(indio_dev); + ret = devm_add_action_or_reset(&client->dev, + veml6030_als_shut_down_action, data); if (ret < 0) return ret; - ret = devm_add_action_or_reset(&client->dev, - veml6030_als_shut_down_action, data); + ret = veml6030_hw_init(indio_dev); if (ret < 0) return ret;
Move devm_add_action_or_reset() with a device shut down action above the hardware initialization function to ensure that any error path after powering on the device leads to a power off. The power off action is carried out by setting the VEML6030_ALS_SD bit of the VEML6030_REG_ALS_CONF, which is harmless in error paths were the device is already off. On the other hand, making use of the registered action in all error paths makes them more homogeneous by avoiding special action depending on the current power state of the device. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/light/veml6030.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)