drm/rockchip: Cope with endpoints that haven't been registered yet
Subsystems (4)
PATCH 1/1 drm/rockchip: Cope with endpoints that haven't been registered yet
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.
--- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++-- drivers/gpu/drm/rockchip/rockchip_rgb.c | 7 ++++--- 2 files changed, 10 insertions(+), 5 deletions(-)
drivers/gpu/drm/rockchip/rockchip_drm_drv.c +6 -2
@@ -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
drivers/gpu/drm/rockchip/rockchip_rgb.c +4 -3
@@ -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++;
Signature · 7 lines
2.20.1 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Heiko Stuebner
maintainerHi Jonathan,
Am Dienstag, 16. März 2021, 19:27:53 CET schrieb Jonathan McDowell:
Quoted 27 lines
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_DEFERinstead of ENODEV and don't ignore such devices. The subsequent call todrm_of_find_panel_or_bridge()will returnEPROBE_DEFERas well if there's actually a valid device we should wait for.Signed-off-by Jonathan McDowell <[email protected]>--- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++-- drivers/gpu/drm/rockchip/rockchip_rgb.c | 7 ++++--- 2 files changed, 10 insertions(+), 5 deletions(-)drivers/gpu/drm/rockchip/rockchip_drm_drv.c +0
-1@@ -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 */
drivers/gpu/drm/rockchip/rockchip_drm_drv.c int rockchip_drm_endpoint_is_subdriver(struct device_node *ep) · line 281
Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_drv.c +6
-1+/* +* 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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 27 lines · Heiko Stuebner
Am Dienstag, 16. März 2021, 19:27:53 CET schrieb Jonathan McDowell:
Quoted 26 lines
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_DEFERinstead of ENODEV and don't ignore such devices. The subsequent call todrm_of_find_panel_or_bridge()will returnEPROBE_DEFERas well if there's actually a valid device we should wait for.Signed-off-by Jonathan McDowell <[email protected]>--- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 8 ++++++-- drivers/gpu/drm/rockchip/rockchip_rgb.c | 7 ++++--- 2 files changed, 10 insertions(+), 5 deletions(-)drivers/gpu/drm/rockchip/rockchip_drm_drv.c +0
-0@@ -270,11 +270,15 @@ int rockchip_drm_endpoint_is_subdriver(struct device_node *ep)if (!node)return -ENODEV ;
Quoted 14 lines
drivers/gpu/drm/rockchip/rockchip_drm_drv.c int rockchip_drm_endpoint_is_subdriver(struct device_node *ep) · line 281
Quoted 11 lines
drivers/gpu/drm/rockchip/rockchip_drm_drv.c +6
-2-/* 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_nodesupposedly 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.
Quoted 18 lines
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; }
Quoted 4 lines
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.
Signature · 5 lines
101 things you can't have too much of : 3 - Sleep. _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel