Re: [PATCH 7/9] media: qcom: camss: change internals of endpoint parsing to fwnode handling

[neil.armstrong@linaro.org]

On 13/05/2025 16:23, Vladimir Zapolskiy wrote:
> Since a few called V4L2 functions operate with fwnode arguments the change
> from OF device nodes to fwnodes brings a simplification to the code.
> 
> Because camss_probe() as the single caller of camss_of_parse_endpoint_node()
> has no need to know a number of async registered remote devices, it makes
> sense to remove the related computation from it. In addition there is no
> reason to check for a OF device availability on CAMSS side, the check is
> useless as the always passed one.

I think you should explain why it's useless, TBH I'm not even sure why it's
not necessary. What if we set the remote endpoint as disabled, this is
a regression, no ?

Why not replace it with  fwnode_device_is_available() and remove it in another
patch really explaining why it's useless ?

Neil

> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 52 ++++++++++-------------
>   1 file changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 39c5472f4552..d4745fb21152 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2973,16 +2973,16 @@ static const struct parent_dev_ops vfe_parent_dev_ops = {
>   };
>   
>   /*
> - * camss_of_parse_endpoint_node - Parse port endpoint node
> - * @dev: Device
> - * @node: Device node to be parsed
> + * camss_parse_endpoint_node - Parse port endpoint node
> + * @dev: CAMSS device
> + * @ep: Device endpoint to be parsed
>    * @csd: Parsed data from port endpoint node
>    *
>    * Return 0 on success or a negative error code on failure
>    */
> -static int camss_of_parse_endpoint_node(struct device *dev,
> -					struct device_node *node,
> -					struct camss_async_subdev *csd)
> +static int camss_parse_endpoint_node(struct device *dev,
> +				     struct fwnode_handle *ep,
> +				     struct camss_async_subdev *csd)
>   {
>   	struct csiphy_lanes_cfg *lncfg = &csd->interface.csi2.lane_cfg;
>   	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2;
> @@ -2990,7 +2990,7 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>   	unsigned int i;
>   	int ret;
>   
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
> +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
>   	if (ret)
>   		return ret;
>   
> @@ -3025,52 +3025,46 @@ static int camss_of_parse_endpoint_node(struct device *dev,
>   }
>   
>   /*
> - * camss_of_parse_ports - Parse ports node
> - * @dev: Device
> - * @notifier: v4l2_device notifier data
> + * camss_parse_ports - Parse ports node
> + * @dev: CAMSS device
>    *
> - * Return number of "port" nodes found in "ports" node
> + * Return 0 on success or a negative error code on failure
>    */
> -static int camss_of_parse_ports(struct camss *camss)
> +static int camss_parse_ports(struct camss *camss)
>   {
>   	struct device *dev = camss->dev;
> -	struct device_node *node = NULL;
> -	struct device_node *remote = NULL;
> -	int ret, num_subdevs = 0;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
> +	int ret;
>   
> -	for_each_endpoint_of_node(dev->of_node, node) {
> +	fwnode_graph_for_each_endpoint(fwnode, ep) {
>   		struct camss_async_subdev *csd;
> +		struct fwnode_handle *remote;
>   
> -		if (!of_device_is_available(node))
> -			continue;
> -
> -		remote = of_graph_get_remote_port_parent(node);
> +		remote = fwnode_graph_get_remote_port_parent(ep);
>   		if (!remote) {
>   			dev_err(dev, "Cannot get remote parent\n");
>   			ret = -EINVAL;
>   			goto err_cleanup;
>   		}
>   
> -		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
> -					       of_fwnode_handle(remote),
> +		csd = v4l2_async_nf_add_fwnode(&camss->notifier, remote,
>   					       struct camss_async_subdev);
> -		of_node_put(remote);
> +		fwnode_handle_put(remote);
>   		if (IS_ERR(csd)) {
>   			ret = PTR_ERR(csd);
>   			goto err_cleanup;
>   		}
>   
> -		ret = camss_of_parse_endpoint_node(dev, node, csd);
> +		ret = camss_parse_endpoint_node(dev, ep, csd);
>   		if (ret < 0)
>   			goto err_cleanup;
> -
> -		num_subdevs++;
>   	}
>   
> -	return num_subdevs;
> +	return 0;
>   
>   err_cleanup:
> -	of_node_put(node);
> +	fwnode_handle_put(ep);
> +
>   	return ret;
>   }
>   
> @@ -3626,7 +3620,7 @@ static int camss_probe(struct platform_device *pdev)
>   
>   	pm_runtime_enable(dev);
>   
> -	ret = camss_of_parse_ports(camss);
> +	ret = camss_parse_ports(camss);
>   	if (ret < 0)
>   		goto err_v4l2_device_unregister;
>