| Message ID | 20210718203746.7159-1-theobf@usp.br (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: dac: max5821: convert device register to device managed function | expand |
On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > Add a device managed hook, via devm_add_action_or_reset() and > max5821_regulator_disable(), to disable voltage regulator on device > detach. > Replace iio_device_register() by devm_iio_device_register() and remove > the max5821_remove() function used to unregister the device and disable the > voltage regulator. > Remove i2c_set_clientdata() from the probe function, since > i2c_get_clientdata() is not used anymore. Looks good overall. A few comments inline. > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > --- > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > index bd6e75699a63..44c04ae70b32 100644 > --- a/drivers/iio/dac/max5821.c > +++ b/drivers/iio/dac/max5821.c > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > .write_raw = max5821_write_raw, > }; > > +static void max5821_regulator_disable(void *data) > +{ > + struct regulator *rdata = data; > + > + regulator_disable(rdata); This can be simplified a bit: static void max5821_regulator_disable(void *reg) { regulator_disable(reg); } I used to do explicit casting, but then I also figured that it's not necessary. > +} > + > static int max5821_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > if (!indio_dev) > return -ENOMEM; > data = iio_priv(indio_dev); > - i2c_set_clientdata(client, indio_dev); > data->client = client; > mutex_init(&data->lock); > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > goto error_free_reg; > } > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > + data->vref_reg); > + if (ret) { > + dev_err(&client->dev, > + "Failed to add action to managed regulator: %d\n", ret); > + goto error_disable_reg; return ret; devm_add_action_or_reset() should call max5821_regulator_disable() in case of error > + } > + > ret = regulator_get_voltage(data->vref_reg); > if (ret < 0) { > dev_err(&client->dev, > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &max5821_info; > > - return iio_device_register(indio_dev); > + return devm_iio_device_register(&client->dev, indio_dev); > > error_disable_reg: This entire goto block should be removed. The idea of using only devm_ functions is to not have these goto statements. > regulator_disable(data->vref_reg); > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > return ret; > } > > -static int max5821_remove(struct i2c_client *client) > -{ > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > - struct max5821_data *data = iio_priv(indio_dev); > - > - iio_device_unregister(indio_dev); > - regulator_disable(data->vref_reg); > - > - return 0; > -} > - > static const struct i2c_device_id max5821_id[] = { > { "max5821", ID_MAX5821 }, > { } > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > .pm = &max5821_pm_ops, > }, > .probe = max5821_probe, > - .remove = max5821_remove, > .id_table = max5821_id, > }; > module_i2c_driver(max5821_driver); > -- > 2.20.1 >
Hi, Alexandru. Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean <ardeleanalex@gmail.com> escreveu: > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > Add a device managed hook, via devm_add_action_or_reset() and > > max5821_regulator_disable(), to disable voltage regulator on device > > detach. > > Replace iio_device_register() by devm_iio_device_register() and remove > > the max5821_remove() function used to unregister the device and disable the > > voltage regulator. > > Remove i2c_set_clientdata() from the probe function, since > > i2c_get_clientdata() is not used anymore. > > Looks good overall. > A few comments inline. > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > --- > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > > index bd6e75699a63..44c04ae70b32 100644 > > --- a/drivers/iio/dac/max5821.c > > +++ b/drivers/iio/dac/max5821.c > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > > .write_raw = max5821_write_raw, > > }; > > > > +static void max5821_regulator_disable(void *data) > > +{ > > + struct regulator *rdata = data; > > + > > + regulator_disable(rdata); > > This can be simplified a bit: > > static void max5821_regulator_disable(void *reg) > { > regulator_disable(reg); > } > > I used to do explicit casting, but then I also figured that it's not necessary. > Ok. > > +} > > + > > static int max5821_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > > if (!indio_dev) > > return -ENOMEM; > > data = iio_priv(indio_dev); > > - i2c_set_clientdata(client, indio_dev); > > data->client = client; > > mutex_init(&data->lock); > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > > goto error_free_reg; > > } > > > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > > + data->vref_reg); > > + if (ret) { > > + dev_err(&client->dev, > > + "Failed to add action to managed regulator: %d\n", ret); > > + goto error_disable_reg; > > return ret; > > devm_add_action_or_reset() should call max5821_regulator_disable() in > case of error > Ok. > > + } > > + > > ret = regulator_get_voltage(data->vref_reg); > > if (ret < 0) { > > dev_err(&client->dev, > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->info = &max5821_info; > > > > - return iio_device_register(indio_dev); > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > error_disable_reg: > > This entire goto block should be removed. > The idea of using only devm_ functions is to not have these goto statements. > I thought the action added via devm_add_action (and devres_add) was called only on driver detach, thus the error_disable_reg label would be necessary to handle the possible error on regulator_get_voltage. Could you please clarify for me when does a driver detach happen? Thanks for your reply, Théo > > regulator_disable(data->vref_reg); > > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > > return ret; > > } > > > > -static int max5821_remove(struct i2c_client *client) > > -{ > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > - struct max5821_data *data = iio_priv(indio_dev); > > - > > - iio_device_unregister(indio_dev); > > - regulator_disable(data->vref_reg); > > - > > - return 0; > > -} > > - > > static const struct i2c_device_id max5821_id[] = { > > { "max5821", ID_MAX5821 }, > > { } > > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > > .pm = &max5821_pm_ops, > > }, > > .probe = max5821_probe, > > - .remove = max5821_remove, > > .id_table = max5821_id, > > }; > > module_i2c_driver(max5821_driver); > > -- > > 2.20.1 > >
On Mon, Jul 19, 2021 at 6:25 PM Théo Borém Fabris <theobf@usp.br> wrote: > > Hi, Alexandru. > > Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean > <ardeleanalex@gmail.com> escreveu: > > > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > Add a device managed hook, via devm_add_action_or_reset() and > > > max5821_regulator_disable(), to disable voltage regulator on device > > > detach. > > > Replace iio_device_register() by devm_iio_device_register() and remove > > > the max5821_remove() function used to unregister the device and disable the > > > voltage regulator. > > > Remove i2c_set_clientdata() from the probe function, since > > > i2c_get_clientdata() is not used anymore. > > > > Looks good overall. > > A few comments inline. > > > > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > > --- > > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > > > index bd6e75699a63..44c04ae70b32 100644 > > > --- a/drivers/iio/dac/max5821.c > > > +++ b/drivers/iio/dac/max5821.c > > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > > > .write_raw = max5821_write_raw, > > > }; > > > > > > +static void max5821_regulator_disable(void *data) > > > +{ > > > + struct regulator *rdata = data; > > > + > > > + regulator_disable(rdata); > > > > This can be simplified a bit: > > > > static void max5821_regulator_disable(void *reg) > > { > > regulator_disable(reg); > > } > > > > I used to do explicit casting, but then I also figured that it's not necessary. > > > Ok. > > > > +} > > > + > > > static int max5821_probe(struct i2c_client *client, > > > const struct i2c_device_id *id) > > > { > > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > > > if (!indio_dev) > > > return -ENOMEM; > > > data = iio_priv(indio_dev); > > > - i2c_set_clientdata(client, indio_dev); > > > data->client = client; > > > mutex_init(&data->lock); > > > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > > > goto error_free_reg; > > > } > > > > > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > > > + data->vref_reg); > > > + if (ret) { > > > + dev_err(&client->dev, > > > + "Failed to add action to managed regulator: %d\n", ret); > > > + goto error_disable_reg; > > > > return ret; > > > > devm_add_action_or_reset() should call max5821_regulator_disable() in > > case of error > > > Ok. > > > > + } > > > + > > > ret = regulator_get_voltage(data->vref_reg); > > > if (ret < 0) { > > > dev_err(&client->dev, > > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > indio_dev->info = &max5821_info; > > > > > > - return iio_device_register(indio_dev); > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > > > error_disable_reg: > > > > This entire goto block should be removed. > > The idea of using only devm_ functions is to not have these goto statements. > > > I thought the action added via devm_add_action (and devres_add) was called only > on driver detach, thus the error_disable_reg label would be necessary > to handle the devm_add_action() yes this is devm_add_action_or_reset() which looks like this: static inline int devm_add_action_or_reset(struct device *dev, void (*action)(void *), void *data) { int ret; ret = devm_add_action(dev, action, data); if (ret) action(data); return ret; } it can be found in "include/linux/device.h" > possible error on regulator_get_voltage. Could you please clarify for > me when does > a driver detach happen? a driver detach happens when: * the kmod is unloaded (assuming the driver is running as a kmod) * manually unbinding the driver from sysfs ; a quick article about this [it's for USB, but other interfaces use the same mechanism]: https://lwn.net/Articles/143397/ there should be something under /sys/bus/spi/drivers/xxxx/unbind * when the system powers down (reboots) * maybe there is some other new method to do this [that I don't know about] > > Thanks for your reply, > Théo > > > > regulator_disable(data->vref_reg); > > > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > > > return ret; > > > } > > > > > > -static int max5821_remove(struct i2c_client *client) > > > -{ > > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > > - struct max5821_data *data = iio_priv(indio_dev); > > > - > > > - iio_device_unregister(indio_dev); > > > - regulator_disable(data->vref_reg); > > > - > > > - return 0; > > > -} > > > - > > > static const struct i2c_device_id max5821_id[] = { > > > { "max5821", ID_MAX5821 }, > > > { } > > > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > > > .pm = &max5821_pm_ops, > > > }, > > > .probe = max5821_probe, > > > - .remove = max5821_remove, > > > .id_table = max5821_id, > > > }; > > > module_i2c_driver(max5821_driver); > > > -- > > > 2.20.1 > > >
> From: Alexandru Ardelean <ardeleanalex@gmail.com> > Sent: Tuesday, July 20, 2021 9:23 AM > To: Théo Borém Fabris <theobf@usp.br> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; linux-iio <linux-iio@vger.kernel.org>; LKML > <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] iio: dac: max5821: convert device register to > device managed function > > On Mon, Jul 19, 2021 at 6:25 PM Théo Borém Fabris <theobf@usp.br> > wrote: > > > > Hi, Alexandru. > > > > Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean > > <ardeleanalex@gmail.com> escreveu: > > > > > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris > <theobf@usp.br> wrote: > > > > > > > > Add a device managed hook, via devm_add_action_or_reset() > and > > > > max5821_regulator_disable(), to disable voltage regulator on > device > > > > detach. > > > > Replace iio_device_register() by devm_iio_device_register() and > remove > > > > the max5821_remove() function used to unregister the device > and disable the > > > > voltage regulator. > > > > Remove i2c_set_clientdata() from the probe function, since > > > > i2c_get_clientdata() is not used anymore. > > > > > > Looks good overall. > > > A few comments inline. > > > > > > > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > > > --- > > > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/iio/dac/max5821.c > b/drivers/iio/dac/max5821.c > > > > index bd6e75699a63..44c04ae70b32 100644 > > > > --- a/drivers/iio/dac/max5821.c > > > > +++ b/drivers/iio/dac/max5821.c > > > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info > = { > > > > .write_raw = max5821_write_raw, > > > > }; > > > > > > > > +static void max5821_regulator_disable(void *data) > > > > +{ > > > > + struct regulator *rdata = data; > > > > + > > > > + regulator_disable(rdata); > > > > > > This can be simplified a bit: > > > > > > static void max5821_regulator_disable(void *reg) > > > { > > > regulator_disable(reg); > > > } > > > > > > I used to do explicit casting, but then I also figured that it's not > necessary. > > > > > Ok. > > > > > > +} > > > > + > > > > static int max5821_probe(struct i2c_client *client, > > > > const struct i2c_device_id *id) > > > > { > > > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client > *client, > > > > if (!indio_dev) > > > > return -ENOMEM; > > > > data = iio_priv(indio_dev); > > > > - i2c_set_clientdata(client, indio_dev); > > > > data->client = client; > > > > mutex_init(&data->lock); > > > > > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct > i2c_client *client, > > > > goto error_free_reg; > > > > } > > > > > > > > + ret = devm_add_action_or_reset(&client->dev, > max5821_regulator_disable, > > > > + data->vref_reg); > > > > + if (ret) { > > > > + dev_err(&client->dev, > > > > + "Failed to add action to managed regulator: %d\n", > ret); > > > > + goto error_disable_reg; > > > > > > return ret; > > > > > > devm_add_action_or_reset() should call > max5821_regulator_disable() in > > > case of error > > > > > Ok. > > > > > > + } > > > > + > > > > ret = regulator_get_voltage(data->vref_reg); > > > > if (ret < 0) { > > > > dev_err(&client->dev, > > > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client > *client, > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > indio_dev->info = &max5821_info; > > > > > > > > - return iio_device_register(indio_dev); > > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > > > > > error_disable_reg: > > > > > > This entire goto block should be removed. > > > The idea of using only devm_ functions is to not have these goto > statements. > > > > > I thought the action added via devm_add_action (and devres_add) > was called only > > on driver detach, thus the error_disable_reg label would be > necessary > > to handle the > > devm_add_action() yes > this is devm_add_action_or_reset() which looks like this: > > static inline int devm_add_action_or_reset(struct device *dev, > void (*action)(void *), void *data) > { > int ret; > > ret = devm_add_action(dev, action, data); > if (ret) > action(data); > > return ret; > } > > it can be found in "include/linux/device.h" > > > possible error on regulator_get_voltage. Could you please clarify for > > me when does > > a driver detach happen? > > a driver detach happens when: > * the kmod is unloaded (assuming the driver is running as a kmod) > * manually unbinding the driver from sysfs ; > a quick article about this [it's for USB, but other interfaces use > the same mechanism]: > > https://urldefense.com/v3/__https://lwn.net/Articles/143397/__;!!A > 3Ni8CS0y2Y!rw- > qxijVlohZWCG9q_wWFPGASqW7JWcfS48uJSwY8coKiBPoA9Z0G2WSCZ > clCw$ > there should be something under /sys/bus/spi/drivers/xxxx/unbind > * when the system powers down (reboots) > * maybe there is some other new method to do this [that I don't know > about] If the device get's deleted with 'device_del()'... - Nuno Sá
Em ter., 20 de jul. de 2021 às 04:22, Alexandru Ardelean <ardeleanalex@gmail.com> escreveu: > > On Mon, Jul 19, 2021 at 6:25 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > Hi, Alexandru. > > > > Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean > > <ardeleanalex@gmail.com> escreveu: > > > > > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > > > Add a device managed hook, via devm_add_action_or_reset() and > > > > max5821_regulator_disable(), to disable voltage regulator on device > > > > detach. > > > > Replace iio_device_register() by devm_iio_device_register() and remove > > > > the max5821_remove() function used to unregister the device and disable the > > > > voltage regulator. > > > > Remove i2c_set_clientdata() from the probe function, since > > > > i2c_get_clientdata() is not used anymore. > > > > > > Looks good overall. > > > A few comments inline. > > > > > > > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > > > --- > > > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > > > > index bd6e75699a63..44c04ae70b32 100644 > > > > --- a/drivers/iio/dac/max5821.c > > > > +++ b/drivers/iio/dac/max5821.c > > > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > > > > .write_raw = max5821_write_raw, > > > > }; > > > > > > > > +static void max5821_regulator_disable(void *data) > > > > +{ > > > > + struct regulator *rdata = data; > > > > + > > > > + regulator_disable(rdata); > > > > > > This can be simplified a bit: > > > > > > static void max5821_regulator_disable(void *reg) > > > { > > > regulator_disable(reg); > > > } > > > > > > I used to do explicit casting, but then I also figured that it's not necessary. > > > > > Ok. > > > > > > +} > > > > + > > > > static int max5821_probe(struct i2c_client *client, > > > > const struct i2c_device_id *id) > > > > { > > > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > > > > if (!indio_dev) > > > > return -ENOMEM; > > > > data = iio_priv(indio_dev); > > > > - i2c_set_clientdata(client, indio_dev); > > > > data->client = client; > > > > mutex_init(&data->lock); > > > > > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > > > > goto error_free_reg; > > > > } > > > > > > > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > > > > + data->vref_reg); > > > > + if (ret) { > > > > + dev_err(&client->dev, > > > > + "Failed to add action to managed regulator: %d\n", ret); > > > > + goto error_disable_reg; > > > > > > return ret; > > > > > > devm_add_action_or_reset() should call max5821_regulator_disable() in > > > case of error > > > > > Ok. > > > > > > + } > > > > + > > > > ret = regulator_get_voltage(data->vref_reg); > > > > if (ret < 0) { > > > > dev_err(&client->dev, > > > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > indio_dev->info = &max5821_info; > > > > > > > > - return iio_device_register(indio_dev); > > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > > > > > error_disable_reg: > > > > > > This entire goto block should be removed. > > > The idea of using only devm_ functions is to not have these goto statements. > > > > > I thought the action added via devm_add_action (and devres_add) was called only > > on driver detach, thus the error_disable_reg label would be necessary > > to handle the > > devm_add_action() yes > this is devm_add_action_or_reset() which looks like this: > > static inline int devm_add_action_or_reset(struct device *dev, > void (*action)(void *), void *data) > { > int ret; > > ret = devm_add_action(dev, action, data); > if (ret) > action(data); > > return ret; > } > > it can be found in "include/linux/device.h" > > > possible error on regulator_get_voltage. Could you please clarify for > > me when does > > a driver detach happen? > > a driver detach happens when: > * the kmod is unloaded (assuming the driver is running as a kmod) > * manually unbinding the driver from sysfs ; > a quick article about this [it's for USB, but other interfaces use > the same mechanism]: > https://lwn.net/Articles/143397/ > there should be something under /sys/bus/spi/drivers/xxxx/unbind > * when the system powers down (reboots) > * maybe there is some other new method to do this [that I don't know about] > Thank you so much. So, should this goto error_disable_reg be replaced by "regulator_disable(.); return ret;"? ret = regulator_get_voltage(data->vref_reg); if (ret < 0) { dev_err(&client->dev, "Failed to get voltage on regulator: %d\n", ret); goto error_disable_reg; } > > > > Thanks for your reply, > > Théo > > > > > > regulator_disable(data->vref_reg); > > > > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > > > > return ret; > > > > } > > > > > > > > -static int max5821_remove(struct i2c_client *client) > > > > -{ > > > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > > > - struct max5821_data *data = iio_priv(indio_dev); > > > > - > > > > - iio_device_unregister(indio_dev); > > > > - regulator_disable(data->vref_reg); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > static const struct i2c_device_id max5821_id[] = { > > > > { "max5821", ID_MAX5821 }, > > > > { } > > > > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > > > > .pm = &max5821_pm_ops, > > > > }, > > > > .probe = max5821_probe, > > > > - .remove = max5821_remove, > > > > .id_table = max5821_id, > > > > }; > > > > module_i2c_driver(max5821_driver); > > > > -- > > > > 2.20.1 > > > >
On Wed, Jul 21, 2021 at 3:11 AM Théo Borém Fabris <theobf@usp.br> wrote: > > Em ter., 20 de jul. de 2021 às 04:22, Alexandru Ardelean > <ardeleanalex@gmail.com> escreveu: > > > > On Mon, Jul 19, 2021 at 6:25 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > Hi, Alexandru. > > > > > > Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean > > > <ardeleanalex@gmail.com> escreveu: > > > > > > > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > > > > > Add a device managed hook, via devm_add_action_or_reset() and > > > > > max5821_regulator_disable(), to disable voltage regulator on device > > > > > detach. > > > > > Replace iio_device_register() by devm_iio_device_register() and remove > > > > > the max5821_remove() function used to unregister the device and disable the > > > > > voltage regulator. > > > > > Remove i2c_set_clientdata() from the probe function, since > > > > > i2c_get_clientdata() is not used anymore. > > > > > > > > Looks good overall. > > > > A few comments inline. > > > > > > > > > > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > > > > --- > > > > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > > > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > > > > > index bd6e75699a63..44c04ae70b32 100644 > > > > > --- a/drivers/iio/dac/max5821.c > > > > > +++ b/drivers/iio/dac/max5821.c > > > > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > > > > > .write_raw = max5821_write_raw, > > > > > }; > > > > > > > > > > +static void max5821_regulator_disable(void *data) > > > > > +{ > > > > > + struct regulator *rdata = data; > > > > > + > > > > > + regulator_disable(rdata); > > > > > > > > This can be simplified a bit: > > > > > > > > static void max5821_regulator_disable(void *reg) > > > > { > > > > regulator_disable(reg); > > > > } > > > > > > > > I used to do explicit casting, but then I also figured that it's not necessary. > > > > > > > Ok. > > > > > > > > +} > > > > > + > > > > > static int max5821_probe(struct i2c_client *client, > > > > > const struct i2c_device_id *id) > > > > > { > > > > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > > > > > if (!indio_dev) > > > > > return -ENOMEM; > > > > > data = iio_priv(indio_dev); > > > > > - i2c_set_clientdata(client, indio_dev); > > > > > data->client = client; > > > > > mutex_init(&data->lock); > > > > > > > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > > > > > goto error_free_reg; > > > > > } > > > > > > > > > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > > > > > + data->vref_reg); > > > > > + if (ret) { > > > > > + dev_err(&client->dev, > > > > > + "Failed to add action to managed regulator: %d\n", ret); > > > > > + goto error_disable_reg; > > > > > > > > return ret; > > > > > > > > devm_add_action_or_reset() should call max5821_regulator_disable() in > > > > case of error > > > > > > > Ok. > > > > > > > > + } > > > > > + > > > > > ret = regulator_get_voltage(data->vref_reg); > > > > > if (ret < 0) { > > > > > dev_err(&client->dev, > > > > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > > indio_dev->info = &max5821_info; > > > > > > > > > > - return iio_device_register(indio_dev); > > > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > > > > > > > error_disable_reg: > > > > > > > > This entire goto block should be removed. > > > > The idea of using only devm_ functions is to not have these goto statements. > > > > > > > I thought the action added via devm_add_action (and devres_add) was called only > > > on driver detach, thus the error_disable_reg label would be necessary > > > to handle the > > > > devm_add_action() yes > > this is devm_add_action_or_reset() which looks like this: > > > > static inline int devm_add_action_or_reset(struct device *dev, > > void (*action)(void *), void *data) > > { > > int ret; > > > > ret = devm_add_action(dev, action, data); > > if (ret) > > action(data); > > > > return ret; > > } > > > > it can be found in "include/linux/device.h" > > > > > possible error on regulator_get_voltage. Could you please clarify for > > > me when does > > > a driver detach happen? > > > > a driver detach happens when: > > * the kmod is unloaded (assuming the driver is running as a kmod) > > * manually unbinding the driver from sysfs ; > > a quick article about this [it's for USB, but other interfaces use > > the same mechanism]: > > https://lwn.net/Articles/143397/ > > there should be something under /sys/bus/spi/drivers/xxxx/unbind > > * when the system powers down (reboots) > > * maybe there is some other new method to do this [that I don't know about] > > > Thank you so much. > > So, should this goto error_disable_reg be replaced by > "regulator_disable(.); return ret;"? > ret = regulator_get_voltage(data->vref_reg); > if (ret < 0) { > dev_err(&client->dev, > "Failed to get voltage on regulator: %d\n", ret); > goto error_disable_reg; > } This is sufficient: [ No extra regulator_disable() is required; regulator_disable() gets called via max5821_regulator_disable() on the error path ] ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, data->vref_reg); if (ret) { dev_err(&client->dev, "Failed to add action to managed regulator: %d\n", ret); return ret; } > > > > > > > Thanks for your reply, > > > Théo > > > > > > > > regulator_disable(data->vref_reg); > > > > > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > > > > > return ret; > > > > > } > > > > > > > > > > -static int max5821_remove(struct i2c_client *client) > > > > > -{ > > > > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > > > > - struct max5821_data *data = iio_priv(indio_dev); > > > > > - > > > > > - iio_device_unregister(indio_dev); > > > > > - regulator_disable(data->vref_reg); > > > > > - > > > > > - return 0; > > > > > -} > > > > > - > > > > > static const struct i2c_device_id max5821_id[] = { > > > > > { "max5821", ID_MAX5821 }, > > > > > { } > > > > > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > > > > > .pm = &max5821_pm_ops, > > > > > }, > > > > > .probe = max5821_probe, > > > > > - .remove = max5821_remove, > > > > > .id_table = max5821_id, > > > > > }; > > > > > module_i2c_driver(max5821_driver); > > > > > -- > > > > > 2.20.1 > > > > >
Thank you, Alexandru and Numo, for your help. I have submitted the second version. Théo Em qua., 21 de jul. de 2021 às 04:23, Alexandru Ardelean <ardeleanalex@gmail.com> escreveu: > > On Wed, Jul 21, 2021 at 3:11 AM Théo Borém Fabris <theobf@usp.br> wrote: > > > > Em ter., 20 de jul. de 2021 às 04:22, Alexandru Ardelean > > <ardeleanalex@gmail.com> escreveu: > > > > > > On Mon, Jul 19, 2021 at 6:25 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > > > Hi, Alexandru. > > > > > > > > Em seg., 19 de jul. de 2021 às 04:33, Alexandru Ardelean > > > > <ardeleanalex@gmail.com> escreveu: > > > > > > > > > > On Sun, Jul 18, 2021 at 11:42 PM Théo Borém Fabris <theobf@usp.br> wrote: > > > > > > > > > > > > Add a device managed hook, via devm_add_action_or_reset() and > > > > > > max5821_regulator_disable(), to disable voltage regulator on device > > > > > > detach. > > > > > > Replace iio_device_register() by devm_iio_device_register() and remove > > > > > > the max5821_remove() function used to unregister the device and disable the > > > > > > voltage regulator. > > > > > > Remove i2c_set_clientdata() from the probe function, since > > > > > > i2c_get_clientdata() is not used anymore. > > > > > > > > > > Looks good overall. > > > > > A few comments inline. > > > > > > > > > > > > > > > > > Signed-off-by: Théo Borém Fabris <theobf@usp.br> > > > > > > --- > > > > > > drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- > > > > > > 1 file changed, 16 insertions(+), 14 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c > > > > > > index bd6e75699a63..44c04ae70b32 100644 > > > > > > --- a/drivers/iio/dac/max5821.c > > > > > > +++ b/drivers/iio/dac/max5821.c > > > > > > @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { > > > > > > .write_raw = max5821_write_raw, > > > > > > }; > > > > > > > > > > > > +static void max5821_regulator_disable(void *data) > > > > > > +{ > > > > > > + struct regulator *rdata = data; > > > > > > + > > > > > > + regulator_disable(rdata); > > > > > > > > > > This can be simplified a bit: > > > > > > > > > > static void max5821_regulator_disable(void *reg) > > > > > { > > > > > regulator_disable(reg); > > > > > } > > > > > > > > > > I used to do explicit casting, but then I also figured that it's not necessary. > > > > > > > > > Ok. > > > > > > > > > > +} > > > > > > + > > > > > > static int max5821_probe(struct i2c_client *client, > > > > > > const struct i2c_device_id *id) > > > > > > { > > > > > > @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, > > > > > > if (!indio_dev) > > > > > > return -ENOMEM; > > > > > > data = iio_priv(indio_dev); > > > > > > - i2c_set_clientdata(client, indio_dev); > > > > > > data->client = client; > > > > > > mutex_init(&data->lock); > > > > > > > > > > > > @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, > > > > > > goto error_free_reg; > > > > > > } > > > > > > > > > > > > + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > > > > > > + data->vref_reg); > > > > > > + if (ret) { > > > > > > + dev_err(&client->dev, > > > > > > + "Failed to add action to managed regulator: %d\n", ret); > > > > > > + goto error_disable_reg; > > > > > > > > > > return ret; > > > > > > > > > > devm_add_action_or_reset() should call max5821_regulator_disable() in > > > > > case of error > > > > > > > > > Ok. > > > > > > > > > > + } > > > > > > + > > > > > > ret = regulator_get_voltage(data->vref_reg); > > > > > > if (ret < 0) { > > > > > > dev_err(&client->dev, > > > > > > @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, > > > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > > > indio_dev->info = &max5821_info; > > > > > > > > > > > > - return iio_device_register(indio_dev); > > > > > > + return devm_iio_device_register(&client->dev, indio_dev); > > > > > > > > > > > > error_disable_reg: > > > > > > > > > > This entire goto block should be removed. > > > > > The idea of using only devm_ functions is to not have these goto statements. > > > > > > > > > I thought the action added via devm_add_action (and devres_add) was called only > > > > on driver detach, thus the error_disable_reg label would be necessary > > > > to handle the > > > > > > devm_add_action() yes > > > this is devm_add_action_or_reset() which looks like this: > > > > > > static inline int devm_add_action_or_reset(struct device *dev, > > > void (*action)(void *), void *data) > > > { > > > int ret; > > > > > > ret = devm_add_action(dev, action, data); > > > if (ret) > > > action(data); > > > > > > return ret; > > > } > > > > > > it can be found in "include/linux/device.h" > > > > > > > possible error on regulator_get_voltage. Could you please clarify for > > > > me when does > > > > a driver detach happen? > > > > > > a driver detach happens when: > > > * the kmod is unloaded (assuming the driver is running as a kmod) > > > * manually unbinding the driver from sysfs ; > > > a quick article about this [it's for USB, but other interfaces use > > > the same mechanism]: > > > https://lwn.net/Articles/143397/ > > > there should be something under /sys/bus/spi/drivers/xxxx/unbind > > > * when the system powers down (reboots) > > > * maybe there is some other new method to do this [that I don't know about] > > > > > Thank you so much. > > > > So, should this goto error_disable_reg be replaced by > > "regulator_disable(.); return ret;"? > > ret = regulator_get_voltage(data->vref_reg); > > if (ret < 0) { > > dev_err(&client->dev, > > "Failed to get voltage on regulator: %d\n", ret); > > goto error_disable_reg; > > } > > > This is sufficient: > [ No extra regulator_disable() is required; regulator_disable() gets > called via max5821_regulator_disable() on the error path ] > > ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, > data->vref_reg); > if (ret) { > dev_err(&client->dev, > "Failed to add action to managed regulator: %d\n", ret); > return ret; > } > > > > > > > > > > > Thanks for your reply, > > > > Théo > > > > > > > > > > regulator_disable(data->vref_reg); > > > > > > @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, > > > > > > return ret; > > > > > > } > > > > > > > > > > > > -static int max5821_remove(struct i2c_client *client) > > > > > > -{ > > > > > > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > > > > > > - struct max5821_data *data = iio_priv(indio_dev); > > > > > > - > > > > > > - iio_device_unregister(indio_dev); > > > > > > - regulator_disable(data->vref_reg); > > > > > > - > > > > > > - return 0; > > > > > > -} > > > > > > - > > > > > > static const struct i2c_device_id max5821_id[] = { > > > > > > { "max5821", ID_MAX5821 }, > > > > > > { } > > > > > > @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { > > > > > > .pm = &max5821_pm_ops, > > > > > > }, > > > > > > .probe = max5821_probe, > > > > > > - .remove = max5821_remove, > > > > > > .id_table = max5821_id, > > > > > > }; > > > > > > module_i2c_driver(max5821_driver); > > > > > > -- > > > > > > 2.20.1 > > > > > >
diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c index bd6e75699a63..44c04ae70b32 100644 --- a/drivers/iio/dac/max5821.c +++ b/drivers/iio/dac/max5821.c @@ -294,6 +294,13 @@ static const struct iio_info max5821_info = { .write_raw = max5821_write_raw, }; +static void max5821_regulator_disable(void *data) +{ + struct regulator *rdata = data; + + regulator_disable(rdata); +} + static int max5821_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -306,7 +313,6 @@ static int max5821_probe(struct i2c_client *client, if (!indio_dev) return -ENOMEM; data = iio_priv(indio_dev); - i2c_set_clientdata(client, indio_dev); data->client = client; mutex_init(&data->lock); @@ -331,6 +337,14 @@ static int max5821_probe(struct i2c_client *client, goto error_free_reg; } + ret = devm_add_action_or_reset(&client->dev, max5821_regulator_disable, + data->vref_reg); + if (ret) { + dev_err(&client->dev, + "Failed to add action to managed regulator: %d\n", ret); + goto error_disable_reg; + } + ret = regulator_get_voltage(data->vref_reg); if (ret < 0) { dev_err(&client->dev, @@ -346,7 +360,7 @@ static int max5821_probe(struct i2c_client *client, indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &max5821_info; - return iio_device_register(indio_dev); + return devm_iio_device_register(&client->dev, indio_dev); error_disable_reg: regulator_disable(data->vref_reg); @@ -356,17 +370,6 @@ static int max5821_probe(struct i2c_client *client, return ret; } -static int max5821_remove(struct i2c_client *client) -{ - struct iio_dev *indio_dev = i2c_get_clientdata(client); - struct max5821_data *data = iio_priv(indio_dev); - - iio_device_unregister(indio_dev); - regulator_disable(data->vref_reg); - - return 0; -} - static const struct i2c_device_id max5821_id[] = { { "max5821", ID_MAX5821 }, { } @@ -386,7 +389,6 @@ static struct i2c_driver max5821_driver = { .pm = &max5821_pm_ops, }, .probe = max5821_probe, - .remove = max5821_remove, .id_table = max5821_id, }; module_i2c_driver(max5821_driver);
Add a device managed hook, via devm_add_action_or_reset() and max5821_regulator_disable(), to disable voltage regulator on device detach. Replace iio_device_register() by devm_iio_device_register() and remove the max5821_remove() function used to unregister the device and disable the voltage regulator. Remove i2c_set_clientdata() from the probe function, since i2c_get_clientdata() is not used anymore. Signed-off-by: Théo Borém Fabris <theobf@usp.br> --- drivers/iio/dac/max5821.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)