Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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