| Message ID | 20241102195037.3013934-11-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote: > 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. ... > #define STK3310_REGFIELD(name) \ > do { \ > data->reg_##name = \ > - devm_regmap_field_alloc(&client->dev, regmap, \ > + devm_regmap_field_alloc(dev, regmap, \ > stk3310_reg_field_##name); \ > - if (IS_ERR(data->reg_##name)) { \ > - dev_err(&client->dev, "reg field alloc failed.\n"); \ > - return PTR_ERR(data->reg_##name); \ > - } \ > + if (IS_ERR(data->reg_##name)) \ > + return dev_err_probe(dev, \ > + PTR_ERR(data->reg_##name), \ AFAICS these two can be put on one. > + "reg field alloc failed.\n"); \ > } while (0) ... > @@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client) > int ret; > struct iio_dev *indio_dev; > struct stk3310_data *data; > + struct device *dev = &client->dev; This should has been done a few patches earlier...
On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote: > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote: > > 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. > > ... > > > #define STK3310_REGFIELD(name) \ > > do { \ > > data->reg_##name = \ > > - devm_regmap_field_alloc(&client->dev, regmap, \ > > + devm_regmap_field_alloc(dev, regmap, \ > > stk3310_reg_field_##name); \ > > - if (IS_ERR(data->reg_##name)) { \ > > - dev_err(&client->dev, "reg field alloc failed.\n"); \ > > - return PTR_ERR(data->reg_##name); \ > > - } \ > > + if (IS_ERR(data->reg_##name)) \ > > > + return dev_err_probe(dev, \ > > + PTR_ERR(data->reg_##name), \ > > AFAICS these two can be put on one. This doesn't leave room for whitespace between the end of line and "\", replacing "do { } while (0)" with "({ })" and deindenting could make enough room to clean this up the formatting of this macro though. > > + "reg field alloc failed.\n"); \ > > } while (0) > > > ... > > > @@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client) > > int ret; > > struct iio_dev *indio_dev; > > struct stk3310_data *data; > > + struct device *dev = &client->dev; > > This should has been done a few patches earlier... Moving it there now - Aren
Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote: > > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote: ... > > > #define STK3310_REGFIELD(name) \ > > > do { \ > > > data->reg_##name = \ > > > - devm_regmap_field_alloc(&client->dev, regmap, \ > > > + devm_regmap_field_alloc(dev, regmap, \ > > > stk3310_reg_field_##name); \ > > > - if (IS_ERR(data->reg_##name)) { \ > > > - dev_err(&client->dev, "reg field alloc failed.\n"); \ > > > - return PTR_ERR(data->reg_##name); \ > > > - } \ > > > + if (IS_ERR(data->reg_##name)) \ > > > > > + return dev_err_probe(dev, \ > > > + PTR_ERR(data->reg_##name), \ > > > > AFAICS these two can be put on one. > > This doesn't leave room for whitespace between the end of line and "\", Is it a problem? > replacing "do { } while (0)" with "({ })" and deindenting could make > enough room to clean this up the formatting of this macro though. do {} while (0) is C standard, ({}) is not. > > > + "reg field alloc failed.\n"); \ > > > } while (0)
On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote: > > > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote: > > ... > > > > > #define STK3310_REGFIELD(name) \ > > > > do { \ > > > > data->reg_##name = \ > > > > - devm_regmap_field_alloc(&client->dev, regmap, \ > > > > + devm_regmap_field_alloc(dev, regmap, \ > > > > stk3310_reg_field_##name); \ > > > > - if (IS_ERR(data->reg_##name)) { \ > > > > - dev_err(&client->dev, "reg field alloc failed.\n"); \ > > > > - return PTR_ERR(data->reg_##name); \ > > > > - } \ > > > > + if (IS_ERR(data->reg_##name)) \ > > > > > > > + return dev_err_probe(dev, \ > > > > + PTR_ERR(data->reg_##name), \ > > > > > > AFAICS these two can be put on one. > > > > This doesn't leave room for whitespace between the end of line and "\", > > Is it a problem? It feels a bit camped and not as readable to me: #define STK3310_REGFIELD(name) \ do { \ data->reg_##name = \ devm_regmap_field_alloc(dev, regmap, \ stk3310_reg_field_##name); \ if (IS_ERR(data->reg_##name)) \ return dev_err_probe(dev, PTR_ERR(data->reg_##name),\ "reg field alloc failed.\n"); \ } while (0) Removing a level of indentation makes it much better #define STK3310_REGFIELD(name) ({ \ data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ stk3310_reg_field_##name); \ if (IS_ERR(data->reg_##name)) \ return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ "reg field alloc failed\n"); \ }) > > replacing "do { } while (0)" with "({ })" and deindenting could make > > enough room to clean this up the formatting of this macro though. > > do {} while (0) is C standard, ({}) is not. ({ }) is used throughout the kernel, and is documented as such[1]. I don't see a reason to avoid it, if it helps readability. 1: the "GNU Extensions" section of Documentation/kernel-hacking/hacking.rst - Aren
On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > > On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote: > > > > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote: ... > > > > > #define STK3310_REGFIELD(name) \ > > > > > do { \ > > > > > data->reg_##name = \ > > > > > - devm_regmap_field_alloc(&client->dev, regmap, \ > > > > > + devm_regmap_field_alloc(dev, regmap, \ > > > > > stk3310_reg_field_##name); \ > > > > > - if (IS_ERR(data->reg_##name)) { \ > > > > > - dev_err(&client->dev, "reg field alloc failed.\n"); \ > > > > > - return PTR_ERR(data->reg_##name); \ > > > > > - } \ > > > > > + if (IS_ERR(data->reg_##name)) \ > > > > > > > > > + return dev_err_probe(dev, \ > > > > > + PTR_ERR(data->reg_##name), \ > > > > > > > > AFAICS these two can be put on one. > > > > > > This doesn't leave room for whitespace between the end of line and "\", > > > > Is it a problem? > > It feels a bit camped and not as readable to me: > > #define STK3310_REGFIELD(name) \ > do { \ > data->reg_##name = \ > devm_regmap_field_alloc(dev, regmap, \ > stk3310_reg_field_##name); \ > if (IS_ERR(data->reg_##name)) \ > return dev_err_probe(dev, PTR_ERR(data->reg_##name),\ > "reg field alloc failed.\n"); \ > } while (0) Rather this way (besides the fact of having spaces instead of TABs for the last formatting, in such case you even can simply add yet another column with spaces): #define STK3310_REGFIELD(name) \ do { \ data->reg_##name = \ devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ if (IS_ERR(data->reg_##name)) \ return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ "reg field alloc failed.\n"); \ } while (0) > Removing a level of indentation makes it much better You can do it differently #define STK3310_REGFIELD(name) \ do { \ data->reg_##name = \ devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ if (IS_ERR(data->reg_##name)) \ return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ "reg field alloc failed.\n"); \ } while (0) > #define STK3310_REGFIELD(name) ({ \ > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ > stk3310_reg_field_##name); \ > if (IS_ERR(data->reg_##name)) \ > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > "reg field alloc failed\n"); \ > }) I am against unneeded use of GNU extensions. > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > enough room to clean this up the formatting of this macro though. > > > > do {} while (0) is C standard, ({}) is not. > > ({ }) is used throughout the kernel, and is documented as such[1]. I > don't see a reason to avoid it, if it helps readability. I don't see how it makes things better here, and not everybody is familiar with the concept even if it's used in the kernel here and there. Also if a tool is being used in one case it doesn't mean it's suitable for another. > 1: the "GNU Extensions" section of Documentation/kernel-hacking/hacking.rst
Hello Andy, hello Aren, On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote: > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > You can do it differently > > #define STK3310_REGFIELD(name) \ > do { \ > data->reg_##name = \ > devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ > if (IS_ERR(data->reg_##name)) \ > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > "reg field alloc failed.\n"); \ > } while (0) > > > #define STK3310_REGFIELD(name) ({ \ > > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ > > stk3310_reg_field_##name); \ > > if (IS_ERR(data->reg_##name)) \ > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > "reg field alloc failed\n"); \ > > }) > > I am against unneeded use of GNU extensions. > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > > enough room to clean this up the formatting of this macro though. > > > > > > do {} while (0) is C standard, ({}) is not. > > > > ({ }) is used throughout the kernel, and is documented as such[1]. I > > don't see a reason to avoid it, if it helps readability. > > I don't see how it makes things better here, and not everybody is familiar with > the concept even if it's used in the kernel here and there. Also if a tool is > being used in one case it doesn't mean it's suitable for another. Just to throw in my subjective view here: I don't expect anyone with some base level knowledge of C will have doubts about the semantics of ({ ... }) and compared to that I find do { ... } while (0) less optimal, because it's more verbose and when spotting the "do {" part, the semantic only gets clear when you also see the "while (0)". Having said that I also dislike the "do" starting on column 0, IMHO the RHS of the #define should be intended. So if you ask me, this is not an unneeded use of an extension. The extension is used to improve readabilty and I blame the C standard to not support this syntax. While I'm in critics mode: I consider hiding a return in a macro bad style. Best regards Uwe
On Tue, Nov 12, 2024 at 12:15 PM Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote: > > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > > > You can do it differently > > > > #define STK3310_REGFIELD(name) \ > > do { \ > > data->reg_##name = \ > > devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ > > if (IS_ERR(data->reg_##name)) \ > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > "reg field alloc failed.\n"); \ > > } while (0) > > > > > #define STK3310_REGFIELD(name) ({ \ > > > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ > > > stk3310_reg_field_##name); \ > > > if (IS_ERR(data->reg_##name)) \ > > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > > "reg field alloc failed\n"); \ > > > }) > > > > I am against unneeded use of GNU extensions. > > > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > > > enough room to clean this up the formatting of this macro though. > > > > > > > > do {} while (0) is C standard, ({}) is not. > > > > > > ({ }) is used throughout the kernel, and is documented as such[1]. I > > > don't see a reason to avoid it, if it helps readability. > > > > I don't see how it makes things better here, and not everybody is familiar with > > the concept even if it's used in the kernel here and there. Also if a tool is > > being used in one case it doesn't mean it's suitable for another. > > Just to throw in my subjective view here: I don't expect anyone with > some base level knowledge of C will have doubts about the semantics of > ({ ... }) and compared to that I find do { ... } while (0) less optimal, > because it's more verbose and when spotting the "do {" part, the > semantic only gets clear when you also see the "while (0)". Seems we have to agree on a disagreement. > Having said > that I also dislike the "do" starting on column 0, IMHO the RHS of the > #define should be intended. This argument I kinda accept. > So if you ask me, this is not an unneeded use of an extension. The > extension is used to improve readabilty and I blame the C standard to > not support this syntax. Here I agree with you. > While I'm in critics mode: I consider hiding a return in a macro bad > style. So, summarizing the discussion we have a split, hence Jonathan is our arbiter here to judge.
On Tue, 2024-11-12 at 11:15 +0100, Uwe Kleine-König wrote: > Hello Andy, hello Aren, > > On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote: > > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > > > You can do it differently > > > > #define > > STK3310_REGFIELD(name) \ > > do > > { \ > > data->reg_##name > > = \ > > devm_regmap_field_alloc(dev, regmap, > > stk3310_reg_field_##name); \ > > if (IS_ERR(data- > > >reg_##name)) \ > > return dev_err_probe(dev, PTR_ERR(data- > > >reg_##name), \ > > "reg field alloc > > failed.\n"); \ > > } while (0) > > > > > #define STK3310_REGFIELD(name) > > > ({ \ > > > data->reg_##name = devm_regmap_field_alloc(dev, > > > regmap, \ > > > > > > stk3310_reg_field_##name); \ > > > if (IS_ERR(data- > > > >reg_##name)) \ > > > return dev_err_probe(dev, PTR_ERR(data- > > > >reg_##name), \ > > > "reg field alloc > > > failed\n"); \ > > > }) > > > > I am against unneeded use of GNU extensions. > > > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > > > enough room to clean this up the formatting of this macro though. > > > > > > > > do {} while (0) is C standard, ({}) is not. > > > > > > ({ }) is used throughout the kernel, and is documented as such[1]. I > > > don't see a reason to avoid it, if it helps readability. > > > > I don't see how it makes things better here, and not everybody is familiar with > > the concept even if it's used in the kernel here and there. Also if a tool is > > being used in one case it doesn't mean it's suitable for another. > > Just to throw in my subjective view here: I don't expect anyone with > some base level knowledge of C will have doubts about the semantics of > ({ ... }) and compared to that I find do { ... } while (0) less optimal, > because it's more verbose and when spotting the "do {" part, the > semantic only gets clear when you also see the "while (0)". Having said > that I also dislike the "do" starting on column 0, IMHO the RHS of the > #define should be intended. > > So if you ask me, this is not an unneeded use of an extension. The > extension is used to improve readabilty and I blame the C standard to > not support this syntax. > > While I'm in critics mode: I consider hiding a return in a macro bad > style. > Not commenting on the debate between using the extension or not but I totally agree with Uwe about hiding the return in the macro. - Nuno Sá >
On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote: > Hello Andy, hello Aren, > > On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote: > > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > > > You can do it differently > > > > #define STK3310_REGFIELD(name) \ > > do { \ > > data->reg_##name = \ > > devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ > > if (IS_ERR(data->reg_##name)) \ > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > "reg field alloc failed.\n"); \ > > } while (0) > > > > > #define STK3310_REGFIELD(name) ({ \ > > > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ > > > stk3310_reg_field_##name); \ > > > if (IS_ERR(data->reg_##name)) \ > > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > > "reg field alloc failed\n"); \ > > > }) > > > > I am against unneeded use of GNU extensions. > > > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > > > enough room to clean this up the formatting of this macro though. > > > > > > > > do {} while (0) is C standard, ({}) is not. > > > > > > ({ }) is used throughout the kernel, and is documented as such[1]. I > > > don't see a reason to avoid it, if it helps readability. > > > > I don't see how it makes things better here, and not everybody is familiar with > > the concept even if it's used in the kernel here and there. Also if a tool is > > being used in one case it doesn't mean it's suitable for another. > > Just to throw in my subjective view here: I don't expect anyone with > some base level knowledge of C will have doubts about the semantics of > ({ ... }) and compared to that I find do { ... } while (0) less optimal, > because it's more verbose and when spotting the "do {" part, the > semantic only gets clear when you also see the "while (0)". Having said > that I also dislike the "do" starting on column 0, IMHO the RHS of the > #define should be intended. Thank you, this sums up my opinion on this better than I could have (and some bits I hadn't considered). > So if you ask me, this is not an unneeded use of an extension. The > extension is used to improve readabilty and I blame the C standard to > not support this syntax. > > While I'm in critics mode: I consider hiding a return in a macro bad > style. Yeah... probably worse than any of the formatting options here. I guess the proper way would be to use devm_regmap_field_bulk_alloc, but that's well outside the scope of this series. Perhaps it would make sense to move the macro definition to just before the function it's used in so it's at least a little easier to spot? - Aren
On Tue, 12 Nov 2024 18:11:37 -0500 Aren <aren@peacevolution.org> wrote: > On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote: > > Hello Andy, hello Aren, > > > > On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote: > > > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote: > > > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote: > > > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti: > > > > > > You can do it differently > > > > > > #define STK3310_REGFIELD(name) \ > > > do { \ > > > data->reg_##name = \ > > > devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \ > > > if (IS_ERR(data->reg_##name)) \ > > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > > "reg field alloc failed.\n"); \ > > > } while (0) > > > > > > > #define STK3310_REGFIELD(name) ({ \ > > > > data->reg_##name = devm_regmap_field_alloc(dev, regmap, \ > > > > stk3310_reg_field_##name); \ > > > > if (IS_ERR(data->reg_##name)) \ > > > > return dev_err_probe(dev, PTR_ERR(data->reg_##name), \ > > > > "reg field alloc failed\n"); \ > > > > }) > > > > > > I am against unneeded use of GNU extensions. > > > > > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make > > > > > > enough room to clean this up the formatting of this macro though. > > > > > > > > > > do {} while (0) is C standard, ({}) is not. > > > > > > > > ({ }) is used throughout the kernel, and is documented as such[1]. I > > > > don't see a reason to avoid it, if it helps readability. > > > > > > I don't see how it makes things better here, and not everybody is familiar with > > > the concept even if it's used in the kernel here and there. Also if a tool is > > > being used in one case it doesn't mean it's suitable for another. > > > > Just to throw in my subjective view here: I don't expect anyone with > > some base level knowledge of C will have doubts about the semantics of > > ({ ... }) and compared to that I find do { ... } while (0) less optimal, > > because it's more verbose and when spotting the "do {" part, the > > semantic only gets clear when you also see the "while (0)". Having said > > that I also dislike the "do" starting on column 0, IMHO the RHS of the > > #define should be intended. > > Thank you, this sums up my opinion on this better than I could have (and > some bits I hadn't considered). > > > So if you ask me, this is not an unneeded use of an extension. The > > extension is used to improve readabilty and I blame the C standard to > > not support this syntax. > > > > While I'm in critics mode: I consider hiding a return in a macro bad > > style. > > Yeah... probably worse than any of the formatting options here. I guess > the proper way would be to use devm_regmap_field_bulk_alloc, but that's > well outside the scope of this series. Perhaps it would make sense to > move the macro definition to just before the function it's used in so > it's at least a little easier to spot? It's only used 8 times. I'd just get rid of the macro - which now has even less advantage as the change here reduces the length of the macro. Normally I'd argue it should be a precursor patch, but here I think it is fine to just do it in this patch to avoid a lot of churn. No macro, no disagreement on formatting ;) I'm not really sure why I let this macro in to begin with. I normally push back on this sort of thing. Must have been a low caffeine day :( Jonathan > > - Aren
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index f93689c61f44..c9a3f02bdd80 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -61,12 +61,12 @@ #define STK3310_REGFIELD(name) \ do { \ data->reg_##name = \ - devm_regmap_field_alloc(&client->dev, regmap, \ + devm_regmap_field_alloc(dev, regmap, \ stk3310_reg_field_##name); \ - if (IS_ERR(data->reg_##name)) { \ - dev_err(&client->dev, "reg field alloc failed.\n"); \ - return PTR_ERR(data->reg_##name); \ - } \ + if (IS_ERR(data->reg_##name)) \ + return dev_err_probe(dev, \ + PTR_ERR(data->reg_##name), \ + "reg field alloc failed.\n"); \ } while (0) static const struct reg_field stk3310_reg_field_state = @@ -517,10 +517,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(&client->dev, stk3310_set_state_disable, data); if (ret) @@ -529,9 +527,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) @@ -560,14 +558,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; STK3310_REGFIELD(state); @@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client) int ret; struct iio_dev *indio_dev; struct stk3310_data *data; + struct device *dev = &client->dev; indio_dev = devm_iio_device_alloc(&client->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; @@ -675,7 +672,7 @@ static int stk3310_probe(struct i2c_client *client) ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), data->supplies); if (ret) - return dev_err_probe(&client->dev, ret, "get regulators failed\n"); + return dev_err_probe(dev, ret, "get regulators failed\n"); ret = stk3310_regmap_init(data); if (ret < 0) @@ -689,13 +686,11 @@ static int stk3310_probe(struct i2c_client *client) ret = stk3310_regulators_enable(data); if (ret) - return dev_err_probe(&client->dev, ret, - "regulator enable failed\n"); + return dev_err_probe(dev, ret, "regulator enable failed\n"); ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data); if (ret) - return dev_err_probe(&client->dev, ret, - "failed to register regulator cleanup\n"); + return dev_err_probe(dev, ret, "failed to register regulator cleanup\n"); ret = stk3310_init(indio_dev); if (ret < 0) @@ -708,18 +703,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(&client->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 | 61 ++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 35 deletions(-)