* [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() @ 2024-03-02 11:42 Markus Elfring 2024-03-04 3:12 ` Jeff LaBundy 0 siblings, 1 reply; 11+ messages in thread From: Markus Elfring @ 2024-03-02 11:42 UTC (permalink / raw) To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König Cc: LKML From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 2 Mar 2024 11:44:17 +0100 Add a jump target so that a bit of exception handling can be better reused at the end of this function implementation. This issue was transformed by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/input/misc/iqs626a.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c index 0dab54d3a060..fa9570755f7b 100644 --- a/drivers/input/misc/iqs626a.c +++ b/drivers/input/misc/iqs626a.c @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid input type: %u\n", val); - fwnode_handle_put(ev_node); - return -EINVAL; + goto put_fwnode; } iqs626->kp_type[ch_id][i] = val; @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s channel hysteresis: %u\n", fwnode_get_name(ch_node), val); - fwnode_handle_put(ev_node); - return -EINVAL; + goto put_fwnode; } if (i == IQS626_EVENT_DEEP_DN || @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s channel threshold: %u\n", fwnode_get_name(ch_node), val); - fwnode_handle_put(ev_node); - return -EINVAL; + goto put_fwnode; } if (ch_id == IQS626_CH_HALL) @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626, } return 0; + +put_fwnode: + fwnode_handle_put(ev_node); + return -EINVAL; } static noinline_for_stack int -- 2.44.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() 2024-03-02 11:42 [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() Markus Elfring @ 2024-03-04 3:12 ` Jeff LaBundy 2024-03-04 4:29 ` Dmitry Torokhov 0 siblings, 1 reply; 11+ messages in thread From: Jeff LaBundy @ 2024-03-04 3:12 UTC (permalink / raw) To: Markus Elfring Cc: linux-input, kernel-janitors, Dmitry Torokhov, Rob Herring, Uwe Kleine-König, LKML Hi Markus, On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 2 Mar 2024 11:44:17 +0100 > > Add a jump target so that a bit of exception handling can be better reused > at the end of this function implementation. > > This issue was transformed by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/input/misc/iqs626a.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > index 0dab54d3a060..fa9570755f7b 100644 > --- a/drivers/input/misc/iqs626a.c > +++ b/drivers/input/misc/iqs626a.c > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid input type: %u\n", > val); > - fwnode_handle_put(ev_node); > - return -EINVAL; > + goto put_fwnode; > } > > iqs626->kp_type[ch_id][i] = val; > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s channel hysteresis: %u\n", > fwnode_get_name(ch_node), val); > - fwnode_handle_put(ev_node); > - return -EINVAL; > + goto put_fwnode; > } > > if (i == IQS626_EVENT_DEEP_DN || > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s channel threshold: %u\n", > fwnode_get_name(ch_node), val); > - fwnode_handle_put(ev_node); > - return -EINVAL; > + goto put_fwnode; > } > > if (ch_id == IQS626_CH_HALL) > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626, > } > > return 0; > + > +put_fwnode: > + fwnode_handle_put(ev_node); > + return -EINVAL; > } > > static noinline_for_stack int > -- > 2.44.0 > Thank you for this patch, but it seems like a NAK to me. I think this is a matter of personal preference, and according to mine, it is much more confusing to insert a goto label after a function's primary return path than to have 2-3 calls to fwnode_handle_put(). If you feel strongly otherwise, then I would suggest a helper function as recommended by Dmitry in another thread. However, maybe that helper should live in the driver core, as I suspect this driver is not the only place we can avoid calling fwnode_handle_put() in an error path that returns an int. Kind regards, Jeff LaBundy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() 2024-03-04 3:12 ` Jeff LaBundy @ 2024-03-04 4:29 ` Dmitry Torokhov 2024-03-04 8:18 ` Dan Carpenter 2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring 0 siblings, 2 replies; 11+ messages in thread From: Dmitry Torokhov @ 2024-03-04 4:29 UTC (permalink / raw) To: Jeff LaBundy Cc: Markus Elfring, linux-input, kernel-janitors, Rob Herring, Uwe Kleine-König, LKML On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote: > Hi Markus, > > On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote: > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Sat, 2 Mar 2024 11:44:17 +0100 > > > > Add a jump target so that a bit of exception handling can be better reused > > at the end of this function implementation. > > > > This issue was transformed by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > --- > > drivers/input/misc/iqs626a.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > index 0dab54d3a060..fa9570755f7b 100644 > > --- a/drivers/input/misc/iqs626a.c > > +++ b/drivers/input/misc/iqs626a.c > > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > dev_err(&client->dev, > > "Invalid input type: %u\n", > > val); > > - fwnode_handle_put(ev_node); > > - return -EINVAL; > > + goto put_fwnode; > > } > > > > iqs626->kp_type[ch_id][i] = val; > > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > dev_err(&client->dev, > > "Invalid %s channel hysteresis: %u\n", > > fwnode_get_name(ch_node), val); > > - fwnode_handle_put(ev_node); > > - return -EINVAL; > > + goto put_fwnode; > > } > > > > if (i == IQS626_EVENT_DEEP_DN || > > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > dev_err(&client->dev, > > "Invalid %s channel threshold: %u\n", > > fwnode_get_name(ch_node), val); > > - fwnode_handle_put(ev_node); > > - return -EINVAL; > > + goto put_fwnode; > > } > > > > if (ch_id == IQS626_CH_HALL) > > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > } > > > > return 0; > > + > > +put_fwnode: > > + fwnode_handle_put(ev_node); > > + return -EINVAL; > > } > > > > static noinline_for_stack int > > -- > > 2.44.0 > > > > Thank you for this patch, but it seems like a NAK to me. I think this is > a matter of personal preference, and according to mine, it is much more > confusing to insert a goto label after a function's primary return path > than to have 2-3 calls to fwnode_handle_put(). > > If you feel strongly otherwise, then I would suggest a helper function as > recommended by Dmitry in another thread. However, maybe that helper should > live in the driver core, as I suspect this driver is not the only place we > can avoid calling fwnode_handle_put() in an error path that returns an int. Yes, it should go into include/linux/fwnode.h, something like DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T)); Then drivers can do: struct fwnode_handle *ev_node __free(fwnode) = fwnode_get_named_child_node(ch_node, ev_name); and have it automatically be "put" once execution leaves the variable scope. Ah, we actually already have it defined in include/linux/property.h, all the better. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() 2024-03-04 4:29 ` Dmitry Torokhov @ 2024-03-04 8:18 ` Dan Carpenter 2024-03-04 12:31 ` Markus Elfring 2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring 1 sibling, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2024-03-04 8:18 UTC (permalink / raw) To: Dmitry Torokhov Cc: Jeff LaBundy, Markus Elfring, linux-input, kernel-janitors, Rob Herring, Uwe Kleine-König, LKML On Sun, Mar 03, 2024 at 08:29:49PM -0800, Dmitry Torokhov wrote: > On Sun, Mar 03, 2024 at 09:12:16PM -0600, Jeff LaBundy wrote: > > Hi Markus, > > > > On Sat, Mar 02, 2024 at 12:42:08PM +0100, Markus Elfring wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > > Date: Sat, 2 Mar 2024 11:44:17 +0100 > > > > > > Add a jump target so that a bit of exception handling can be better reused > > > at the end of this function implementation. > > > > > > This issue was transformed by using the Coccinelle software. > > > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > > --- > > > drivers/input/misc/iqs626a.c | 13 +++++++------ > > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > > index 0dab54d3a060..fa9570755f7b 100644 > > > --- a/drivers/input/misc/iqs626a.c > > > +++ b/drivers/input/misc/iqs626a.c > > > @@ -530,8 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > dev_err(&client->dev, > > > "Invalid input type: %u\n", > > > val); > > > - fwnode_handle_put(ev_node); > > > - return -EINVAL; > > > + goto put_fwnode; > > > } > > > > > > iqs626->kp_type[ch_id][i] = val; > > > @@ -545,8 +544,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > dev_err(&client->dev, > > > "Invalid %s channel hysteresis: %u\n", > > > fwnode_get_name(ch_node), val); > > > - fwnode_handle_put(ev_node); > > > - return -EINVAL; > > > + goto put_fwnode; > > > } > > > > > > if (i == IQS626_EVENT_DEEP_DN || > > > @@ -566,8 +564,7 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > dev_err(&client->dev, > > > "Invalid %s channel threshold: %u\n", > > > fwnode_get_name(ch_node), val); > > > - fwnode_handle_put(ev_node); > > > - return -EINVAL; > > > + goto put_fwnode; > > > } > > > > > > if (ch_id == IQS626_CH_HALL) > > > @@ -580,6 +577,10 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > > } > > > > > > return 0; > > > + > > > +put_fwnode: > > > + fwnode_handle_put(ev_node); > > > + return -EINVAL; > > > } > > > > > > static noinline_for_stack int > > > -- > > > 2.44.0 > > > > > > > Thank you for this patch, but it seems like a NAK to me. I think this is > > a matter of personal preference, and according to mine, it is much more > > confusing to insert a goto label after a function's primary return path > > than to have 2-3 calls to fwnode_handle_put(). > > > > If you feel strongly otherwise, then I would suggest a helper function as > > recommended by Dmitry in another thread. However, maybe that helper should > > live in the driver core, as I suspect this driver is not the only place we > > can avoid calling fwnode_handle_put() in an error path that returns an int. > > Yes, it should go into include/linux/fwnode.h, something like > > DEFINE_FREE(fwnode, struct fwnode_handle *, if (_T) fwnode_hanlde_put(_T)); > > Then drivers can do: > > struct fwnode_handle *ev_node __free(fwnode) = > fwnode_get_named_child_node(ch_node, ev_name); > > and have it automatically be "put" once execution leaves the variable > scope. > > Ah, we actually already have it defined in include/linux/property.h, all > the better. It's already there. DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T)) I can send a patch for this. You need to be a bit carefull to move the declaration into the correct scope for this to work. I should write some Smatch rules for this... regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Input: iqs626a - Use common error handling code in iqs626_parse_events() 2024-03-04 8:18 ` Dan Carpenter @ 2024-03-04 12:31 ` Markus Elfring 0 siblings, 0 replies; 11+ messages in thread From: Markus Elfring @ 2024-03-04 12:31 UTC (permalink / raw) To: Dan Carpenter, Dmitry Torokhov, Jeff LaBundy, linux-input, kernel-janitors Cc: Rob Herring, Uwe Kleine-König, LKML > DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T)) > > I can send a patch for this. You need to be a bit carefull to move > the declaration into the correct scope for this to work. I should write > some Smatch rules for this... I became also curious how available development tools will evolve further for improved handling of scope-based resource management. Regards, Markus ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 4:29 ` Dmitry Torokhov 2024-03-04 8:18 ` Dan Carpenter @ 2024-03-04 10:52 ` Markus Elfring 2024-03-04 10:55 ` Julia Lawall 1 sibling, 1 reply; 11+ messages in thread From: Markus Elfring @ 2024-03-04 10:52 UTC (permalink / raw) To: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König Cc: LKML, Jonathan Cameron From: Markus Elfring <elfring@users.sourceforge.net> Date: Mon, 4 Mar 2024 11:40:04 +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 and Jeff LaBundy. drivers/input/misc/iqs626a.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c index 0dab54d3a060..86fcb5134f45 100644 --- a/drivers/input/misc/iqs626a.c +++ b/drivers/input/misc/iqs626a.c @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, { struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; struct i2c_client *client = iqs626->client; - struct fwnode_handle *ev_node; const char *ev_name; u8 *thresh, *hyst; unsigned int val; @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626, if (!iqs626_channels[ch_id].events[i]) continue; + struct fwnode_handle *ev_node __free(fwnode_handle); + if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { /* * Trackpad touch events are simply described under the @@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid input type: %u\n", val); - fwnode_handle_put(ev_node); return -EINVAL; } @@ -545,7 +545,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s channel hysteresis: %u\n", fwnode_get_name(ch_node), val); - fwnode_handle_put(ev_node); return -EINVAL; } @@ -566,7 +565,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, dev_err(&client->dev, "Invalid %s channel threshold: %u\n", fwnode_get_name(ch_node), val); - fwnode_handle_put(ev_node); return -EINVAL; } @@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, else *(thresh + iqs626_events[i].th_offs) = val; } - - fwnode_handle_put(ev_node); } return 0; -- 2.44.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring @ 2024-03-04 10:55 ` Julia Lawall 2024-03-04 11:32 ` Dan Carpenter ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Julia Lawall @ 2024-03-04 10:55 UTC (permalink / raw) To: Markus Elfring Cc: linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron [-- Attachment #1: Type: text/plain, Size: 2651 bytes --] On Mon, 4 Mar 2024, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 4 Mar 2024 11:40:04 +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 > and Jeff LaBundy. > > > drivers/input/misc/iqs626a.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > index 0dab54d3a060..86fcb5134f45 100644 > --- a/drivers/input/misc/iqs626a.c > +++ b/drivers/input/misc/iqs626a.c > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > { > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; > struct i2c_client *client = iqs626->client; > - struct fwnode_handle *ev_node; > const char *ev_name; > u8 *thresh, *hyst; > unsigned int val; > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626, > if (!iqs626_channels[ch_id].events[i]) > continue; > > + struct fwnode_handle *ev_node __free(fwnode_handle); Doesn't this need to be initialized? julia > + > if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3) { > /* > * Trackpad touch events are simply described under the > @@ -530,7 +531,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid input type: %u\n", > val); > - fwnode_handle_put(ev_node); > return -EINVAL; > } > > @@ -545,7 +545,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s channel hysteresis: %u\n", > fwnode_get_name(ch_node), val); > - fwnode_handle_put(ev_node); > return -EINVAL; > } > > @@ -566,7 +565,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > dev_err(&client->dev, > "Invalid %s channel threshold: %u\n", > fwnode_get_name(ch_node), val); > - fwnode_handle_put(ev_node); > return -EINVAL; > } > > @@ -575,8 +573,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > else > *(thresh + iqs626_events[i].th_offs) = val; > } > - > - fwnode_handle_put(ev_node); > } > > return 0; > -- > 2.44.0 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 10:55 ` Julia Lawall @ 2024-03-04 11:32 ` Dan Carpenter 2024-03-04 13:30 ` Markus Elfring 2024-03-04 12:10 ` [PATCH v2] " Markus Elfring 2024-03-04 17:16 ` Dmitry Torokhov 2 siblings, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2024-03-04 11:32 UTC (permalink / raw) To: Julia Lawall Cc: Markus Elfring, linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote: > > > On Mon, 4 Mar 2024, Markus Elfring wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Mon, 4 Mar 2024 11:40:04 +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 > > and Jeff LaBundy. > > > > > > drivers/input/misc/iqs626a.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > index 0dab54d3a060..86fcb5134f45 100644 > > --- a/drivers/input/misc/iqs626a.c > > +++ b/drivers/input/misc/iqs626a.c > > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > { > > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; > > struct i2c_client *client = iqs626->client; > > - struct fwnode_handle *ev_node; > > const char *ev_name; > > u8 *thresh, *hyst; > > unsigned int val; > > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > if (!iqs626_channels[ch_id].events[i]) > > continue; > > > > + struct fwnode_handle *ev_node __free(fwnode_handle); > > Doesn't this need to be initialized? > Only if you declared before the continue; statement. Generally kernel style is that you have to declare variables at the start of the block... But that's becoming less universal now that it's not a compile error. regards, dan carpenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 11:32 ` Dan Carpenter @ 2024-03-04 13:30 ` Markus Elfring 0 siblings, 0 replies; 11+ messages in thread From: Markus Elfring @ 2024-03-04 13:30 UTC (permalink / raw) To: Dan Carpenter, Julia Lawall, linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König Cc: LKML, Jonathan Cameron > Generally kernel style is that you have to declare variables at the start of the block... > But that's becoming less universal now that it's not a compile error. Will it become more feasible to adjust the scope for further (local) variables? Regards, Markus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 10:55 ` Julia Lawall 2024-03-04 11:32 ` Dan Carpenter @ 2024-03-04 12:10 ` Markus Elfring 2024-03-04 17:16 ` Dmitry Torokhov 2 siblings, 0 replies; 11+ messages in thread From: Markus Elfring @ 2024-03-04 12:10 UTC (permalink / raw) To: Julia Lawall, linux-input, kernel-janitors, Dmitry Torokhov, Jeff LaBundy, Rob Herring, Uwe Kleine-König Cc: LKML, Jonathan Cameron >> 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. … >> +++ b/drivers/input/misc/iqs626a.c >> @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, >> { >> struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; >> struct i2c_client *client = iqs626->client; >> - struct fwnode_handle *ev_node; >> const char *ev_name; >> u8 *thresh, *hyst; >> unsigned int val; >> @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626, >> if (!iqs626_channels[ch_id].events[i]) >> continue; >> >> + struct fwnode_handle *ev_node __free(fwnode_handle); > > Doesn't this need to be initialized? This variable should usually be set in both branches of the subsequent if statement, shouldn't it? Please take another look at the proposed scope reduction for the affected variable. May additional curly brackets be omitted for this source code transformation? Regards, Markus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] Input: iqs626a - Use scope-based resource management in iqs626_parse_events() 2024-03-04 10:55 ` Julia Lawall 2024-03-04 11:32 ` Dan Carpenter 2024-03-04 12:10 ` [PATCH v2] " Markus Elfring @ 2024-03-04 17:16 ` Dmitry Torokhov 2 siblings, 0 replies; 11+ messages in thread From: Dmitry Torokhov @ 2024-03-04 17:16 UTC (permalink / raw) To: Julia Lawall Cc: Markus Elfring, linux-input, kernel-janitors, Jeff LaBundy, Rob Herring, Uwe Kleine-König, LKML, Jonathan Cameron On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote: > > > On Mon, 4 Mar 2024, Markus Elfring wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Mon, 4 Mar 2024 11:40:04 +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 > > and Jeff LaBundy. > > > > > > drivers/input/misc/iqs626a.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c > > index 0dab54d3a060..86fcb5134f45 100644 > > --- a/drivers/input/misc/iqs626a.c > > +++ b/drivers/input/misc/iqs626a.c > > @@ -462,7 +462,6 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > { > > struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg; > > struct i2c_client *client = iqs626->client; > > - struct fwnode_handle *ev_node; > > const char *ev_name; > > u8 *thresh, *hyst; > > unsigned int val; > > @@ -501,6 +500,8 @@ iqs626_parse_events(struct iqs626_private *iqs626, > > if (!iqs626_channels[ch_id].events[i]) > > continue; > > > > + struct fwnode_handle *ev_node __free(fwnode_handle); > > Doesn't this need to be initialized? It would be better if we had a helper here, iqs626_get_ev_node() or something. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-03-04 17:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-02 11:42 [PATCH] Input: iqs626a - Use common error handling code in iqs626_parse_events() Markus Elfring 2024-03-04 3:12 ` Jeff LaBundy 2024-03-04 4:29 ` Dmitry Torokhov 2024-03-04 8:18 ` Dan Carpenter 2024-03-04 12:31 ` Markus Elfring 2024-03-04 10:52 ` [PATCH v2] Input: iqs626a - Use scope-based resource management " Markus Elfring 2024-03-04 10:55 ` Julia Lawall 2024-03-04 11:32 ` Dan Carpenter 2024-03-04 13:30 ` Markus Elfring 2024-03-04 12:10 ` [PATCH v2] " Markus Elfring 2024-03-04 17:16 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox