| Message ID | 20210316182753.GA25685@earth.li (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: Cope with endpoints that haven't been registered yet | expand |
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
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.
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++;
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(-)