| Message ID | 20240420164927.15290-1-prosunofficial@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] remove indentation for common path [linux-next] | expand |
On Sat, 20 Apr 2024 at 19:49, sundar <prosunofficial@gmail.com> wrote: > > Added check if pointer is null and removed indentation for common path > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: sundar <prosunofficial@gmail.com> > --- > > Fixed nitpicks in code according to comments received on other patch. > > https://lore.kernel.org/all/2024041103-doornail-professor-7c1e@gregkh/ > > goal is to get rid of of_node_put,but sending this patch first to do one > thing at a time. > > Changes since v1 - fixed the typo error for spell from identation to > indentation > > v1 patch link - https://lore.kernel.org/all/20240420145522.15018-1-prosunofficial@gmail.com/ > > drivers/usb/typec/mux/nb7vpq904m.c | 49 +++++++++++++++--------------- > 1 file changed, 25 insertions(+), 24 deletions(-) > > diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c > index b17826713753..fe0257840dd5 100644 > --- a/drivers/usb/typec/mux/nb7vpq904m.c > +++ b/drivers/usb/typec/mux/nb7vpq904m.c > @@ -321,35 +321,37 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > > ep = 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; > - if (ret < 0) > - goto out_error; > - > - if (ret != DATA_LANES_COUNT) { > - dev_err(&nb7->client->dev, "expected 4 data lanes\n"); > - ret = -EINVAL; > - goto out_error; > - } > + if (!ep) > + return 0; > > - ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); > - if (ret) > - goto out_error; > + ret = of_property_count_u32_elems(ep, "data-lanes"); > + if (ret == -EINVAL) > + /* Property isn't here, consider default mapping */ > + goto out_done; > + if (ret < 0) > + goto out_error; > + > + if (ret != DATA_LANES_COUNT) { > + dev_err(&nb7->client->dev, "expected 4 data lanes\n"); > + ret = -EINVAL; > + goto out_error; > + } > > - for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { > - for (j = 0; j < DATA_LANES_COUNT; j++) { > - if (data_lanes[j] != supported_data_lane_mapping[i][j]) > - break; > - } > + ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); > + if (ret) > + goto out_error; > > - if (j == DATA_LANES_COUNT) > + for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { > + for (j = 0; j < DATA_LANES_COUNT; j++) { > + if (data_lanes[j] != supported_data_lane_mapping[i][j]) > break; > } > > - switch (i) { > + if (j == DATA_LANES_COUNT) > + break; > + } > + > + switch (i) { > case NORMAL_LANE_MAPPING: switch-cases should also be shifted one level to the left, see Documentation/process/coding-style.rst > break; > case INVERT_LANE_MAPPING: > @@ -360,7 +362,6 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > dev_err(&nb7->client->dev, "invalid data lanes mapping\n"); > ret = -EINVAL; > goto out_error; > - } > } > > out_done: > -- > 2.34.1 >
On 20/04/24 22:37, Joe Perches wrote: > On Sat, 2024-04-20 at 22:19 +0530, sundar wrote: > > >> ``` > @@ -321,35 +321,37 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > > ep = 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; > - if (ret < 0) > - goto out_error; > - > - if (ret != DATA_LANES_COUNT) { > - dev_err(&nb7->client->dev, "expected 4 data lanes\n"); > - ret = -EINVAL; > - goto out_error; > - } > + if (!ep) > + return 0; > ``` > > > Not equivalent code as the out_error: > > of_node_put(ep); > > isn't done > > Hi joe perches, If ep is null, I believe we dont need to call of_node_put. Because passing null pointer to of_node_put() make no difference. In of_node_put() definition, if pointer is null, there is no operation. Thanks, Sundar
On 21/04/24 02:17, Dmitry Baryshkov wrote: > On Sat, 20 Apr 2024 at 19:49, sundar <prosunofficial@gmail.com> wrote: >> >> Added check if pointer is null and removed indentation for common path >> >> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: sundar <prosunofficial@gmail.com> >> --- >> >> Fixed nitpicks in code according to comments received on other patch. >> >> https://lore.kernel.org/all/2024041103-doornail-professor-7c1e@gregkh/ >> >> goal is to get rid of of_node_put,but sending this patch first to do one >> thing at a time. >> >> Changes since v1 - fixed the typo error for spell from identation to >> indentation >> >> v1 patch link - https://lore.kernel.org/all/20240420145522.15018-1-prosunofficial@gmail.com/ >> >> drivers/usb/typec/mux/nb7vpq904m.c | 49 +++++++++++++++--------------- >> 1 file changed, 25 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c >> index b17826713753..fe0257840dd5 100644 >> --- a/drivers/usb/typec/mux/nb7vpq904m.c >> +++ b/drivers/usb/typec/mux/nb7vpq904m.c >> @@ -321,35 +321,37 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) >> >> ep = 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; >> - if (ret < 0) >> - goto out_error; >> - >> - if (ret != DATA_LANES_COUNT) { >> - dev_err(&nb7->client->dev, "expected 4 data lanes\n"); >> - ret = -EINVAL; >> - goto out_error; >> - } >> + if (!ep) >> + return 0; >> >> - ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); >> - if (ret) >> - goto out_error; >> + ret = of_property_count_u32_elems(ep, "data-lanes"); >> + if (ret == -EINVAL) >> + /* Property isn't here, consider default mapping */ >> + goto out_done; >> + if (ret < 0) >> + goto out_error; >> + >> + if (ret != DATA_LANES_COUNT) { >> + dev_err(&nb7->client->dev, "expected 4 data lanes\n"); >> + ret = -EINVAL; >> + goto out_error; >> + } >> >> - for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { >> - for (j = 0; j < DATA_LANES_COUNT; j++) { >> - if (data_lanes[j] != supported_data_lane_mapping[i][j]) >> - break; >> - } >> + ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); >> + if (ret) >> + goto out_error; >> >> - if (j == DATA_LANES_COUNT) >> + for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { >> + for (j = 0; j < DATA_LANES_COUNT; j++) { >> + if (data_lanes[j] != supported_data_lane_mapping[i][j]) >> break; >> } >> >> - switch (i) { >> + if (j == DATA_LANES_COUNT) >> + break; >> + } >> + >> + switch (i) { >> case NORMAL_LANE_MAPPING: > > switch-cases should also be shifted one level to the left, see > Documentation/process/coding-style.rst > >> break; >> case INVERT_LANE_MAPPING: >> @@ -360,7 +362,6 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) >> dev_err(&nb7->client->dev, "invalid data lanes mapping\n"); >> ret = -EINVAL; >> goto out_error; >> - } >> } >> >> out_done: >> -- >> 2.34.1 >> > > Hi Dmitry, Thanks for the review.will fix my code. Thanks, Sundar
On Sun, 2024-04-21 at 06:13 +0530, sundar wrote: > On 20/04/24 22:37, Joe Perches wrote: > > On Sat, 2024-04-20 at 22:19 +0530, sundar wrote: > > > > > > > ``` > > @@ -321,35 +321,37 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) > > > > ep = 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; > > - if (ret < 0) > > - goto out_error; > > - > > - if (ret != DATA_LANES_COUNT) { > > - dev_err(&nb7->client->dev, "expected 4 data lanes\n"); > > - ret = -EINVAL; > > - goto out_error; > > - } > > + if (!ep) > > + return 0; > > ``` > > > > > > Not equivalent code as the out_error: > > > > of_node_put(ep); > > > > isn't done > > > > > > Hi joe perches, > > If ep is null, I believe we dont need to call of_node_put. Because > passing null pointer to of_node_put() make no difference. > > In of_node_put() definition, if pointer is null, there is no operation. > Fine, but you should explain that in the changelog and not make reviewers look it up.
diff --git a/drivers/usb/typec/mux/nb7vpq904m.c b/drivers/usb/typec/mux/nb7vpq904m.c index b17826713753..fe0257840dd5 100644 --- a/drivers/usb/typec/mux/nb7vpq904m.c +++ b/drivers/usb/typec/mux/nb7vpq904m.c @@ -321,35 +321,37 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) ep = 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; - if (ret < 0) - goto out_error; - - if (ret != DATA_LANES_COUNT) { - dev_err(&nb7->client->dev, "expected 4 data lanes\n"); - ret = -EINVAL; - goto out_error; - } + if (!ep) + return 0; - ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); - if (ret) - goto out_error; + ret = of_property_count_u32_elems(ep, "data-lanes"); + if (ret == -EINVAL) + /* Property isn't here, consider default mapping */ + goto out_done; + if (ret < 0) + goto out_error; + + if (ret != DATA_LANES_COUNT) { + dev_err(&nb7->client->dev, "expected 4 data lanes\n"); + ret = -EINVAL; + goto out_error; + } - for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { - for (j = 0; j < DATA_LANES_COUNT; j++) { - if (data_lanes[j] != supported_data_lane_mapping[i][j]) - break; - } + ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT); + if (ret) + goto out_error; - if (j == DATA_LANES_COUNT) + for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) { + for (j = 0; j < DATA_LANES_COUNT; j++) { + if (data_lanes[j] != supported_data_lane_mapping[i][j]) break; } - switch (i) { + if (j == DATA_LANES_COUNT) + break; + } + + switch (i) { case NORMAL_LANE_MAPPING: break; case INVERT_LANE_MAPPING: @@ -360,7 +362,6 @@ static int nb7vpq904m_parse_data_lanes_mapping(struct nb7vpq904m *nb7) dev_err(&nb7->client->dev, "invalid data lanes mapping\n"); ret = -EINVAL; goto out_error; - } } out_done:
Added check if pointer is null and removed indentation for common path Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: sundar <prosunofficial@gmail.com> --- Fixed nitpicks in code according to comments received on other patch. https://lore.kernel.org/all/2024041103-doornail-professor-7c1e@gregkh/ goal is to get rid of of_node_put,but sending this patch first to do one thing at a time. Changes since v1 - fixed the typo error for spell from identation to indentation v1 patch link - https://lore.kernel.org/all/20240420145522.15018-1-prosunofficial@gmail.com/ drivers/usb/typec/mux/nb7vpq904m.c | 49 +++++++++++++++--------------- 1 file changed, 25 insertions(+), 24 deletions(-)