[PATCH] drm/rockchip: Cope with endpoints that haven't been registered yet

[PATCH] drm/rockchip: Cope with endpoints that haven't been registered yet

[Jonathan McDowell]

The Rockchip RGB CRTC output driver attempts to avoid probing Rockchip
subdrivers to see if they're a connected panel or bridge. However part
of its checks assumes that if no OF platform device is found then it
can't be a valid bridge or panel. This causes issues with I2C controlled
bridges that have not yet been registered to the point they can be
found.

Change this to return EPROBE_DEFER instead of ENODEV and don't ignore
such devices. The subsequent call to drm_of_find_panel_or_bridge() will
return EPROBE_DEFER as well if there's actually a valid device we should
wait for.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++--
 drivers/gpu/drm/rockchip/rockchip_rgb.c     | 7 ++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 212bd87c0c4a..b0d63a566501 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -270,11 +270,15 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)
 	if (!node)
 		return -ENODEV;
 
-	/* status disabled will prevent creation of platform-devices */
+	/*
+	 * status disabled will prevent creation of platform-devices,
+	 * but equally we can't rely on the driver having been registered
+	 * yet (e.g. I2C bridges).
+	 */
 	pdev = of_find_device_by_node(node);
 	of_node_put(node);
 	if (!pdev)
-		return -ENODEV;
+		return -EPROBE_DEFER;
 
 	/*
 	 * All rockchip subdrivers have probed at this point, so
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index c079714477d8..989595087397 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -77,7 +77,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 	struct drm_encoder *encoder;
 	struct device_node *port, *endpoint;
 	u32 endpoint_id;
-	int ret = 0, child_count = 0;
+	int subret, ret = 0, child_count = 0;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 
@@ -96,8 +96,9 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 		if (of_property_read_u32(endpoint, "reg", &endpoint_id))
 			endpoint_id = 0;
 
-		/* if subdriver (> 0) or error case (< 0), ignore entry */
-		if (rockchip_drm_endpoint_is_subdriver(endpoint) != 0)
+		/* if subdriver (> 0) or non-defer error case (< 0), ignore entry */
+		subret = rockchip_drm_endpoint_is_subdriver(endpoint);
+		if (subret != 0 && subret != -EPROBE_DEFER)
 			continue;
 
 		child_count++;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/rockchip: Cope with endpoints that haven't been registered yet

[Heiko Stuebner]

Hi Jonathan,

Am Dienstag, 16. März 2021, 19:27:53 CET schrieb Jonathan McDowell:
> The Rockchip RGB CRTC output driver attempts to avoid probing Rockchip
> subdrivers to see if they're a connected panel or bridge. However part
> of its checks assumes that if no OF platform device is found then it
> can't be a valid bridge or panel. This causes issues with I2C controlled
> bridges that have not yet been registered to the point they can be
> found.
> 
> Change this to return EPROBE_DEFER instead of ENODEV and don't ignore
> such devices. The subsequent call to drm_of_find_panel_or_bridge() will
> return EPROBE_DEFER as well if there's actually a valid device we should
> wait for.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++--
>  drivers/gpu/drm/rockchip/rockchip_rgb.c     | 7 ++++---
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 212bd87c0c4a..b0d63a566501 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -270,11 +270,15 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)
>  	if (!node)
>  		return -ENODEV;
>  
> -	/* status disabled will prevent creation of platform-devices */
> +	/*
> +	 * status disabled will prevent creation of platform-devices,
> +	 * but equally we can't rely on the driver having been registered
> +	 * yet (e.g. I2C bridges).
> +	 */
>  	pdev = of_find_device_by_node(node);
>  	of_node_put(node);
>  	if (!pdev)
> -		return -ENODEV;
> +		return -EPROBE_DEFER;

In general, how does that relate to i2c-bridge-drivers, as
of_find_device_by_node supposedly only acts on platform-devices?

Also if that points to a disabled bridge (hdmi, etc) that would likely make
it probe-defer indefinitly, as that device will never become available?

Maybe we could do something like of_device_is_available() which checks
the status property of the node. So something like:

  	pdev = of_find_device_by_node(node);
  	if (!pdev) {
		bool avail = of_device_is_available(node);

		of_node_put(node);

		/* if disabled
		if (!avail)
			return -ENODEV;
		else
			return -EPROBE_DEFER;
	}
  	of_node_put(node);

Though I still do not understand how that should actually pick up on
i2c devices at all.


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/rockchip: Cope with endpoints that haven't been registered yet

[Jonathan McDowell]

On Sun, Mar 21, 2021 at 07:58:13PM +0100, Heiko Stuebner wrote:
> Am Dienstag, 16. März 2021, 19:27:53 CET schrieb Jonathan McDowell:
> > The Rockchip RGB CRTC output driver attempts to avoid probing Rockchip
> > subdrivers to see if they're a connected panel or bridge. However part
> > of its checks assumes that if no OF platform device is found then it
> > can't be a valid bridge or panel. This causes issues with I2C controlled
> > bridges that have not yet been registered to the point they can be
> > found.
> > 
> > Change this to return EPROBE_DEFER instead of ENODEV and don't ignore
> > such devices. The subsequent call to drm_of_find_panel_or_bridge() will
> > return EPROBE_DEFER as well if there's actually a valid device we should
> > wait for.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++--
> >  drivers/gpu/drm/rockchip/rockchip_rgb.c     | 7 ++++---
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > index 212bd87c0c4a..b0d63a566501 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> > @@ -270,11 +270,15 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)
> >  	if (!node)
> >  		return -ENODEV;
> >  
> > -	/* status disabled will prevent creation of platform-devices */
> > +	/*
> > +	 * status disabled will prevent creation of platform-devices,
> > +	 * but equally we can't rely on the driver having been registered
> > +	 * yet (e.g. I2C bridges).
> > +	 */
> >  	pdev = of_find_device_by_node(node);
> >  	of_node_put(node);
> >  	if (!pdev)
> > -		return -ENODEV;
> > +		return -EPROBE_DEFER;
> 
> In general, how does that relate to i2c-bridge-drivers, as
> of_find_device_by_node supposedly only acts on platform-devices?

I think the problem here is that not finding the device node means we
return an error here, which means it's not actually possible to attach
an i2c bridge driver to the Rockchip RGB interface at present.

> Also if that points to a disabled bridge (hdmi, etc) that would likely make
> it probe-defer indefinitly, as that device will never become available?
> 
> Maybe we could do something like of_device_is_available() which checks
> the status property of the node. So something like:
> 
>   	pdev = of_find_device_by_node(node);
>   	if (!pdev) {
> 		bool avail = of_device_is_available(node);
> 
> 		of_node_put(node);
> 
> 		/* if disabled
> 		if (!avail)
> 			return -ENODEV;
> 		else
> 			return -EPROBE_DEFER;
> 	}
>   	of_node_put(node);
> 
> Though I still do not understand how that should actually pick up on
> i2c devices at all.

of_find_device_by_node will fail here, as it's not a platform device,
but then of_device_is_available should return true so I think I can
actually just return false here rather than EPROBE_DEFER - because if
it's not a platform device then it's not a subdriver, which is what
we're checking for.

I'll re-roll and test this weekend and post an updated revision. Thanks
for the pointers.

J.

-- 
101 things you can't have too much of : 3 - Sleep.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel