Re: [PATCH v11 1/9] device property: Add remote endpoint to devcon matcher

[Sakari Ailus]

Hi Pin-yen,

On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani <pmalani@xxxxxxxxxxxx>
> 
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
> 
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
> 
> Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
> Reviewed-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> Tested-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>

Thanks for the update.

I intended to give my Reviewed-by: but there's something still needs to be
addressed. See below.

> 
> ---
> 
> Changes in v11:
> - Added missing fwnode_handle_put in drivers/base/property.c
> 
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
> 
> Changes in v6:
> - New in v6
> 
>  drivers/base/property.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..e6f915b72eb7 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
>  			break;
>  		}
>  
> +		/*
> +		 * Some drivers may register devices for endpoints. Check
> +		 * the remote-endpoints for matches in addition to the remote
> +		 * port parent.
> +		 */
> +		node = fwnode_graph_get_remote_endpoint(ep);

Here fwnode_graph_get_remote_endpoint() returns an endpoint...

> +		if (fwnode_device_is_available(node)) {

and you're calling fwnode_device_is_available() on the endpoint node, which
always returns true.

Shouldn't you call this on the device node instead? What about match()
below?

> +			ret = match(node, con_id, data);
> +			if (ret) {
> +				if (matches)
> +					matches[count] = ret;
> +				count++;
> +			}
> +		}
> +		fwnode_handle_put(node);
> +
>  		node = fwnode_graph_get_remote_port_parent(ep);
>  		if (!fwnode_device_is_available(node)) {
>  			fwnode_handle_put(node);

-- 
Kind regards,

Sakari Ailus