usb: typec: mux: replace of_node_put() with __free [linux-next]
Subsystems (2)
PATCH 1/1 usb: typec: mux: replace of_node_put() with __free [linux-next]
use the new cleanup magic to replace of_node_put() with __free(device_node) marking to auto release and to simplify the error paths.
--- drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
drivers/usb/typec/mux/nb7vpq904m.c +8 -17
@@ -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)
Signature · 1 line
2.34.1
Quoted 16 lines · R SUNDAR
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 <[email protected]>Signed-off-by R SUNDAR <[email protected]>--- drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)drivers/usb/typec/mux/nb7vpq904m.c +0
-0
drivers/usb/typec/mux/nb7vpq904m.c static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { · line 321
Quoted 10 lines
drivers/usb/typec/mux/nb7vpq904m.c +2
-3@@ -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.
Quoted 49 lines
drivers/usb/typec/mux/nb7vpq904m.c +6
-15if (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
Signature · 2 lines
With best wishes Dmitry
Greg KH
maintainerQuoted 6 lines · R SUNDAR
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 <[email protected]>Signed-off-by R SUNDAR <[email protected]>
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.)
Quoted 9 lines
--- drivers/usb/typec/mux/nb7vpq904m.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)drivers/usb/typec/mux/nb7vpq904m.c +0
-0
drivers/usb/typec/mux/nb7vpq904m.c static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { · line 320
Quoted 9 lines
drivers/usb/typec/mux/nb7vpq904m.c +1
-3@@ -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?
drivers/usb/typec/mux/nb7vpq904m.c static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { · line 323
Quoted 3 lines
drivers/usb/typec/mux/nb7vpq904m.c +1
-0+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
Hi greg k-h,
Thanks for comments provided.
Quoted 10 lines · Greg KH
Quoted 6 lines · R SUNDAR
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 <[email protected]>Signed-off-by R SUNDAR <[email protected]>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.
Quoted 6 lines
drivers/usb/typec/mux/nb7vpq904m.c static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { · line 320
Quoted 3 lines
drivers/usb/typec/mux/nb7vpq904m.c +1
-2- - 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.
Quoted 7 lines
drivers/usb/typec/mux/nb7vpq904m.c static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = { · line 323
Quoted 4 lines
drivers/usb/typec/mux/nb7vpq904m.c +1
-0+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.
Quoted 4 lines
thanks,
greg k-h
Regards,
Sundar