On Mon, 4 Mar 2024, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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/[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
> 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
>
>
>
On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote:
>
>
> On Mon, 4 Mar 2024, Markus Elfring wrote:
>
> > From: Markus Elfring <[email protected]>
> > 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/[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
> > 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
>> 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.
…
>> +++ 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
> 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
On Mon, Mar 04, 2024 at 11:55:23AM +0100, Julia Lawall wrote:
>
>
> On Mon, 4 Mar 2024, Markus Elfring wrote:
>
> > From: Markus Elfring <[email protected]>
> > 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/[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
> > 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