[PATCH] Input: iqs269a - Use common error handling code in iqs269_parse_chan()

[PATCH] Input: iqs269a - Use common error handling code in iqs269_parse_chan()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 2 Mar 2024 09:15:20 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function implementation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/misc/iqs269a.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index cd14ff9f57cf..1379f80c00e2 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -744,8 +744,7 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u threshold: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
-				return -EINVAL;
+				goto put_fwnode;
 			}

 			ch_reg->thresh[iqs269_events[i].th_offs] = val;
@@ -758,8 +757,7 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u hysteresis: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
-				return -EINVAL;
+				goto put_fwnode;
 			}

 			if (i == IQS269_EVENT_DEEP_DN ||
@@ -805,6 +803,10 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 	}

 	return 0;
+
+put_fwnode:
+	fwnode_handle_put(ev_node);
+	return -EINVAL;
 }

 static int iqs269_parse_prop(struct iqs269_private *iqs269)
--
2.44.0

Re: [PATCH] Input: iqs269a - Use common error handling code in iqs269_parse_chan()

[Dmitry Torokhov]

Hi Markus,

On Sat, Mar 02, 2024 at 09:24:53AM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 2 Mar 2024 09:15:20 +0100
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function implementation.

No, I do not think this is needed. However if you can introduce a fwnode
cleanup/free function (see include/linux/cleanup.h) maybe it would be
more useful and we could apply it to various drivers.

Thanks.

-- 
Dmitry

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

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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@kernel.org


* 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@users.sourceforge.net>
---

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;
 			}

-			ch_reg->thresh[iqs269_events[i].th_offs] = val;
-		}
-
-		if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
-			u8 *hyst = &ch_reg->hyst;
-
-			if (val > IQS269_CHx_HYST_MAX) {
-				dev_err(&client->dev,
-					"Invalid channel %u hysteresis: %u\n",
-					reg, val);
-				fwnode_handle_put(ev_node);
-				return -EINVAL;
+			if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
+				u8 *hyst = &ch_reg->hyst;
+
+				if (val > IQS269_CHx_HYST_MAX) {
+					dev_err(&client->dev,
+						"Invalid channel %u hysteresis: %u\n",
+						reg, val);
+					return -EINVAL;
+				}
+
+				if (i == IQS269_EVENT_DEEP_DN ||
+				    i == IQS269_EVENT_DEEP_UP) {
+					*hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
+					*hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
+				} else if (i == IQS269_EVENT_TOUCH_DN ||
+					   i == IQS269_EVENT_TOUCH_UP) {
+					*hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
+					*hyst |= val;
+				}
 			}

-			if (i == IQS269_EVENT_DEEP_DN ||
-			    i == IQS269_EVENT_DEEP_UP) {
-				*hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
-				*hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
-			} else if (i == IQS269_EVENT_TOUCH_DN ||
-				   i == IQS269_EVENT_TOUCH_UP) {
-				*hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
-				*hyst |= val;
-			}
+			error = fwnode_property_read_u32(ev_node, "linux,code", &val);
 		}

-		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
-		fwnode_handle_put(ev_node);
 		if (error == -EINVAL) {
 			continue;
 		} else if (error) {
--
2.44.0

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

[Jeff LaBundy]

Hi Markus,

On Mon, Mar 04, 2024 at 10:55:11AM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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@kernel.org
> 
> 
> * 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@users.sourceforge.net>
> ---
> 
> 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()?

>  			}
> 
> -			ch_reg->thresh[iqs269_events[i].th_offs] = val;
> -		}
> -
> -		if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
> -			u8 *hyst = &ch_reg->hyst;
> -
> -			if (val > IQS269_CHx_HYST_MAX) {
> -				dev_err(&client->dev,
> -					"Invalid channel %u hysteresis: %u\n",
> -					reg, val);
> -				fwnode_handle_put(ev_node);
> -				return -EINVAL;
> +			if (!fwnode_property_read_u32(ev_node, "azoteq,hyst", &val)) {
> +				u8 *hyst = &ch_reg->hyst;
> +
> +				if (val > IQS269_CHx_HYST_MAX) {
> +					dev_err(&client->dev,
> +						"Invalid channel %u hysteresis: %u\n",
> +						reg, val);
> +					return -EINVAL;
> +				}
> +
> +				if (i == IQS269_EVENT_DEEP_DN ||
> +				    i == IQS269_EVENT_DEEP_UP) {
> +					*hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
> +					*hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
> +				} else if (i == IQS269_EVENT_TOUCH_DN ||
> +					   i == IQS269_EVENT_TOUCH_UP) {
> +					*hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
> +					*hyst |= val;
> +				}
>  			}
> 
> -			if (i == IQS269_EVENT_DEEP_DN ||
> -			    i == IQS269_EVENT_DEEP_UP) {
> -				*hyst &= ~IQS269_CHx_HYST_DEEP_MASK;
> -				*hyst |= (val << IQS269_CHx_HYST_DEEP_SHIFT);
> -			} else if (i == IQS269_EVENT_TOUCH_DN ||
> -				   i == IQS269_EVENT_TOUCH_UP) {
> -				*hyst &= ~IQS269_CHx_HYST_TOUCH_MASK;
> -				*hyst |= val;
> -			}
> +			error = fwnode_property_read_u32(ev_node, "linux,code", &val);
>  		}
> 
> -		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
> -		fwnode_handle_put(ev_node);
>  		if (error == -EINVAL) {
>  			continue;
>  		} else if (error) {
> --
> 2.44.0
> 

Kind regards,
Jeff LaBundy

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@users.sourceforge.net>
> > 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@kernel.org
> > 
> > 
> > * 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@users.sourceforge.net>
> > ---
> > 
> > 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

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

[Markus Elfring]

> 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.

I present an other development opinion here.
I got the impression that the required scope should be smaller for
the adjusted local variable “ev_node” (according to the previous function implementation).

Otherwise:
How do you think about to move any source code part from the loop
into a separate function?

Regards,
Markus

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

[Dmitry Torokhov]

On Mon, Mar 04, 2024 at 06:48:58PM +0100, Markus Elfring wrote:
> > 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.
> 
> I present an other development opinion here.
> I got the impression that the required scope should be smaller for
> the adjusted local variable “ev_node” (according to the previous function implementation).
> 
> Otherwise:
> How do you think about to move any source code part from the loop
> into a separate function?

No, it should simply look like this:


diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index cd14ff9f57cf..98119c48c65f 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,8 +733,9 @@ 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);
+		struct fwnode_handle *ev_node __free(fwnode_handle) =
+			fwnode_get_named_child_node(ch_node,
+						    iqs269_events[i].name);
 		if (!ev_node)
 			continue;
 
@@ -744,7 +744,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u threshold: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -758,7 +757,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 				dev_err(&client->dev,
 					"Invalid channel %u hysteresis: %u\n",
 					reg, val);
-				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -774,7 +772,6 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
 		}
 
 		error = fwnode_property_read_u32(ev_node, "linux,code", &val);
-		fwnode_handle_put(ev_node);
 		if (error == -EINVAL) {
 			continue;
 		} else if (error) {

Thanks.

-- 
Dmitry