From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
Hi Markus,
On Sat, Mar 02, 2024 at 09:24:53AM +0100, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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
From: Markus Elfring <[email protected]>
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/[email protected]
* 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 <[email protected]>
---
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
Hi Markus,
On Mon, Mar 04, 2024 at 10:55:11AM +0100, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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/[email protected]
>
>
> * 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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
> > 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/[email protected]
> >
> >
> > * 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 <[email protected]>
> > ---
> >
> > 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
> 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
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