Use device-managed APIs to simplify the code.
The remove functions are redundant now and can
be deleted.
Signed-off-by: Chuhong Yuan <[email protected]>
---
drivers/iio/light/cm3323.c | 31 ++++++++++-----------------
drivers/iio/light/si1145.c | 44 ++++++++++++++------------------------
2 files changed, 27 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
index 50f3438c2b49..fd352c0a4507 100644
--- a/drivers/iio/light/cm3323.c
+++ b/drivers/iio/light/cm3323.c
@@ -101,15 +101,16 @@ static int cm3323_init(struct iio_dev *indio_dev)
return 0;
}
-static void cm3323_disable(struct iio_dev *indio_dev)
+static void cm3323_disable(void *data)
{
int ret;
- struct cm3323_data *data = iio_priv(indio_dev);
+ struct iio_dev *indio_dev = data;
+ struct cm3323_data *cm_data = iio_priv(indio_dev);
- ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
+ ret = i2c_smbus_write_word_data(cm_data->client, CM3323_CMD_CONF,
CM3323_CONF_SD_BIT);
if (ret < 0)
- dev_err(&data->client->dev, "Error writing reg_conf\n");
+ dev_err(&cm_data->client->dev, "Error writing reg_conf\n");
}
static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
@@ -243,25 +244,16 @@ static int cm3323_probe(struct i2c_client *client,
return ret;
}
- ret = iio_device_register(indio_dev);
+ ret = devm_add_action_or_reset(&client->dev, cm3323_disable, indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_iio_device_register(&client->dev, indio_dev);
if (ret < 0) {
dev_err(&client->dev, "failed to register iio dev\n");
- goto err_init;
+ return ret;
}
- return 0;
-err_init:
- cm3323_disable(indio_dev);
- return ret;
-}
-
-static int cm3323_remove(struct i2c_client *client)
-{
- struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
- iio_device_unregister(indio_dev);
- cm3323_disable(indio_dev);
-
return 0;
}
@@ -276,7 +268,6 @@ static struct i2c_driver cm3323_driver = {
.name = CM3323_DRV_NAME,
},
.probe = cm3323_probe,
- .remove = cm3323_remove,
.id_table = cm3323_id,
};
diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
index 6579d2418814..61867552b27c 100644
--- a/drivers/iio/light/si1145.c
+++ b/drivers/iio/light/si1145.c
@@ -1271,13 +1271,14 @@ static int si1145_probe_trigger(struct iio_dev *indio_dev)
return 0;
}
-static void si1145_remove_trigger(struct iio_dev *indio_dev)
+static void si1145_remove_trigger(void *data)
{
- struct si1145_data *data = iio_priv(indio_dev);
+ struct iio_dev *indio_dev = data;
+ struct si1145_data *si_data = iio_priv(indio_dev);
- if (data->trig) {
- iio_trigger_unregister(data->trig);
- data->trig = NULL;
+ if (si_data->trig) {
+ iio_trigger_unregister(si_data->trig);
+ si_data->trig = NULL;
}
}
@@ -1332,7 +1333,8 @@ static int si1145_probe(struct i2c_client *client,
if (ret < 0)
return ret;
- ret = iio_triggered_buffer_setup(indio_dev, NULL,
+ ret = devm_iio_triggered_buffer_setup(&client->dev,
+ indio_dev, NULL,
si1145_trigger_handler, &si1145_buffer_setup_ops);
if (ret < 0)
return ret;
@@ -1340,23 +1342,21 @@ static int si1145_probe(struct i2c_client *client,
if (client->irq) {
ret = si1145_probe_trigger(indio_dev);
if (ret < 0)
- goto error_free_buffer;
+ return ret;
+
+ ret = devm_add_action_or_reset(&client->dev,
+ si1145_remove_trigger, indio_dev);
+ if (ret < 0)
+ return ret;
+
} else {
dev_info(&client->dev, "no irq, using polling\n");
}
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(&client->dev, indio_dev);
if (ret < 0)
- goto error_free_trigger;
+ return ret;
return 0;
-
-error_free_trigger:
- si1145_remove_trigger(indio_dev);
-error_free_buffer:
- iio_triggered_buffer_cleanup(indio_dev);
-
- return ret;
}
static const struct i2c_device_id si1145_ids[] = {
@@ -1371,23 +1371,11 @@ static const struct i2c_device_id si1145_ids[] = {
};
MODULE_DEVICE_TABLE(i2c, si1145_ids);
-static int si1145_remove(struct i2c_client *client)
-{
- struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
- iio_device_unregister(indio_dev);
- si1145_remove_trigger(indio_dev);
- iio_triggered_buffer_cleanup(indio_dev);
-
- return 0;
-}
-
static struct i2c_driver si1145_driver = {
.driver = {
.name = "si1145",
},
.probe = si1145_probe,
- .remove = si1145_remove,
.id_table = si1145_ids,
};
--
2.20.1
On Fri, 26 Jul 2019 17:08:29 +0800
Chuhong Yuan <[email protected]> wrote:
> Use device-managed APIs to simplify the code.
> The remove functions are redundant now and can
> be deleted.
>
> Signed-off-by: Chuhong Yuan <[email protected]>
Hi,
This should have been split along the lines of the drivers as these
have different authors and hence could be merged at different times.
Not to mention the nature of the changes isn't exactly the same
between the two.
I might have taken the cm3323 one with a small tweak as below if they
had been separate.
Thanks,
Jonathan
> ---
> drivers/iio/light/cm3323.c | 31 ++++++++++-----------------
> drivers/iio/light/si1145.c | 44 ++++++++++++++------------------------
> 2 files changed, 27 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> index 50f3438c2b49..fd352c0a4507 100644
> --- a/drivers/iio/light/cm3323.c
> +++ b/drivers/iio/light/cm3323.c
> @@ -101,15 +101,16 @@ static int cm3323_init(struct iio_dev *indio_dev)
> return 0;
> }
>
> -static void cm3323_disable(struct iio_dev *indio_dev)
> +static void cm3323_disable(void *data)
> {
> int ret;
> - struct cm3323_data *data = iio_priv(indio_dev);
> + struct iio_dev *indio_dev = data;
> + struct cm3323_data *cm_data = iio_priv(indio_dev);
>
> - ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
> + ret = i2c_smbus_write_word_data(cm_data->client, CM3323_CMD_CONF,
> CM3323_CONF_SD_BIT);
> if (ret < 0)
> - dev_err(&data->client->dev, "Error writing reg_conf\n");
> + dev_err(&cm_data->client->dev, "Error writing reg_conf\n");
> }
>
> static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
> @@ -243,25 +244,16 @@ static int cm3323_probe(struct i2c_client *client,
> return ret;
> }
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_add_action_or_reset(&client->dev, cm3323_disable, indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> if (ret < 0) {
> dev_err(&client->dev, "failed to register iio dev\n");
> - goto err_init;
> + return ret;
I'd drop the failed to register message and just
return devm_iio_device_register(...);
Most of the likely paths that can cause that to fail report errors anyway
in the core code.
> }
>
> - return 0;
> -err_init:
> - cm3323_disable(indio_dev);
> - return ret;
> -}
> -
> -static int cm3323_remove(struct i2c_client *client)
> -{
> - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> - iio_device_unregister(indio_dev);
> - cm3323_disable(indio_dev);
> -
> return 0;
> }
>
> @@ -276,7 +268,6 @@ static struct i2c_driver cm3323_driver = {
> .name = CM3323_DRV_NAME,
> },
> .probe = cm3323_probe,
> - .remove = cm3323_remove,
> .id_table = cm3323_id,
> };
>
> diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
> index 6579d2418814..61867552b27c 100644
> --- a/drivers/iio/light/si1145.c
> +++ b/drivers/iio/light/si1145.c
> @@ -1271,13 +1271,14 @@ static int si1145_probe_trigger(struct iio_dev *indio_dev)
> return 0;
> }
>
> -static void si1145_remove_trigger(struct iio_dev *indio_dev)
> +static void si1145_remove_trigger(void *data)
> {
> - struct si1145_data *data = iio_priv(indio_dev);
> + struct iio_dev *indio_dev = data;
> + struct si1145_data *si_data = iio_priv(indio_dev);
>
> - if (data->trig) {
> - iio_trigger_unregister(data->trig);
> - data->trig = NULL;
> + if (si_data->trig) {
> + iio_trigger_unregister(si_data->trig);
This doesn't look right. Why not use the devm trigger
registration directly?
> + si_data->trig = NULL;
I find it unlikely that we actually need to set this to
NULL, but please check closely in the driver.
The obvious path would be that the interrupt fired
after the trigger was unregistered (shouldn't be possible
unless something odd is going on), however there is no
obviously signs of this being checked there.
> }
> }
>
> @@ -1332,7 +1333,8 @@ static int si1145_probe(struct i2c_client *client,
> if (ret < 0)
> return ret;
>
> - ret = iio_triggered_buffer_setup(indio_dev, NULL,
> + ret = devm_iio_triggered_buffer_setup(&client->dev,
> + indio_dev, NULL,
> si1145_trigger_handler, &si1145_buffer_setup_ops);
> if (ret < 0)
> return ret;
> @@ -1340,23 +1342,21 @@ static int si1145_probe(struct i2c_client *client,
> if (client->irq) {
> ret = si1145_probe_trigger(indio_dev);
> if (ret < 0)
> - goto error_free_buffer;
> + return ret;
> +
> + ret = devm_add_action_or_reset(&client->dev,
> + si1145_remove_trigger, indio_dev);
> + if (ret < 0)
> + return ret;
> +
> } else {
> dev_info(&client->dev, "no irq, using polling\n");
> }
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> if (ret < 0)
> - goto error_free_trigger;
> + return ret;
>
> return 0;
> -
> -error_free_trigger:
> - si1145_remove_trigger(indio_dev);
> -error_free_buffer:
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> - return ret;
> }
>
> static const struct i2c_device_id si1145_ids[] = {
> @@ -1371,23 +1371,11 @@ static const struct i2c_device_id si1145_ids[] = {
> };
> MODULE_DEVICE_TABLE(i2c, si1145_ids);
>
> -static int si1145_remove(struct i2c_client *client)
> -{
> - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> - iio_device_unregister(indio_dev);
> - si1145_remove_trigger(indio_dev);
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> - return 0;
> -}
> -
> static struct i2c_driver si1145_driver = {
> .driver = {
> .name = "si1145",
> },
> .probe = si1145_probe,
> - .remove = si1145_remove,
> .id_table = si1145_ids,
> };
>
Jonathan Cameron <[email protected]> 于2019年7月28日周日 上午1:08写道：
>
> On Fri, 26 Jul 2019 17:08:29 +0800
> Chuhong Yuan <[email protected]> wrote:
>
> > Use device-managed APIs to simplify the code.
> > The remove functions are redundant now and can
> > be deleted.
> >
> > Signed-off-by: Chuhong Yuan <[email protected]>
> Hi,
>
> This should have been split along the lines of the drivers as these
> have different authors and hence could be merged at different times.
> Not to mention the nature of the changes isn't exactly the same
> between the two.
>
> I might have taken the cm3323 one with a small tweak as below if they
> had been separate.
>
> Thanks,
>
> Jonathan
I will separate them and resend later.
>
> > ---
> > drivers/iio/light/cm3323.c | 31 ++++++++++-----------------
> > drivers/iio/light/si1145.c | 44 ++++++++++++++------------------------
> > 2 files changed, 27 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/iio/light/cm3323.c b/drivers/iio/light/cm3323.c
> > index 50f3438c2b49..fd352c0a4507 100644
> > --- a/drivers/iio/light/cm3323.c
> > +++ b/drivers/iio/light/cm3323.c
> > @@ -101,15 +101,16 @@ static int cm3323_init(struct iio_dev *indio_dev)
> > return 0;
> > }
> >
> > -static void cm3323_disable(struct iio_dev *indio_dev)
> > +static void cm3323_disable(void *data)
> > {
> > int ret;
> > - struct cm3323_data *data = iio_priv(indio_dev);
> > + struct iio_dev *indio_dev = data;
> > + struct cm3323_data *cm_data = iio_priv(indio_dev);
> >
> > - ret = i2c_smbus_write_word_data(data->client, CM3323_CMD_CONF,
> > + ret = i2c_smbus_write_word_data(cm_data->client, CM3323_CMD_CONF,
> > CM3323_CONF_SD_BIT);
> > if (ret < 0)
> > - dev_err(&data->client->dev, "Error writing reg_conf\n");
> > + dev_err(&cm_data->client->dev, "Error writing reg_conf\n");
> > }
> >
> > static int cm3323_set_it_bits(struct cm3323_data *data, int val, int val2)
> > @@ -243,25 +244,16 @@ static int cm3323_probe(struct i2c_client *client,
> > return ret;
> > }
> >
> > - ret = iio_device_register(indio_dev);
> > + ret = devm_add_action_or_reset(&client->dev, cm3323_disable, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_iio_device_register(&client->dev, indio_dev);
> > if (ret < 0) {
> > dev_err(&client->dev, "failed to register iio dev\n");
> > - goto err_init;
> > + return ret;
>
> I'd drop the failed to register message and just
> return devm_iio_device_register(...);
>
> Most of the likely paths that can cause that to fail report errors anyway
> in the core code.
>
> > }
> >
> > - return 0;
> > -err_init:
> > - cm3323_disable(indio_dev);
> > - return ret;
> > -}
> > -
> > -static int cm3323_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > - iio_device_unregister(indio_dev);
> > - cm3323_disable(indio_dev);
> > -
> > return 0;
> > }
> >
> > @@ -276,7 +268,6 @@ static struct i2c_driver cm3323_driver = {
> > .name = CM3323_DRV_NAME,
> > },
> > .probe = cm3323_probe,
> > - .remove = cm3323_remove,
> > .id_table = cm3323_id,
> > };
> >
> > diff --git a/drivers/iio/light/si1145.c b/drivers/iio/light/si1145.c
> > index 6579d2418814..61867552b27c 100644
> > --- a/drivers/iio/light/si1145.c
> > +++ b/drivers/iio/light/si1145.c
> > @@ -1271,13 +1271,14 @@ static int si1145_probe_trigger(struct iio_dev *indio_dev)
> > return 0;
> > }
> >
> > -static void si1145_remove_trigger(struct iio_dev *indio_dev)
> > +static void si1145_remove_trigger(void *data)
> > {
> > - struct si1145_data *data = iio_priv(indio_dev);
> > + struct iio_dev *indio_dev = data;
> > + struct si1145_data *si_data = iio_priv(indio_dev);
> >
> > - if (data->trig) {
> > - iio_trigger_unregister(data->trig);
> > - data->trig = NULL;
> > + if (si_data->trig) {
> > + iio_trigger_unregister(si_data->trig);
>
> This doesn't look right. Why not use the devm trigger
> registration directly?
>
> > + si_data->trig = NULL;
>
> I find it unlikely that we actually need to set this to
> NULL, but please check closely in the driver.
> The obvious path would be that the interrupt fired
> after the trigger was unregistered (shouldn't be possible
> unless something odd is going on), however there is no
> obviously signs of this being checked there.
>
Thanks for your advice!
I will check whether data->trig is not needed to be set to NULL
and can devm_iio_trigger_register be used.
If so, I will revise them in version 2.
Regards,
Chuhong
>
> > }
> > }
> >
> > @@ -1332,7 +1333,8 @@ static int si1145_probe(struct i2c_client *client,
> > if (ret < 0)
> > return ret;
> >
> > - ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > + ret = devm_iio_triggered_buffer_setup(&client->dev,
> > + indio_dev, NULL,
> > si1145_trigger_handler, &si1145_buffer_setup_ops);
> > if (ret < 0)
> > return ret;
> > @@ -1340,23 +1342,21 @@ static int si1145_probe(struct i2c_client *client,
> > if (client->irq) {
> > ret = si1145_probe_trigger(indio_dev);
> > if (ret < 0)
> > - goto error_free_buffer;
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&client->dev,
> > + si1145_remove_trigger, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > } else {
> > dev_info(&client->dev, "no irq, using polling\n");
> > }
> >
> > - ret = iio_device_register(indio_dev);
> > + ret = devm_iio_device_register(&client->dev, indio_dev);
> > if (ret < 0)
> > - goto error_free_trigger;
> > + return ret;
> >
> > return 0;
> > -
> > -error_free_trigger:
> > - si1145_remove_trigger(indio_dev);
> > -error_free_buffer:
> > - iio_triggered_buffer_cleanup(indio_dev);
> > -
> > - return ret;
> > }
> >
> > static const struct i2c_device_id si1145_ids[] = {
> > @@ -1371,23 +1371,11 @@ static const struct i2c_device_id si1145_ids[] = {
> > };
> > MODULE_DEVICE_TABLE(i2c, si1145_ids);
> >
> > -static int si1145_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > - iio_device_unregister(indio_dev);
> > - si1145_remove_trigger(indio_dev);
> > - iio_triggered_buffer_cleanup(indio_dev);
> > -
> > - return 0;
> > -}
> > -
> > static struct i2c_driver si1145_driver = {
> > .driver = {
> > .name = "si1145",
> > },
> > .probe = si1145_probe,
> > - .remove = si1145_remove,
> > .id_table = si1145_ids,
> > };
> >
>