* [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]
@ 2024-04-10 16:52 R SUNDAR
2024-04-10 17:13 ` Dmitry Baryshkov
2024-04-11 12:17 ` Greg KH
0 siblings, 2 replies; 4+ messages in thread
From: R SUNDAR @ 2024-04-10 16:52 UTC (permalink / raw
To: heikki.krogerus, gregkh, dmitry.baryshkov, neil.armstrong,
christophe.jaillet, u.kleine-koenig
Cc: skhan, javier.carrasco.cruz, linux-usb, linux-kernel, R SUNDAR,
Julia Lawall
use the new cleanup magic to replace of_node_put() with
__free(device_node) marking to auto release and to simplify the error
paths.
Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: R SUNDAR <prosunofficial@gmail.com>
---
drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c
index b17826713753..72ec9ef3ac58 100644
--- a/drivers/usb/typec/mux/nb7vpq904m.c
+++ b/drivers/usb/typec/mux/nb7vpq904m.c
@@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
{
- struct device_node *ep;
u32 data_lanes[4];
int ret, i, j;
-
- ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
+ struct device_node *ep __free(device_node) =
+ of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
if (ep) {
ret = of_property_count_u32_elems(ep, "data-lanes");
if (ret == -EINVAL)
/* Property isn't here, consider default mapping */
- goto out_done;
+ return 0;
if (ret < 0)
- goto out_error;
+ return ret;
if (ret != DATA_LANES_COUNT) {
dev_err(&nb7->client->dev, "expected 4 data lanes\n");
- ret = -EINVAL;
- goto out_error;
+ return -EINVAL;
}
ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
if (ret)
- goto out_error;
+ return ret;
for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
for (j = 0; j < DATA_LANES_COUNT; j++) {
@@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
break;
default:
dev_err(&nb7->client->dev, "invalid data lanes mapping\n");
- ret = -EINVAL;
- goto out_error;
+ return -EINVAL;
}
}
-out_done:
- ret = 0;
-
-out_error:
- of_node_put(ep);
-
- return ret;
+ return 0;
}
static int nb7vpq904m_probe(struct i2c_client *client)
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]
2024-04-10 16:52 [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next] R SUNDAR
@ 2024-04-10 17:13 ` Dmitry Baryshkov
2024-04-11 12:17 ` Greg KH
1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Baryshkov @ 2024-04-10 17:13 UTC (permalink / raw
To: R SUNDAR
Cc: heikki.krogerus, gregkh, neil.armstrong, christophe.jaillet,
u.kleine-koenig, skhan, javier.carrasco.cruz, linux-usb,
linux-kernel, Julia Lawall
On Wed, 10 Apr 2024 at 19:52, R SUNDAR <prosunofficial@gmail.com> wrote:
>
> use the new cleanup magic to replace of_node_put() with
> __free(device_node) marking to auto release and to simplify the error
> paths.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: R SUNDAR <prosunofficial@gmail.com>
> ---
> drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++-----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c
> index b17826713753..72ec9ef3ac58 100644
> --- a/drivers/usb/typec/mux/nb7vpq904m.c
> +++ b/drivers/usb/typec/mux/nb7vpq904m.c
> @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
>
> static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
> {
> - struct device_node *ep;
> u32 data_lanes[4];
> int ret, i, j;
> -
> - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
> + struct device_node *ep __free(device_node) =
> + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
Please stick this to the top of the function. Otherwise LGTM.
>
> if (ep) {
> ret = of_property_count_u32_elems(ep, "data-lanes");
> if (ret == -EINVAL)
> /* Property isn't here, consider default mapping */
> - goto out_done;
> + return 0;
> if (ret < 0)
> - goto out_error;
> + return ret;
>
> if (ret != DATA_LANES_COUNT) {
> dev_err(&nb7->client->dev, "expected 4 data lanes\n");
> - ret = -EINVAL;
> - goto out_error;
> + return -EINVAL;
> }
>
> ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
> if (ret)
> - goto out_error;
> + return ret;
>
> for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> for (j = 0; j < DATA_LANES_COUNT; j++) {
> @@ -358,18 +356,11 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
> break;
> default:
> dev_err(&nb7->client->dev, "invalid data lanes mapping\n");
> - ret = -EINVAL;
> - goto out_error;
> + return -EINVAL;
> }
> }
>
> -out_done:
> - ret = 0;
> -
> -out_error:
> - of_node_put(ep);
> -
> - return ret;
> + return 0;
> }
>
> static int nb7vpq904m_probe(struct i2c_client *client)
> --
> 2.34.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]
2024-04-10 16:52 [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next] R SUNDAR
2024-04-10 17:13 ` Dmitry Baryshkov
@ 2024-04-11 12:17 ` Greg KH
2024-04-12 3:47 ` sundar.R
1 sibling, 1 reply; 4+ messages in thread
From: Greg KH @ 2024-04-11 12:17 UTC (permalink / raw
To: R SUNDAR
Cc: heikki.krogerus, dmitry.baryshkov, neil.armstrong,
christophe.jaillet, u.kleine-koenig, skhan, javier.carrasco.cruz,
linux-usb, linux-kernel, Julia Lawall
On Wed, Apr 10, 2024 at 10:22:22PM +0530, R SUNDAR wrote:
> use the new cleanup magic to replace of_node_put() with
> __free(device_node) marking to auto release and to simplify the error
> paths.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: R SUNDAR <prosunofficial@gmail.com>
All CAPS for your name? Is that how it is written on documents (for
some countries this is normal, but usually not, so I have to ask.)
> ---
> drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++-----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c
> index b17826713753..72ec9ef3ac58 100644
> --- a/drivers/usb/typec/mux/nb7vpq904m.c
> +++ b/drivers/usb/typec/mux/nb7vpq904m.c
> @@ -315,29 +315,27 @@ static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
>
> static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7)
> {
> - struct device_node *ep;
> u32 data_lanes[4];
> int ret, i, j;
> -
> - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
> + struct device_node *ep __free(device_node) =
This use of __free() for device_node would be the first one in the tree
(outside of a macro in a .h file.) Is this really what the OF
maintainer wants to have happen here for this type of pattern?
And if so, why this random USB driver being the first one? Have you
tested this?
> + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>
> if (ep) {
Nit, this function should be rewritten to not work like this, the
"common" path should not be indented, but only the exception (i.e. bail
if ep is not allocated properly.)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next]
2024-04-11 12:17 ` Greg KH
@ 2024-04-12 3:47 ` sundar.R
0 siblings, 0 replies; 4+ messages in thread
From: sundar.R @ 2024-04-12 3:47 UTC (permalink / raw
To: Greg KH
Cc: heikki.krogerus, dmitry.baryshkov, neil.armstrong,
christophe.jaillet, u.kleine-koenig, skhan, javier.carrasco.cruz,
linux-usb, linux-kernel, Julia Lawall
Hi greg k-h,
Thanks for comments provided.
On 11/04/24 17:47, Greg KH wrote:
> On Wed, Apr 10, 2024 at 10:22:22PM +0530, R SUNDAR wrote:
>> use the new cleanup magic to replace of_node_put() with
>> __free(device_node) marking to auto release and to simplify the error
>> paths.
>>
>> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
>> Signed-off-by: R SUNDAR <prosunofficial@gmail.com>
>
> All CAPS for your name? Is that how it is written on documents (for
> some countries this is normal, but usually not, so I have to ask.)
In documents it was like that.I also thought to change to small letters
in git user name.
will change to small letter.
>> -
>> - ep = of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>> + struct device_node *ep __free(device_node) =
>
> And if so, why this random USB driver being the first one? Have you
> tested this?
I haven't tested it.I picked this usb driver to get cleanup module
changes with __free.
I believe since of_node_put() is scope based here,we can use cleanup to
remove the goto label uses in case of error.
> >
>> + of_graph_get_endpoint_by_regs(nb7->client->dev.of_node, 1, 0);
>>
>> if (ep) {
>
> the "common" path should not be indented, but only the exception (i.e. bail
> if ep is not allocated properly.)
I Tried in same line initialization, it exceeds column limit.
so indented in next line.
>
> thanks,
>
> greg k-h
Regards,
Sundar
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-12 3:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-10 16:52 [PATCH] usb: typec: mux: replace of_node_put() with __free [linux-next] R SUNDAR
2024-04-10 17:13 ` Dmitry Baryshkov
2024-04-11 12:17 ` Greg KH
2024-04-12 3:47 ` sundar.R
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.