Added check if pointer is null and removed indentation for common path
Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: sundar <[email protected]>
---
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/[email protected]/
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:
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 Sat, 20 Apr 2024 at 19:49, sundar <[email protected]> wrote:
>
> Added check if pointer is null and removed indentation for common path
>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: sundar <[email protected]>
> ---
>
> 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/[email protected]/
>
> 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
>
--
With best wishes
Dmitry
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 <[email protected]> wrote:
>>
>> Added check if pointer is null and removed indentation for common path
>>
>> Suggested-by: Greg Kroah-Hartman <[email protected]>
>> Signed-off-by: sundar <[email protected]>
>> ---
>>
>> 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/[email protected]/
>>
>> 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.