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 <[email protected]>
Signed-off-by: R Sundar <[email protected]>
---
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;
}
--
2.34.1
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 <[email protected]>
> Signed-off-by: R Sundar <[email protected]>
> ---
> 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 <[email protected]>
>> Signed-off-by: R Sundar <[email protected]>
>> ---
>> 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 <[email protected]>
> > > Signed-off-by: R Sundar <[email protected]>
> > > ---
> > > 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 <[email protected]>
>>>> Signed-off-by: R Sundar <[email protected]>
>>>> ---
>>>> 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