| Message ID | 20231126163247.10131-2-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] dt-bindings: tc358775: Add support for tc358765 | expand |
> The current code assume hardcoded dsi host endpoint 1, which may not > be the case. Let's fix that and simplify the code by getting the dsi > endpoint with of_graph_get_remote_endpoint() that does not assume any > endpoint numbering. > > Fixes: b26975593b17 ("display/drm/bridge: TC358775 DSI/LVDS driver") > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/bridge/tc358775.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c > --- a/drivers/gpu/drm/bridge/tc358775.c > +++ b/drivers/gpu/drm/bridge/tc358775.c > @@ -528,25 +528,17 @@ tc_mode_valid(struct drm_bridge *bridge, > static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc) > { > struct device_node *endpoint; > - struct device_node *parent; > struct device_node *remote; > int dsi_lanes = -1; > > - /* > - * To get the data-lanes of dsi, we need to access the dsi0_out of port1 > - * of dsi0 endpoint from bridge port0 of d2l_in > - */ > endpoint = of_graph_get_endpoint_by_regs(tc->dev->of_node, > TC358775_DSI_IN, -1); > if (endpoint) { > - /* dsi0_out node */ > - parent = of_graph_get_remote_port_parent(endpoint); > + /* Get the configured data lanes on the dsi host side */ > + remote = of_graph_get_remote_endpoint(endpoint); > of_node_put(endpoint); > - if (parent) { > - /* dsi0 port 1 */ > - dsi_lanes = drm_of_get_data_lanes_count_ep(parent, 1, -1, 1, 4); > - of_node_put(parent); > - } > + dsi_lanes = drm_of_get_data_lanes_count(remote, 1, 4); I actually have the same fix, but with one additional detail, which I'm unsure about though: This looks at the data-lanes property of the *remote* endpoint whereas other bridge drivers (see tc358767, ti-sn65dsi83, lt8912b, anx7625) look at the local endpoint and I'm not sure what is correct. -michael
* Michael Walle <mwalle@kernel.org> [231127 13:10]: > I actually have the same fix, but with one additional detail, which I'm > unsure about though: This looks at the data-lanes property of the *remote* > endpoint whereas other bridge drivers (see tc358767, ti-sn65dsi83, lt8912b, > anx7625) look at the local endpoint and I'm not sure what is correct. Yes I've been wondering about that too. Let's just move it over to the bridge node? We could produce a warning if the dsi host node has the data-lanes property.. No current in kernel users AFAIK. FYI, for omapdrm, we already have a legacy dt property "lanes" for the wiring that tells number of lanes used and the order of the lanes. Regards, Tony
+ dt maintainers >> I actually have the same fix, but with one additional detail, which >> I'm >> unsure about though: This looks at the data-lanes property of the >> *remote* >> endpoint whereas other bridge drivers (see tc358767, ti-sn65dsi83, >> lt8912b, >> anx7625) look at the local endpoint and I'm not sure what is correct. > > Yes I've been wondering about that too. Let's just move it over to the > bridge node? We could produce a warning if the dsi host node has the > data-lanes property.. No current in kernel users AFAIK. I haven't found any in-tree users either. In my patch, I first try the remote end and then the local end. But thinking more about it I don't think this is correct. Maybe we can do it the other way around, first try data-lanes of the local endpoint and if not found, then try the remote one. That way, we would at least be backwards compatible in the driver. And for the dt-bindings, make it mandatory to have a local data-lanes. -michael > FYI, for omapdrm, we already have a legacy dt property "lanes" for the > wiring that tells number of lanes used and the order of the lanes. > > Regards, > > Tony
* Michael Walle <mwalle@kernel.org> [231127 14:31]: > + dt maintainers > > > > I actually have the same fix, but with one additional detail, which > > > I'm > > > unsure about though: This looks at the data-lanes property of the > > > *remote* > > > endpoint whereas other bridge drivers (see tc358767, ti-sn65dsi83, > > > lt8912b, > > > anx7625) look at the local endpoint and I'm not sure what is correct. > > > > Yes I've been wondering about that too. Let's just move it over to the > > bridge node? We could produce a warning if the dsi host node has the > > data-lanes property.. No current in kernel users AFAIK. > > I haven't found any in-tree users either. In my patch, I first try the > remote > end and then the local end. But thinking more about it I don't think > this is correct. Maybe we can do it the other way around, first try > data-lanes of the local endpoint and if not found, then try the remote > one. That way, we would at least be backwards compatible in the driver. > And for the dt-bindings, make it mandatory to have a local data-lanes. OK sounds good to me. Tony
diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c --- a/drivers/gpu/drm/bridge/tc358775.c +++ b/drivers/gpu/drm/bridge/tc358775.c @@ -528,25 +528,17 @@ tc_mode_valid(struct drm_bridge *bridge, static int tc358775_parse_dt(struct device_node *np, struct tc_data *tc) { struct device_node *endpoint; - struct device_node *parent; struct device_node *remote; int dsi_lanes = -1; - /* - * To get the data-lanes of dsi, we need to access the dsi0_out of port1 - * of dsi0 endpoint from bridge port0 of d2l_in - */ endpoint = of_graph_get_endpoint_by_regs(tc->dev->of_node, TC358775_DSI_IN, -1); if (endpoint) { - /* dsi0_out node */ - parent = of_graph_get_remote_port_parent(endpoint); + /* Get the configured data lanes on the dsi host side */ + remote = of_graph_get_remote_endpoint(endpoint); of_node_put(endpoint); - if (parent) { - /* dsi0 port 1 */ - dsi_lanes = drm_of_get_data_lanes_count_ep(parent, 1, -1, 1, 4); - of_node_put(parent); - } + dsi_lanes = drm_of_get_data_lanes_count(remote, 1, 4); + of_node_put(remote); } if (dsi_lanes < 0)
The current code assume hardcoded dsi host endpoint 1, which may not be the case. Let's fix that and simplify the code by getting the dsi endpoint with of_graph_get_remote_endpoint() that does not assume any endpoint numbering. Fixes: b26975593b17 ("display/drm/bridge: TC358775 DSI/LVDS driver") Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/gpu/drm/bridge/tc358775.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)