| Message ID | 20240429171543.13032-1-prosunofficial@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [linux-next] media:cdns-csi2tx: replace of_node_put() with __free | expand |
Le 29/04/2024 à 19:15, R Sundar a écrit : > Use the new cleanup magic to replace of_node_put() with > __free(device_node) marking to auto release when they get out of scope. > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > Signed-off-by: R Sundar <prosunofficial@gmail.com> > --- > drivers/media/platform/cadence/cdns-csi2tx.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c b/drivers/media/platform/cadence/cdns-csi2tx.c > index 3d98f91f1bee..88aed2f299fd 100644 > --- a/drivers/media/platform/cadence/cdns-csi2tx.c > +++ b/drivers/media/platform/cadence/cdns-csi2tx.c > @@ -496,48 +496,43 @@ static int csi2tx_get_resources(struct csi2tx_priv *csi2tx, > static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx) > { > struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 }; > - struct device_node *ep; > int ret, i; > - > - ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); > + struct device_node *ep __free(device_node) = > + of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); > + > if (!ep) > return -EINVAL; > > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > if (ret) { > dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n"); > - goto out; > + return ret; > } > > if (v4l2_ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n", > v4l2_ep.bus_type); > - ret = -EINVAL; > - goto out; > + return -EINVAL; > } > > csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; > if (csi2tx->num_lanes > csi2tx->max_lanes) { > dev_err(csi2tx->dev, > "Current configuration uses more lanes than supported\n"); > - ret = -EINVAL; > - goto out; > + return -EINVAL; > } > > for (i = 0; i < csi2tx->num_lanes; i++) { > if (v4l2_ep.bus.mipi_csi2.data_lanes[i] < 1) { > dev_err(csi2tx->dev, "Invalid lane[%d] number: %u\n", > i, v4l2_ep.bus.mipi_csi2.data_lanes[i]); > - ret = -EINVAL; > - goto out; > + return -EINVAL; > } > } > > memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes, > sizeof(csi2tx->lanes)); > > -out: > - of_node_put(ep); > return ret; Hi, Nit: return 0; ? CJ > } >
On 30/04/24 00:10, Christophe JAILLET wrote: > Le 29/04/2024 à 19:15, R Sundar a écrit : >> Use the new cleanup magic to replace of_node_put() with >> __free(device_node) marking to auto release when they get out of scope. >> >> Suggested-by: Julia Lawall <julia.lawall@inria.fr> >> Signed-off-by: R Sundar <prosunofficial@gmail.com> >> --- >> drivers/media/platform/cadence/cdns-csi2tx.c | 19 +++++++------------ >> 1 file changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c >> b/drivers/media/platform/cadence/cdns-csi2tx.c >> index 3d98f91f1bee..88aed2f299fd 100644 >> --- a/drivers/media/platform/cadence/cdns-csi2tx.c >> +++ b/drivers/media/platform/cadence/cdns-csi2tx.c >> @@ -496,48 +496,43 @@ static int csi2tx_get_resources(struct >> csi2tx_priv *csi2tx, >> static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx) >> { >> struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 }; >> - struct device_node *ep; >> int ret, i; >> - >> - ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); >> + struct device_node *ep __free(device_node) = >> + of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); >> + >> if (!ep) >> return -EINVAL; >> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); >> if (ret) { >> dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n"); >> - goto out; >> + return ret; >> } >> if (v4l2_ep.bus_type != V4L2_MBUS_CSI2_DPHY) { >> dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n", >> v4l2_ep.bus_type); >> - ret = -EINVAL; >> - goto out; >> + return -EINVAL; >> } >> csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; >> if (csi2tx->num_lanes > csi2tx->max_lanes) { >> dev_err(csi2tx->dev, >> "Current configuration uses more lanes than supported\n"); >> - ret = -EINVAL; >> - goto out; >> + return -EINVAL; >> } >> for (i = 0; i < csi2tx->num_lanes; i++) { >> if (v4l2_ep.bus.mipi_csi2.data_lanes[i] < 1) { >> dev_err(csi2tx->dev, "Invalid lane[%d] number: %u\n", >> i, v4l2_ep.bus.mipi_csi2.data_lanes[i]); >> - ret = -EINVAL; >> - goto out; >> + return -EINVAL; >> } >> } >> memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes, >> sizeof(csi2tx->lanes)); >> -out: >> - of_node_put(ep); >> return ret; > > Hi, > > Nit: return 0; ? > > CJ > >> } > Hi, In success case, ret variable value also will be zero, else for non-zero ret value it will return from v4l2_fwnode_endpoint_parse()'s error case handling block. Thanks, Sundar
On Tue, 30 Apr 2024, R Sundar wrote: > On 30/04/24 00:10, Christophe JAILLET wrote: > > Le 29/04/2024 à 19:15, R Sundar a écrit : > > > Use the new cleanup magic to replace of_node_put() with > > > __free(device_node) marking to auto release when they get out of scope. > > > > > > Suggested-by: Julia Lawall <julia.lawall@inria.fr> > > > Signed-off-by: R Sundar <prosunofficial@gmail.com> > > > --- > > > drivers/media/platform/cadence/cdns-csi2tx.c | 19 +++++++------------ > > > 1 file changed, 7 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c > > > b/drivers/media/platform/cadence/cdns-csi2tx.c > > > index 3d98f91f1bee..88aed2f299fd 100644 > > > --- a/drivers/media/platform/cadence/cdns-csi2tx.c > > > +++ b/drivers/media/platform/cadence/cdns-csi2tx.c > > > @@ -496,48 +496,43 @@ static int csi2tx_get_resources(struct csi2tx_priv > > > *csi2tx, > > > static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx) > > > { > > > struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 }; > > > - struct device_node *ep; > > > int ret, i; > > > - > > > - ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); > > > + struct device_node *ep __free(device_node) = > > > + of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); > > > + > > > if (!ep) > > > return -EINVAL; > > > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > > > if (ret) { > > > dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n"); > > > - goto out; > > > + return ret; > > > } > > > if (v4l2_ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > > > dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n", > > > v4l2_ep.bus_type); > > > - ret = -EINVAL; > > > - goto out; > > > + return -EINVAL; > > > } > > > csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; > > > if (csi2tx->num_lanes > csi2tx->max_lanes) { > > > dev_err(csi2tx->dev, > > > "Current configuration uses more lanes than supported\n"); > > > - ret = -EINVAL; > > > - goto out; > > > + return -EINVAL; > > > } > > > for (i = 0; i < csi2tx->num_lanes; i++) { > > > if (v4l2_ep.bus.mipi_csi2.data_lanes[i] < 1) { > > > dev_err(csi2tx->dev, "Invalid lane[%d] number: %u\n", > > > i, v4l2_ep.bus.mipi_csi2.data_lanes[i]); > > > - ret = -EINVAL; > > > - goto out; > > > + return -EINVAL; > > > } > > > } > > > memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes, > > > sizeof(csi2tx->lanes)); > > > -out: > > > - of_node_put(ep); > > > return ret; > > > > Hi, > > > > Nit: return 0; ? > > > > CJ > > > > > } > > > Hi, > > In success case, ret variable value also will be zero, else for non-zero ret > value it will return from v4l2_fwnode_endpoint_parse()'s error case handling > block. Indeed, but it seems that the return ret at the end of the function always returns 0? If that is the case, return 0 would be better, as one can see that that code is only reached in the success case. julia
On 30/04/24 22:53, Julia Lawall wrote: > > > On Tue, 30 Apr 2024, R Sundar wrote: > >> On 30/04/24 00:10, Christophe JAILLET wrote: >>> Le 29/04/2024 à 19:15, R Sundar a écrit : >>>> Use the new cleanup magic to replace of_node_put() with >>>> __free(device_node) marking to auto release when they get out of scope. >>>> >>>> Suggested-by: Julia Lawall <julia.lawall@inria.fr> >>>> Signed-off-by: R Sundar <prosunofficial@gmail.com> >>>> --- >>>> drivers/media/platform/cadence/cdns-csi2tx.c | 19 +++++++------------ >>>> 1 file changed, 7 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c >>>> b/drivers/media/platform/cadence/cdns-csi2tx.c >>>> index 3d98f91f1bee..88aed2f299fd 100644 >>>> --- a/drivers/media/platform/cadence/cdns-csi2tx.c >>>> +++ b/drivers/media/platform/cadence/cdns-csi2tx.c >>>> @@ -496,48 +496,43 @@ static int csi2tx_get_resources(struct csi2tx_priv >>>> *csi2tx, >>>> static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx) >>>> { >>>> struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 }; >>>> - struct device_node *ep; >>>> int ret, i; >>>> - >>>> - ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); >>>> + struct device_node *ep __free(device_node) = >>>> + of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); >>>> + >>>> if (!ep) >>>> return -EINVAL; >>>> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); >>>> if (ret) { >>>> dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n"); >>>> - goto out; >>>> + return ret; >>>> } >>>> if (v4l2_ep.bus_type != V4L2_MBUS_CSI2_DPHY) { >>>> dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n", >>>> v4l2_ep.bus_type); >>>> - ret = -EINVAL; >>>> - goto out; >>>> + return -EINVAL; >>>> } >>>> csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; >>>> if (csi2tx->num_lanes > csi2tx->max_lanes) { >>>> dev_err(csi2tx->dev, >>>> "Current configuration uses more lanes than supported\n"); >>>> - ret = -EINVAL; >>>> - goto out; >>>> + return -EINVAL; >>>> } >>>> for (i = 0; i < csi2tx->num_lanes; i++) { >>>> if (v4l2_ep.bus.mipi_csi2.data_lanes[i] < 1) { >>>> dev_err(csi2tx->dev, "Invalid lane[%d] number: %u\n", >>>> i, v4l2_ep.bus.mipi_csi2.data_lanes[i]); >>>> - ret = -EINVAL; >>>> - goto out; >>>> + return -EINVAL; >>>> } >>>> } >>>> memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes, >>>> sizeof(csi2tx->lanes)); >>>> -out: >>>> - of_node_put(ep); >>>> return ret; >>> >>> Hi, >>> >>> Nit: return 0; ? >>> >>> CJ >>> >>>> } >>> >> Hi, >> >> In success case, ret variable value also will be zero, else for non-zero ret >> value it will return from v4l2_fwnode_endpoint_parse()'s error case handling >> block. > > Indeed, but it seems that the return ret at the end of the function always > returns 0? If that is the case, return 0 would be better, as one can see > that that code is only reached in the success case. > > julia Hi Julia, Noted. @CJ, Thanks for comments. Understood the point of Nit. Will update the changes. Thanks, Sundar
diff --git a/drivers/media/platform/cadence/cdns-csi2tx.c b/drivers/media/platform/cadence/cdns-csi2tx.c index 3d98f91f1bee..88aed2f299fd 100644 --- a/drivers/media/platform/cadence/cdns-csi2tx.c +++ b/drivers/media/platform/cadence/cdns-csi2tx.c @@ -496,48 +496,43 @@ static int csi2tx_get_resources(struct csi2tx_priv *csi2tx, static int csi2tx_check_lanes(struct csi2tx_priv *csi2tx) { struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 }; - struct device_node *ep; int ret, i; - - ep = of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); + struct device_node *ep __free(device_node) = + of_graph_get_endpoint_by_regs(csi2tx->dev->of_node, 0, 0); + if (!ep) return -EINVAL; ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); if (ret) { dev_err(csi2tx->dev, "Could not parse v4l2 endpoint\n"); - goto out; + return ret; } if (v4l2_ep.bus_type != V4L2_MBUS_CSI2_DPHY) { dev_err(csi2tx->dev, "Unsupported media bus type: 0x%x\n", v4l2_ep.bus_type); - ret = -EINVAL; - goto out; + return -EINVAL; } csi2tx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; if (csi2tx->num_lanes > csi2tx->max_lanes) { dev_err(csi2tx->dev, "Current configuration uses more lanes than supported\n"); - ret = -EINVAL; - goto out; + return -EINVAL; } for (i = 0; i < csi2tx->num_lanes; i++) { if (v4l2_ep.bus.mipi_csi2.data_lanes[i] < 1) { dev_err(csi2tx->dev, "Invalid lane[%d] number: %u\n", i, v4l2_ep.bus.mipi_csi2.data_lanes[i]); - ret = -EINVAL; - goto out; + return -EINVAL; } } memcpy(csi2tx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes, sizeof(csi2tx->lanes)); -out: - of_node_put(ep); return ret; }
Use the new cleanup magic to replace of_node_put() with __free(device_node) marking to auto release when they get out of scope. Suggested-by: Julia Lawall <julia.lawall@inria.fr> Signed-off-by: R Sundar <prosunofficial@gmail.com> --- drivers/media/platform/cadence/cdns-csi2tx.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)