| Message ID | 20250208211325.992280-8-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote: > Using dev_err_probe instead of dev_err and return makes the errors Use dev_err_probe() dev_err() > easier to understand by including the error name, and saves a little > code. I believe this patch will make more sense before switching to local 'dev' variable. Then the previous one will have an additional justification as the "struct device *dev = ...;" lines in some cases will be added already by this patch. ... > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > - if (!indio_dev) { > - dev_err(&client->dev, "iio allocation failed!\n"); > - return -ENOMEM; > - } > + if (!indio_dev) > + return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n"); We don't issue the messages for -ENOMEM. If it's in the current code, add a new patch to drop this message and return an error code directly. ... > + if (ret < 0) Perhaps, while at it, drop these ' < 0' parts where they are not hinting about anything. > + return dev_err_probe(dev, ret, "device_register failed\n");
On Sun, 9 Feb 2025 16:47:44 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote: > > Using dev_err_probe instead of dev_err and return makes the errors > > Use dev_err_probe() > dev_err() > > > easier to understand by including the error name, and saves a little > > code. > > I believe this patch will make more sense before switching to local 'dev' > variable. Then the previous one will have an additional justification as > the "struct device *dev = ...;" lines in some cases will be added already > by this patch. I'm not sure I follow this one comment. The only line that has struct device *dev = added in this patch is replacing an existing client->dev lookup that could have been pushed to previous patch if this patch ordering was maintained. For dev_err() to dev_err_probe() the number of references to dev is the same after all. The only additional justification this patch makes is some longer lines that using a local dev pointer shortens again. > > ... > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > - if (!indio_dev) { > > - dev_err(&client->dev, "iio allocation failed!\n"); > > - return -ENOMEM; > > - } > > + if (!indio_dev) > > + return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n"); > > We don't issue the messages for -ENOMEM. > > If it's in the current code, add a new patch to drop this message and return an > error code directly. I'd be fine with that dev_err() dropped in this patch as long as the description mentions it. > > ... > > > + if (ret < 0) > > Perhaps, while at it, drop these ' < 0' parts where they are not hinting about > anything. That would be a separate patch and indeed makes sense to me as well. Jonathan > > > + return dev_err_probe(dev, ret, "device_register failed\n"); >
On Tue, Feb 11, 2025 at 07:43:11PM +0000, Jonathan Cameron wrote: > On Sun, 9 Feb 2025 16:47:44 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote: > > > Using dev_err_probe instead of dev_err and return makes the errors > > > > Use dev_err_probe() > > dev_err() > > > > > easier to understand by including the error name, and saves a little > > > code. > > > > I believe this patch will make more sense before switching to local 'dev' > > variable. Then the previous one will have an additional justification as > > the "struct device *dev = ...;" lines in some cases will be added already > > by this patch. > > I'm not sure I follow this one comment. > The only line that has struct device *dev = added in this patch is > replacing an existing client->dev lookup that could have been pushed > to previous patch if this patch ordering was maintained. > > For dev_err() to dev_err_probe() the number of references to dev > is the same after all. The only additional justification this patch > makes is some longer lines that using a local dev pointer shortens > again. When converting to dev_err_probe() in some cases it makes sense to add a temporary variable at the same time. if (ret) { dev_err(&pdev->dev, ...); return ...; } ===> struct device *dev = &pdev->dev; ... if (ret) return dev_err_probe(dev, ...); which reduces automatically the churn in the patch that wants to (re)use that temporary variable and also adds to the justification as "we already have that variable, just want to use it".
On Sun, Feb 09, 2025 at 04:47:44PM +0200, Andy Shevchenko wrote: > On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote: > > Using dev_err_probe instead of dev_err and return makes the errors > > Use dev_err_probe() > dev_err() > > > easier to understand by including the error name, and saves a little > > code. > > I believe this patch will make more sense before switching to local 'dev' > variable. Then the previous one will have an additional justification as > the "struct device *dev = ...;" lines in some cases will be added already > by this patch. That will only be added in one spot, and I skipped updating the dev_err calls in the previous patch that this patch rewrites, so churn shouldn't be an issue. That also makes it trivial to reorder them, so I guess it can't hurt. > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > - if (!indio_dev) { > > - dev_err(&client->dev, "iio allocation failed!\n"); > > - return -ENOMEM; > > - } > > + if (!indio_dev) > > + return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n"); > > We don't issue the messages for -ENOMEM. > > If it's in the current code, add a new patch to drop this message and return an > error code directly. > > ... > > > + if (ret < 0) > > Perhaps, while at it, drop these ' < 0' parts where they are not hinting about > anything. Sure, I can add patches for these, although continuing to rebase this series is getting a bit cumbersome (perhaps just because I haven't found a good workflow for it). Would I be better off reordering this so the refactoring patches come first and can be partially merged? Regards - Aren
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index 2233eab63b7aa..9d517d51f6bae 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -504,10 +504,8 @@ static int stk3310_init(struct iio_dev *indio_dev) state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS; ret = stk3310_set_state(data, state); - if (ret < 0) { - dev_err(&client->dev, "failed to enable sensor"); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "failed to enable sensor\n"); ret = devm_add_action_or_reset(dev, stk3310_set_state_disable, data); if (ret) @@ -516,9 +514,9 @@ static int stk3310_init(struct iio_dev *indio_dev) /* Enable PS interrupts */ ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN); if (ret < 0) - dev_err(&client->dev, "failed to enable interrupts!\n"); + return dev_err_probe(dev, ret, "failed to enable interrupts!\n"); - return ret; + return 0; } static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg) @@ -547,14 +545,14 @@ static const struct regmap_config stk3310_regmap_config = { static int stk3310_regmap_init(struct stk3310_data *data) { struct regmap *regmap; - struct i2c_client *client; + struct i2c_client *client = data->client; + struct device *dev = &client->dev; - client = data->client; regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config); - if (IS_ERR(regmap)) { - dev_err(&client->dev, "regmap initialization failed.\n"); - return PTR_ERR(regmap); - } + if (IS_ERR(regmap)) + return dev_err_probe(dev, PTR_ERR(regmap), + "regmap initialization failed\n"); + data->regmap = regmap; data->reg_state = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_state); @@ -676,10 +674,8 @@ static int stk3310_probe(struct i2c_client *client) struct device *dev = &client->dev; indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); - if (!indio_dev) { - dev_err(&client->dev, "iio allocation failed!\n"); - return -ENOMEM; - } + if (!indio_dev) + return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n"); data = iio_priv(indio_dev); data->client = client; @@ -728,18 +724,14 @@ static int stk3310_probe(struct i2c_client *client) IRQF_TRIGGER_FALLING | IRQF_ONESHOT, STK3310_EVENT, indio_dev); - if (ret < 0) { - dev_err(&client->dev, "request irq %d failed\n", - client->irq); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "request irq %d failed\n", + client->irq); } ret = devm_iio_device_register(dev, indio_dev); - if (ret < 0) { - dev_err(&client->dev, "device_register failed\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "device_register failed\n"); return 0; }
Using dev_err_probe instead of dev_err and return makes the errors easier to understand by including the error name, and saves a little code. Signed-off-by: Aren Moynihan <aren@peacevolution.org> --- Notes: Changes in v4: - Get a struct device ahead of time so it can be passed as "dev" instead of "&client->dev" No changes in v3 Added in v2 drivers/iio/light/stk3310.c | 42 +++++++++++++++---------------------- 1 file changed, 17 insertions(+), 25 deletions(-)