| Message ID | 20241102195037.3013934-7-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
Hi Aren, > @@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client) > device_property_read_u32(&client->dev, "proximity-near-level", > &data->ps_near_level); > > - mutex_init(&data->lock); > + devm_mutex_init(&client->dev, &data->lock); ret = devm_mutex_init() if (ret) return ret; It is very unlikely to fail but technically it can. Andy has been fixing this up across the kernel (including IIO) so let's not introduce another case that doesn't check it! If nothing else comes up I can probably tidy that up whilst applying. Jonathan > > ret = stk3310_regmap_init(data); > if (ret < 0) > @@ -650,29 +666,17 @@ static int stk3310_probe(struct i2c_client *client) > if (ret < 0) { > dev_err(&client->dev, "request irq %d failed\n", > client->irq); > - goto err_standby; > + return ret; > } > } > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(&client->dev, indio_dev); > if (ret < 0) { > dev_err(&client->dev, "device_register failed\n"); > - goto err_standby; > + return ret; > } > > return 0; > - > -err_standby: > - stk3310_set_state(data, STK3310_STATE_STANDBY); > - return ret; > -} > - > -static void stk3310_remove(struct i2c_client *client) > -{ > - struct iio_dev *indio_dev = i2c_get_clientdata(client); > - > - iio_device_unregister(indio_dev); > - stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > } > > static int stk3310_suspend(struct device *dev) > @@ -736,7 +740,6 @@ static struct i2c_driver stk3310_driver = { > .acpi_match_table = stk3310_acpi_id, > }, > .probe = stk3310_probe, > - .remove = stk3310_remove, > .id_table = stk3310_i2c_id, > }; >
On Sun, Nov 03, 2024 at 11:22:08AM +0000, Jonathan Cameron wrote: > Hi Aren, > > > @@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client) > > device_property_read_u32(&client->dev, "proximity-near-level", > > &data->ps_near_level); > > > > - mutex_init(&data->lock); > > + devm_mutex_init(&client->dev, &data->lock); > ret = devm_mutex_init() > if (ret) > return ret; > > It is very unlikely to fail but technically it can. Andy has been fixing > this up across the kernel (including IIO) so let's not introduce another > case that doesn't check it! Right, I'll take this as my periodic reminder to read the docs / types more carefully :) > If nothing else comes up I can probably tidy that up whilst applying. That would be great Thanks - Aren
On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote: > Using devm callbacks helps to make the ordering of probe / remove > operations easier to reason about and removes some duplicate code > between the probe error path and driver remove. Where is SoB? ... > + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); Why not simply 'dev' as in below call? > + if (ret) > + return dev_err_probe(dev, ret, "failed to register cleanup function\n"); ... > - mutex_init(&data->lock); > + devm_mutex_init(&client->dev, &data->lock); Missed error check, otherwise what's the point? Also can add a temporary variable for 'dev'.
On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote: > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote: > > Using devm callbacks helps to make the ordering of probe / remove > > operations easier to reason about and removes some duplicate code > > between the probe error path and driver remove. > > Where is SoB? Oops that got lost in a rebase > ... > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); > > Why not simply 'dev' as in below call? I was trying to avoid refactoring the entire function to replace &client->dev with dev, I'll add a patch for that to the next revision. > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to register cleanup function\n"); > > ... > > > - mutex_init(&data->lock); > > + devm_mutex_init(&client->dev, &data->lock); > > Missed error check, otherwise what's the point? > > > Also can add a temporary variable for 'dev'. Yup, fixing... I need to read the docs / function type more carefully sometimes. Thanks - Aren
Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti: > On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote: > > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote: ... > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); > > > > Why not simply 'dev' as in below call? > > I was trying to avoid refactoring the entire function to replace > &client->dev with dev, I'll add a patch for that to the next revision. I'm not talking about refactoring, I'm talking only about the lines that you have touched / added. > > > + if (ret) > > > + return dev_err_probe(dev, ret, "failed to register cleanup function\n");
On Sun, Nov 10, 2024 at 09:51:04PM +0200, Andy Shevchenko wrote: > Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti: > > On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote: > > > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote: > > ... > > > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); > > > > > > Why not simply 'dev' as in below call? > > > > I was trying to avoid refactoring the entire function to replace > > &client->dev with dev, I'll add a patch for that to the next revision. > > I'm not talking about refactoring, I'm talking only about the lines that you > have touched / added. Ah right, this one makes sense, my comment should have been on the next patch in this series which is a little more complex. For that patch it seemed inconsistent to use dev only in new code and mix it with calls using &client->dev. - Aren
On Sun, Nov 10, 2024 at 05:37:56PM -0500, Aren wrote: > On Sun, Nov 10, 2024 at 09:51:04PM +0200, Andy Shevchenko wrote: > > Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti: > > > On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote: > > > > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote: ... > > > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); > > > > > > > > Why not simply 'dev' as in below call? > > > > > > I was trying to avoid refactoring the entire function to replace > > > &client->dev with dev, I'll add a patch for that to the next revision. > > > > I'm not talking about refactoring, I'm talking only about the lines that you > > have touched / added. > > Ah right, this one makes sense, my comment should have been on the next > patch in this series which is a little more complex. For that patch it > seemed inconsistent to use dev only in new code and mix it with calls > using &client->dev. It's fine, you can add a new cleanup patch later on.
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index ed20b6714546..181b7acb3f96 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -484,6 +484,17 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state) return ret; } +static void stk3310_set_state_disable(void *private) +{ + int ret; + struct stk3310_data *data = private; + struct device *dev = &data->client->dev; + + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); + if (ret) + dev_err(dev, "failed to set state to standby: %d\n", ret); +} + static int stk3310_init(struct iio_dev *indio_dev) { int ret; @@ -491,6 +502,7 @@ static int stk3310_init(struct iio_dev *indio_dev) u8 state; struct stk3310_data *data = iio_priv(indio_dev); struct i2c_client *client = data->client; + struct device *dev = &client->dev; ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid); if (ret < 0) @@ -507,6 +519,10 @@ static int stk3310_init(struct iio_dev *indio_dev) return ret; } + ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data); + if (ret) + return dev_err_probe(dev, ret, "failed to register cleanup function\n"); + /* Enable PS interrupts */ ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN); if (ret < 0) @@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client) device_property_read_u32(&client->dev, "proximity-near-level", &data->ps_near_level); - mutex_init(&data->lock); + devm_mutex_init(&client->dev, &data->lock); ret = stk3310_regmap_init(data); if (ret < 0) @@ -650,29 +666,17 @@ static int stk3310_probe(struct i2c_client *client) if (ret < 0) { dev_err(&client->dev, "request irq %d failed\n", client->irq); - goto err_standby; + return ret; } } - ret = iio_device_register(indio_dev); + ret = devm_iio_device_register(&client->dev, indio_dev); if (ret < 0) { dev_err(&client->dev, "device_register failed\n"); - goto err_standby; + return ret; } return 0; - -err_standby: - stk3310_set_state(data, STK3310_STATE_STANDBY); - return ret; -} - -static void stk3310_remove(struct i2c_client *client) -{ - struct iio_dev *indio_dev = i2c_get_clientdata(client); - - iio_device_unregister(indio_dev); - stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); } static int stk3310_suspend(struct device *dev) @@ -736,7 +740,6 @@ static struct i2c_driver stk3310_driver = { .acpi_match_table = stk3310_acpi_id, }, .probe = stk3310_probe, - .remove = stk3310_remove, .id_table = stk3310_i2c_id, };
Using devm callbacks helps to make the ordering of probe / remove operations easier to reason about and removes some duplicate code between the probe error path and driver remove. --- Notes: Changes in v4: - also replace mutex_init with devm_mutex_init Added in v3 drivers/iio/light/stk3310.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)