On 2/6/24 14:41, Laurent Pinchart wrote: > Hi Morimoto-san, > > (Adding Krzysztof Hałasa) > > Thank you for the patch. > > On Tue, Feb 06, 2024 at 02:55:27AM +0000, Kuninori Morimoto wrote: >> From DT point of view, in general, drivers should be asking for a >> specific port number because their function is fixed in the binding. >> >> of_graph_get_next_endpoint() doesn't match to this concept. >> >> Simply replace >> >> - of_graph_get_next_endpoint(xxx, NULL); >> + of_graph_get_endpoint_by_regs(xxx, 0, -1); >> >> Link: https://lore.kernel.org/r/20240202174941.GA310089-robh@xxxxxxxxxx >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> >> --- >> drivers/media/i2c/adv7343.c | 2 +- >> drivers/media/i2c/adv7604.c | 2 +- >> drivers/media/i2c/mt9p031.c | 2 +- >> drivers/media/i2c/mt9v032.c | 2 +- >> drivers/media/i2c/ov2659.c | 2 +- >> drivers/media/i2c/ov5645.c | 2 +- >> drivers/media/i2c/ov5647.c | 2 +- >> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +- >> drivers/media/i2c/s5k5baf.c | 2 +- >> drivers/media/i2c/tc358743.c | 2 +- >> drivers/media/i2c/tda1997x.c | 2 +- >> drivers/media/i2c/tvp514x.c | 2 +- >> drivers/media/i2c/tvp7002.c | 2 +- >> 13 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c >> index ff21cd4744d3..4fbe4e18570e 100644 >> --- a/drivers/media/i2c/adv7343.c >> +++ b/drivers/media/i2c/adv7343.c >> @@ -403,7 +403,7 @@ adv7343_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - np = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + np = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!np) >> return NULL; >> >> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c >> index b202a85fbeaa..dcfdbb975473 100644 >> --- a/drivers/media/i2c/adv7604.c >> +++ b/drivers/media/i2c/adv7604.c >> @@ -3205,7 +3205,7 @@ static int adv76xx_parse_dt(struct adv76xx_state *state) >> np = state->i2c_clients[ADV76XX_PAGE_IO]->dev.of_node; >> >> /* Parse the endpoint. */ >> - endpoint = of_graph_get_next_endpoint(np, NULL); >> + endpoint = of_graph_get_endpoint_by_regs(np, 0, -1); > > I think this should be port 1 for the adv7611 and port2 for the adv7612. > The adv7610 may need to use port 1 too, but the bindings likely need to > be updated. > > Hans, Krzysztof, any opinion ? It looks like it. But I suspect the code never worked. The endpoint parsing is only needed if a specific mbus type is used (i.e., not 'UNKNOWN'), and I don't think that is used in the device trees in the kernel. So everything silently falls back to UNKNOWN and some default bus config that 'just works' (tm). I'm pretty sure this code is wrong, but nobody ever noticed. Changing it to the new code just makes it bug-compatible :-) Ideally someone would have to actually test this, perhaps with one of those Renesas boards. While I do have one, it got bricked after I attempted a u-boot update :-( Regards, Hans > >> if (!endpoint) >> return -EINVAL; >> >> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c >> index 348f1e1098fb..c57a0d436421 100644 >> --- a/drivers/media/i2c/mt9p031.c >> +++ b/drivers/media/i2c/mt9p031.c >> @@ -1080,7 +1080,7 @@ mt9p031_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - np = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + np = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!np) >> return NULL; >> >> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c >> index 1c6f6cea1204..14d277680fa2 100644 >> --- a/drivers/media/i2c/mt9v032.c >> +++ b/drivers/media/i2c/mt9v032.c >> @@ -1008,7 +1008,7 @@ mt9v032_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - np = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + np = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!np) >> return NULL; >> >> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c >> index 2c3dbe164eb6..edea62a02320 100644 >> --- a/drivers/media/i2c/ov2659.c >> +++ b/drivers/media/i2c/ov2659.c >> @@ -1388,7 +1388,7 @@ ov2659_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + endpoint = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!endpoint) >> return NULL; >> >> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c >> index a70db7e601a4..31d214bd4a83 100644 >> --- a/drivers/media/i2c/ov5645.c >> +++ b/drivers/media/i2c/ov5645.c >> @@ -1053,7 +1053,7 @@ static int ov5645_probe(struct i2c_client *client) >> ov5645->i2c_client = client; >> ov5645->dev = dev; >> >> - endpoint = of_graph_get_next_endpoint(dev->of_node, NULL); >> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); >> if (!endpoint) { >> dev_err(dev, "endpoint node not found\n"); >> return -EINVAL; >> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c >> index dcfe3129c63a..beab2813c672 100644 >> --- a/drivers/media/i2c/ov5647.c >> +++ b/drivers/media/i2c/ov5647.c >> @@ -1363,7 +1363,7 @@ static int ov5647_parse_dt(struct ov5647 *sensor, struct device_node *np) >> struct device_node *ep; >> int ret; >> >> - ep = of_graph_get_next_endpoint(np, NULL); >> + ep = of_graph_get_endpoint_by_regs(np, 0, -1); >> if (!ep) >> return -EINVAL; >> >> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> index ed5b10731a14..aaec5f64f31a 100644 >> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c >> @@ -1555,7 +1555,7 @@ static int s5c73m3_get_dt_data(struct s5c73m3 *state) >> "failed to request gpio S5C73M3_RST\n"); >> gpiod_set_consumer_name(state->reset, "S5C73M3_RST"); >> >> - node_ep = of_graph_get_next_endpoint(node, NULL); >> + node_ep = of_graph_get_endpoint_by_regs(node, 0, -1); >> if (!node_ep) { >> dev_warn(dev, "no endpoint defined for node: %pOF\n", node); >> return 0; >> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c >> index 67da2045f543..af7e49e6cc5b 100644 >> --- a/drivers/media/i2c/s5k5baf.c >> +++ b/drivers/media/i2c/s5k5baf.c >> @@ -1836,7 +1836,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev) >> state->mclk_frequency); >> } >> >> - node_ep = of_graph_get_next_endpoint(node, NULL); >> + node_ep = of_graph_get_endpoint_by_regs(node, 0, -1); >> if (!node_ep) { >> dev_err(dev, "no endpoint defined at node %pOF\n", node); >> return -EINVAL; >> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c >> index 2785935da497..872e802cdfbe 100644 >> --- a/drivers/media/i2c/tc358743.c >> +++ b/drivers/media/i2c/tc358743.c >> @@ -1895,7 +1895,7 @@ static int tc358743_probe_of(struct tc358743_state *state) >> return dev_err_probe(dev, PTR_ERR(refclk), >> "failed to get refclk\n"); >> >> - ep = of_graph_get_next_endpoint(dev->of_node, NULL); >> + ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); >> if (!ep) { >> dev_err(dev, "missing endpoint node\n"); >> return -EINVAL; >> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c >> index 325e99125941..662efd5e69b9 100644 >> --- a/drivers/media/i2c/tda1997x.c >> +++ b/drivers/media/i2c/tda1997x.c >> @@ -2307,7 +2307,7 @@ static int tda1997x_parse_dt(struct tda1997x_state *state) >> pdata->vidout_sel_de = DE_FREF_SEL_DE_VHREF; >> >> np = state->client->dev.of_node; >> - ep = of_graph_get_next_endpoint(np, NULL); >> + ep = of_graph_get_endpoint_by_regs(np, 0, -1); >> if (!ep) >> return -EINVAL; >> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c >> index c37f605cb75f..b1630a6c396b 100644 >> --- a/drivers/media/i2c/tvp514x.c >> +++ b/drivers/media/i2c/tvp514x.c >> @@ -988,7 +988,7 @@ tvp514x_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + endpoint = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!endpoint) >> return NULL; >> >> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c >> index a2d7bc799849..8e58ce400df2 100644 >> --- a/drivers/media/i2c/tvp7002.c >> +++ b/drivers/media/i2c/tvp7002.c >> @@ -893,7 +893,7 @@ tvp7002_get_pdata(struct i2c_client *client) >> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> return client->dev.platform_data; >> >> - endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL); >> + endpoint = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1); >> if (!endpoint) >> return NULL; >> >
