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(-)
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;
}
--
2.47.0On 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...
--
With Best Regards,
Andy Shevchenko
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)
--
With Best Regards,
Andy Shevchenko
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
--
With Best Regards,
Andy Shevchenko
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 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
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 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.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2024 Red Hat, Inc.