Re: [PATCH v2] Input: iqs269a - Use scope-based resource management in iqs269_parse_chan()

[Dmitry Torokhov]

On Mon, Mar 04, 2024 at 11:10:43AM -0600, Jeff LaBundy wrote:
> Hi Markus,
> 
> On Mon, Mar 04, 2024 at 10:55:11AM +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > Date: Mon, 4 Mar 2024 10:30:52 +0100
> > 
> > Scope-based resource management became supported also for this software
> > area by contributions of Jonathan Cameron on 2024-02-17.
> > 
> > device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
> > https://lore.kernel.org/r/20240217164249.921878-3-jic23@xxxxxxxxxx
> > 
> > 
> > * Thus use the attribute “__free(fwnode_handle)”.
> > 
> > * Reduce the scope for the local variable “ev_node” into a for loop.
> > 
> > Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 
> > v2:
> > An other cleanup technique was applied as requested by Dmitry Torokhov.
> > 
> > 
> >  drivers/input/misc/iqs269a.c | 73 ++++++++++++++++++------------------
> >  1 file changed, 37 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> > index cd14ff9f57cf..9caee936927b 100644
> > --- a/drivers/input/misc/iqs269a.c
> > +++ b/drivers/input/misc/iqs269a.c
> > @@ -557,7 +557,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> >  			     const struct fwnode_handle *ch_node)
> >  {
> >  	struct i2c_client *client = iqs269->client;
> > -	struct fwnode_handle *ev_node;
> >  	struct iqs269_ch_reg *ch_reg;
> >  	u16 engine_a, engine_b;
> >  	unsigned int reg, val;
> > @@ -734,47 +733,49 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> >  	}
> > 
> >  	for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
> > -		ev_node = fwnode_get_named_child_node(ch_node,
> > -						      iqs269_events[i].name);
> > -		if (!ev_node)
> > -			continue;
> > -
> > -		if (!fwnode_property_read_u32(ev_node, "azoteq,thresh", &val)) {
> > -			if (val > IQS269_CHx_THRESH_MAX) {
> > -				dev_err(&client->dev,
> > -					"Invalid channel %u threshold: %u\n",
> > -					reg, val);
> > -				fwnode_handle_put(ev_node);
> > -				return -EINVAL;
> > +		{
> > +			struct fwnode_handle *ev_node __free(fwnode_handle)
> > +						      = fwnode_get_named_child_node(ch_node,
> > +										    iqs269_events[i].name);
> > +
> > +			if (!ev_node)
> > +				continue;
> > +
> > +			if (!fwnode_property_read_u32(ev_node, "azoteq,thresh", &val)) {
> > +				if (val > IQS269_CHx_THRESH_MAX) {
> > +					dev_err(&client->dev,
> > +						"Invalid channel %u threshold: %u\n",
> > +						reg, val);
> > +					return -EINVAL;
> > +				}
> > +
> > +				ch_reg->thresh[iqs269_events[i].th_offs] = val;
> 
> I may just be a curmudgeon, but this is another NAK for me. The dummy
> curly braces and extra indentation make the code difficult to understand,
> and this simply does not seem like a natural way to write a driver. Just
> to remove 2-3 calls to fwnode_handle_put()?

The extra curly braces are absolutely not needed. The for loop's body
already defines scope, __cleanup()s should be called at the end of the
body.

Thanks.

-- 
Dmitry