Re: [RFC 04/12] staging: media: max96712: change DT parsing routine

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Jan 31, 2025 at 06:33:58PM +0200, Laurentiu Palcu wrote:
> -static int max96712_parse_dt(struct max96712_priv *priv)
> +static int max96712_parse_rx_ports(struct max96712_priv *priv, struct device_node *node,
> +				   struct of_endpoint *ep)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max96712_rx_port *source;
> +	struct fwnode_handle *remote_port_parent;
> +
> +	if (priv->rx_ports[ep->port].fwnode) {
> +		dev_info(dev, "Multiple port endpoints are not supported: %d", ep->port);
> +		return 0;
> +	}
> +
> +	source = &priv->rx_ports[ep->port];
> +	source->fwnode = fwnode_graph_get_remote_endpoint(of_fwnode_handle(node));
> +	if (!source->fwnode) {
> +		dev_info(dev, "Endpoint %pOF has no remote endpoint connection\n", ep->local_node);
> +		return 0;
> +	}
> +
> +	remote_port_parent = fwnode_graph_get_remote_port_parent(of_fwnode_handle(node));
> +
> +	if (!fwnode_device_is_available(remote_port_parent)) {
> +		dev_dbg(dev, "Skipping port %d as remote port parent is disabled.\n",
> +			ep->port);
> +		source->fwnode = NULL;

I don't understand the refcounting in this function.  Should we call
fwnode_handle_put(source->fwnode); before setting this to NULL?

> +		goto fwnode_put;
> +	}
> +
> +	priv->rx_port_mask |= BIT(ep->port);
> +	priv->n_rx_ports++;
> +
> +fwnode_put:
> +	fwnode_handle_put(remote_port_parent);
> +	fwnode_handle_put(source->fwnode);
> +	return 0;

I don't understand why we save source->fwnode but drop the refcount on
the success path.  My instinct says that it should be a source_fwnode
local stack variable instead.  (But again, I've only glanced at this
code so I could be wrong).


> +}
> +

regards,
dan carpenter