[PATCH 0/4] of: replace of_graph_get_next_endpoint()

[PATCH 0/4] of: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

Hi Rob

We should get rid of or minimize of_graph_get_next_endpoint() in
its current form. In general, drivers should be asking for a specific 
port number because their function is fixed in the binding.

	https://lore.kernel.org/r/20240131184347.GA1906672-robh@kernel.org

This patch-set replace of_graph_get_next_endpoint() by
of_graph_get_endpoint_by_regs(). There are still next_endpoint()
after this patch-set, but it will be replaced by
for_each_endpoint_of_node() in next patch-set (A)

[*] this patch-set
[o] done

	[o] tidyup of_graph_get_endpoint_count()
	[*] replace endpoint func - use endpoint_by_regs()
(A)	[ ] replace endpoint func - use for_each()
	[ ] rename endpoint func to device_endpoint
	[ ] add new port function
	[ ] add new endpont function
	[ ] remove of_graph_get_next_device_endpoint()

Kuninori Morimoto (4):
  gpu: drm: replace of_graph_get_next_endpoint()
  media: i2c: replace of_graph_get_next_endpoint()
  media: platform: replace of_graph_get_next_endpoint()
  video: fbdev: replace of_graph_get_next_endpoint()

 drivers/gpu/drm/drm_of.c                      |  2 +-
 .../drm/panel/panel-raspberrypi-touchscreen.c |  2 +-
 drivers/gpu/drm/tiny/arcpgu.c                 |  2 +-
 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 +-
 drivers/media/platform/atmel/atmel-isi.c      |  4 ++--
 drivers/media/platform/intel/pxa_camera.c     |  2 +-
 .../platform/samsung/exynos4-is/fimc-is.c     |  2 +-
 .../platform/samsung/exynos4-is/mipi-csis.c   |  2 +-
 drivers/media/platform/st/stm32/stm32-dcmi.c  |  4 ++--
 drivers/media/platform/ti/davinci/vpif.c      |  3 +--
 drivers/video/fbdev/amba-clcd.c               |  2 +-
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 20 +------------------
 drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/venc.c   |  3 ++-
 drivers/video/fbdev/pxafb.c                   |  2 +-
 include/video/omapfb_dss.h                    |  3 ---
 30 files changed, 35 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH 1/4] gpu: drm: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

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@kernel.org
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/gpu/drm/drm_of.c                              | 2 +-
 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 2 +-
 drivers/gpu/drm/tiny/arcpgu.c                         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 177b600895d3..c2eae9296012 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -516,7 +516,7 @@ struct mipi_dsi_host *drm_of_get_dsi_bus(struct device *dev)
 	/*
 	 * Get first endpoint child from device.
 	 */
-	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
 	if (!endpoint)
 		return ERR_PTR(-ENODEV);
 
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
index 4618c892cdd6..e10e469aa7a6 100644
--- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -400,7 +400,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c)
 	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
 
 	/* Look up the DSI host.  It needs to probe before we do. */
-	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
 	if (!endpoint)
 		return -ENODEV;
 
diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index e5b10e41554a..04d0053b9315 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -288,7 +288,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
 	 * There is only one output port inside each device. It is linked with
 	 * encoder endpoint.
 	 */
-	endpoint_node = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
+	endpoint_node = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
 	if (endpoint_node) {
 		encoder_node = of_graph_get_remote_port_parent(endpoint_node);
 		of_node_put(endpoint_node);
-- 
2.25.1

Re: [PATCH 1/4] gpu: drm: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Tue, Feb 06, 2024 at 02:55:01AM +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@kernel.org
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/gpu/drm/drm_of.c                              | 2 +-
>  drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 2 +-
>  drivers/gpu/drm/tiny/arcpgu.c                         | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 177b600895d3..c2eae9296012 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -516,7 +516,7 @@ struct mipi_dsi_host *drm_of_get_dsi_bus(struct device *dev)
>  	/*
>  	 * Get first endpoint child from device.
>  	 */
> -	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);

This assumes that the DSI device's port@0 will also be the input. That's
fine for current users of this function, but we should at least document
it.

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 177b600895d3..012c4d04cf51 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -504,6 +504,8 @@ EXPORT_SYMBOL_GPL(drm_of_get_data_lanes_count_ep);
  * Gets parent DSI bus for a DSI device controlled through a bus other
  * than MIPI-DCS (SPI, I2C, etc.) using the Device Tree.
  *
+ * This function assumes that the device's port@0 is the DSI input.
+ *
  * Returns pointer to mipi_dsi_host if successful, -EINVAL if the
  * request is unsupported, -EPROBE_DEFER if the DSI host is found but
  * not available, or -ENODEV otherwise.

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  	if (!endpoint)
>  		return ERR_PTR(-ENODEV);
>  
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> index 4618c892cdd6..e10e469aa7a6 100644
> --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> @@ -400,7 +400,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c)
>  	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
>  
>  	/* Look up the DSI host.  It needs to probe before we do. */
> -	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
>  	if (!endpoint)
>  		return -ENODEV;
>  
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index e5b10e41554a..04d0053b9315 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
> @@ -288,7 +288,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
>  	 * There is only one output port inside each device. It is linked with
>  	 * encoder endpoint.
>  	 */
> -	endpoint_node = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> +	endpoint_node = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
>  	if (endpoint_node) {
>  		encoder_node = of_graph_get_remote_port_parent(endpoint_node);
>  		of_node_put(endpoint_node);

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

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@kernel.org
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 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);
 	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;
 
-- 
2.25.1

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

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@kernel.org
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  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 ?

>  	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;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Hans Verkuil]

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@kernel.org
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> ---
>>  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;
>>  
> 

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

Hi Laurent, Hans

> >> 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);
(snip)
> >>  	/* 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 :-)

Nice ;)
So, let's add /* FIXME */ here in v2

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Krzysztof Hałasa]

Hans,

Hans Verkuil <hverkuil-cisco@xs4all.nl> writes:

> 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 :-(

May be reversible, though.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

On Wed, Feb 07, 2024 at 02:14:33PM +0100, Krzysztof Hałasa wrote:
> Hans,
> 
> Hans Verkuil <hverkuil-cisco@xs4all.nl> writes:
> 
> > 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 :-(
> 
> May be reversible, though.

Maybe Morimoto-san could help ? :-) What board did you use Hans ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Hans Verkuil]

On 07/02/2024 14:51, Laurent Pinchart wrote:
> On Wed, Feb 07, 2024 at 02:14:33PM +0100, Krzysztof Hałasa wrote:
>> Hans,
>>
>> Hans Verkuil <hverkuil-cisco@xs4all.nl> writes:
>>
>>> 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 :-(
>>
>> May be reversible, though.
> 
> Maybe Morimoto-san could help ? :-) What board did you use Hans ?
> 

I have a Koelsch. I tried to update uboot at one time, but bricked it and was
unable to get a different uboot installed.

It would be nice if it can be revived.

Regards,

	Hans

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Krzysztof Hałasa]

Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

>> +++ 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.

To be honest I have no idea about ADV7611 and 7612.
The 7610 I have on Tinyrex "mobo" seems to be single port.

ADV7611 seems to be mostly a 7610 in a different package (LQFP 64
instead of some BGA 76). The driver simply treats ADV7610 as a 7611.

ADV7612 is apparently dual port (only one port can be used at a time)
though:

[ADV7612] = {
        .type = ADV7612,
        .has_afe = false,
        .max_port = ADV76XX_PAD_HDMI_PORT_A,    /* B not supported */
        .num_dv_ports = 1,                      /* normally 2 */


All related in-tree DTS entries (as of v6.8.0-rc1) seem to be ADV7612.

To me it seems all known devices use the first port only.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

On Wed, Feb 07, 2024 at 02:13:05PM +0100, Krzysztof Hałasa wrote:
> Laurent,
> 
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
> >> +++ 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.
> 
> To be honest I have no idea about ADV7611 and 7612.
> The 7610 I have on Tinyrex "mobo" seems to be single port.

One issue is that the commit that added the adv7610 compatible string to
the DT bindings (commit 901a52c43359, "media: Add ADV7610 support for
adv7604 driver - DT docs.") didn't extend the conditional rules further
down in the file that defines what ports are needed. That's why I don't
know what was intended. I believe the adv7610 should be treated like the
adv7611, given that the driver has

        { .compatible = "adi,adv7610", .data = &adv76xx_chip_info[ADV7611] },
        { .compatible = "adi,adv7611", .data = &adv76xx_chip_info[ADV7611] },

Could you send a patch to address that in the DT bindings ?

> ADV7611 seems to be mostly a 7610 in a different package (LQFP 64
> instead of some BGA 76). The driver simply treats ADV7610 as a 7611.
> 
> ADV7612 is apparently dual port (only one port can be used at a time)
> though:
> 
> [ADV7612] = {
>         .type = ADV7612,
>         .has_afe = false,
>         .max_port = ADV76XX_PAD_HDMI_PORT_A,    /* B not supported */
>         .num_dv_ports = 1,                      /* normally 2 */
> 
> 
> All related in-tree DTS entries (as of v6.8.0-rc1) seem to be ADV7612.
> 
> To me it seems all known devices use the first port only.

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

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@kernel.org
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/media/platform/atmel/atmel-isi.c              | 4 ++--
 drivers/media/platform/intel/pxa_camera.c             | 2 +-
 drivers/media/platform/samsung/exynos4-is/fimc-is.c   | 2 +-
 drivers/media/platform/samsung/exynos4-is/mipi-csis.c | 2 +-
 drivers/media/platform/st/stm32/stm32-dcmi.c          | 4 ++--
 drivers/media/platform/ti/davinci/vpif.c              | 3 +--
 6 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 4046212d48b4..f615aee85968 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -831,7 +831,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
 	isi->pdata.full_mode = 1;
 	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
 
-	np = of_graph_get_next_endpoint(np, NULL);
+	np = of_graph_get_endpoint_by_regs(np, 0, -1);
 	if (!np) {
 		dev_err(&pdev->dev, "Could not find the endpoint\n");
 		return -EINVAL;
@@ -1155,7 +1155,7 @@ static int isi_graph_init(struct atmel_isi *isi)
 	struct device_node *ep;
 	int ret;
 
-	ep = of_graph_get_next_endpoint(isi->dev->of_node, NULL);
+	ep = of_graph_get_endpoint_by_regs(isi->dev->of_node, 0, -1);
 	if (!ep)
 		return -EINVAL;
 
diff --git a/drivers/media/platform/intel/pxa_camera.c b/drivers/media/platform/intel/pxa_camera.c
index 59b89e421dc2..d904952bf00e 100644
--- a/drivers/media/platform/intel/pxa_camera.c
+++ b/drivers/media/platform/intel/pxa_camera.c
@@ -2207,7 +2207,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
 		pcdev->mclk = mclk_rate;
 	}
 
-	np = of_graph_get_next_endpoint(np, NULL);
+	np = of_graph_get_endpoint_by_regs(np, 0, -1);
 	if (!np) {
 		dev_err(dev, "could not find endpoint\n");
 		return -EINVAL;
diff --git a/drivers/media/platform/samsung/exynos4-is/fimc-is.c b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
index a08c87ef6e2d..39aab667910d 100644
--- a/drivers/media/platform/samsung/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
@@ -175,7 +175,7 @@ static int fimc_is_parse_sensor_config(struct fimc_is *is, unsigned int index,
 		return -EINVAL;
 	}
 
-	ep = of_graph_get_next_endpoint(node, NULL);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return -ENXIO;
 
diff --git a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
index 686ca8753ba2..3f8bea2e3934 100644
--- a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
@@ -728,7 +728,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 				 &state->max_num_lanes))
 		return -EINVAL;
 
-	node = of_graph_get_next_endpoint(node, NULL);
+	node = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!node) {
 		dev_err(&pdev->dev, "No port node at %pOF\n",
 				pdev->dev.of_node);
diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index 8cb4fdcae137..4c00aae013af 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -1856,7 +1856,7 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
 	struct device_node *ep;
 	int ret;
 
-	ep = of_graph_get_next_endpoint(dcmi->dev->of_node, NULL);
+	ep = of_graph_get_endpoint_by_regs(dcmi->dev->of_node, 0, -1);
 	if (!ep) {
 		dev_err(dcmi->dev, "Failed to get next endpoint\n");
 		return -EINVAL;
@@ -1915,7 +1915,7 @@ static int dcmi_probe(struct platform_device *pdev)
 				     "Could not get reset control\n");
 
 	/* Get bus characteristics from devicetree */
-	np = of_graph_get_next_endpoint(np, NULL);
+	np = of_graph_get_endpoint_by_regs(np, 0, -1);
 	if (!np) {
 		dev_err(&pdev->dev, "Could not find the endpoint\n");
 		return -ENODEV;
diff --git a/drivers/media/platform/ti/davinci/vpif.c b/drivers/media/platform/ti/davinci/vpif.c
index 63cdfed37bc9..f4e1fa76bf37 100644
--- a/drivers/media/platform/ti/davinci/vpif.c
+++ b/drivers/media/platform/ti/davinci/vpif.c
@@ -465,8 +465,7 @@ static int vpif_probe(struct platform_device *pdev)
 	 * so their devices need to be registered manually here
 	 * for their legacy platform_drivers to work.
 	 */
-	endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
-					      endpoint);
+	endpoint = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
 	if (!endpoint)
 		return 0;
 	of_node_put(endpoint);
-- 
2.25.1

Re: [PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, Feb 06, 2024 at 02:55:38AM +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@kernel.org
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/media/platform/atmel/atmel-isi.c              | 4 ++--
>  drivers/media/platform/intel/pxa_camera.c             | 2 +-
>  drivers/media/platform/samsung/exynos4-is/fimc-is.c   | 2 +-
>  drivers/media/platform/samsung/exynos4-is/mipi-csis.c | 2 +-
>  drivers/media/platform/st/stm32/stm32-dcmi.c          | 4 ++--
>  drivers/media/platform/ti/davinci/vpif.c              | 3 +--
>  6 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 4046212d48b4..f615aee85968 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -831,7 +831,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
>  	isi->pdata.full_mode = 1;
>  	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
>  
> -	np = of_graph_get_next_endpoint(np, NULL);
> +	np = of_graph_get_endpoint_by_regs(np, 0, -1);
>  	if (!np) {
>  		dev_err(&pdev->dev, "Could not find the endpoint\n");
>  		return -EINVAL;
> @@ -1155,7 +1155,7 @@ static int isi_graph_init(struct atmel_isi *isi)
>  	struct device_node *ep;
>  	int ret;
>  
> -	ep = of_graph_get_next_endpoint(isi->dev->of_node, NULL);
> +	ep = of_graph_get_endpoint_by_regs(isi->dev->of_node, 0, -1);
>  	if (!ep)
>  		return -EINVAL;
>  
> diff --git a/drivers/media/platform/intel/pxa_camera.c b/drivers/media/platform/intel/pxa_camera.c
> index 59b89e421dc2..d904952bf00e 100644
> --- a/drivers/media/platform/intel/pxa_camera.c
> +++ b/drivers/media/platform/intel/pxa_camera.c
> @@ -2207,7 +2207,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
>  		pcdev->mclk = mclk_rate;
>  	}
>  
> -	np = of_graph_get_next_endpoint(np, NULL);
> +	np = of_graph_get_endpoint_by_regs(np, 0, -1);
>  	if (!np) {
>  		dev_err(dev, "could not find endpoint\n");
>  		return -EINVAL;
> diff --git a/drivers/media/platform/samsung/exynos4-is/fimc-is.c b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
> index a08c87ef6e2d..39aab667910d 100644
> --- a/drivers/media/platform/samsung/exynos4-is/fimc-is.c
> +++ b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
> @@ -175,7 +175,7 @@ static int fimc_is_parse_sensor_config(struct fimc_is *is, unsigned int index,
>  		return -EINVAL;
>  	}
>  
> -	ep = of_graph_get_next_endpoint(node, NULL);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return -ENXIO;
>  
> diff --git a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> index 686ca8753ba2..3f8bea2e3934 100644
> --- a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> +++ b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> @@ -728,7 +728,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
>  				 &state->max_num_lanes))
>  		return -EINVAL;
>  
> -	node = of_graph_get_next_endpoint(node, NULL);
> +	node = of_graph_get_endpoint_by_regs(node, 0, -1);

This is not correct, see
Documentation/devicetree/bindings/media/samsung,exynos4210-csis.yaml.

>  	if (!node) {
>  		dev_err(&pdev->dev, "No port node at %pOF\n",
>  				pdev->dev.of_node);
> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> index 8cb4fdcae137..4c00aae013af 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> @@ -1856,7 +1856,7 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>  	struct device_node *ep;
>  	int ret;
>  
> -	ep = of_graph_get_next_endpoint(dcmi->dev->of_node, NULL);
> +	ep = of_graph_get_endpoint_by_regs(dcmi->dev->of_node, 0, -1);
>  	if (!ep) {
>  		dev_err(dcmi->dev, "Failed to get next endpoint\n");
>  		return -EINVAL;
> @@ -1915,7 +1915,7 @@ static int dcmi_probe(struct platform_device *pdev)
>  				     "Could not get reset control\n");
>  
>  	/* Get bus characteristics from devicetree */
> -	np = of_graph_get_next_endpoint(np, NULL);
> +	np = of_graph_get_endpoint_by_regs(np, 0, -1);
>  	if (!np) {
>  		dev_err(&pdev->dev, "Could not find the endpoint\n");
>  		return -ENODEV;
> diff --git a/drivers/media/platform/ti/davinci/vpif.c b/drivers/media/platform/ti/davinci/vpif.c
> index 63cdfed37bc9..f4e1fa76bf37 100644
> --- a/drivers/media/platform/ti/davinci/vpif.c
> +++ b/drivers/media/platform/ti/davinci/vpif.c
> @@ -465,8 +465,7 @@ static int vpif_probe(struct platform_device *pdev)
>  	 * so their devices need to be registered manually here
>  	 * for their legacy platform_drivers to work.
>  	 */
> -	endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
> -					      endpoint);
> +	endpoint = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
>  	if (!endpoint)
>  		return 0;
>  	of_node_put(endpoint);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

Hi Laurent

Thank you for your review

> > diff --git a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > index 686ca8753ba2..3f8bea2e3934 100644
> > --- a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > +++ b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > @@ -728,7 +728,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
> >  				 &state->max_num_lanes))
> >  		return -EINVAL;
> >  
> > -	node = of_graph_get_next_endpoint(node, NULL);
> > +	node = of_graph_get_endpoint_by_regs(node, 0, -1);
> 
> This is not correct, see
> Documentation/devicetree/bindings/media/samsung,exynos4210-csis.yaml.

Hmm... Then, It can be like this ?

	+ node = of_graph_get_endpoint_by_regs(node, -1, -1);




Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

On Tue, Feb 06, 2024 at 11:45:58PM +0000, Kuninori Morimoto wrote:
> 
> Hi Laurent
> 
> Thank you for your review
> 
> > > diff --git a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > > index 686ca8753ba2..3f8bea2e3934 100644
> > > --- a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > > +++ b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c
> > > @@ -728,7 +728,7 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
> > >  				 &state->max_num_lanes))
> > >  		return -EINVAL;
> > >  
> > > -	node = of_graph_get_next_endpoint(node, NULL);
> > > +	node = of_graph_get_endpoint_by_regs(node, 0, -1);
> > 
> > This is not correct, see
> > Documentation/devicetree/bindings/media/samsung,exynos4210-csis.yaml.
> 
> Hmm... Then, It can be like this ?
> 
> 	+ node = of_graph_get_endpoint_by_regs(node, -1, -1);

I suppose that would work, even if we should really try not to pass -1
for the port. Rob, any opinion ?

-- 
Regards,

Laurent Pinchart

[PATCH 4/4] video: fbdev: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

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@kernel.org
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/amba-clcd.c               |  2 +-
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 20 +------------------
 drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/venc.c   |  3 ++-
 drivers/video/fbdev/pxafb.c                   |  2 +-
 include/video/omapfb_dss.h                    |  3 ---
 8 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 0399db369e70..2371b204cfd2 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -691,7 +691,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
 	/*
 	 * Fetch the panel endpoint.
 	 */
-	endpoint = of_graph_get_next_endpoint(fb->dev->dev.of_node, NULL);
+	endpoint = of_graph_get_endpoint_by_regs(fb->dev->dev.of_node, 0, -1);
 	if (!endpoint)
 		return -ENODEV;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
index b7eb17a16ec4..1f13bcf73da5 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
@@ -28,6 +28,7 @@
 #include <linux/debugfs.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/component.h>
 
@@ -5079,7 +5080,7 @@ static int dsi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	struct omap_dsi_pin_config pin_cfg;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
index 0282d4eef139..14965a3fd05b 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -130,24 +130,6 @@ static struct device_node *omapdss_of_get_remote_port(const struct device_node *
 	return np;
 }
 
-struct device_node *
-omapdss_of_get_first_endpoint(const struct device_node *parent)
-{
-	struct device_node *port, *ep;
-
-	port = omapdss_of_get_next_port(parent, NULL);
-
-	if (!port)
-		return NULL;
-
-	ep = omapdss_of_get_next_endpoint(port, NULL);
-
-	of_node_put(port);
-
-	return ep;
-}
-EXPORT_SYMBOL_GPL(omapdss_of_get_first_endpoint);
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node)
 {
@@ -155,7 +137,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
 	struct device_node *src_port;
 	struct omap_dss_device *src;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return ERR_PTR(-EINVAL);
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
index f05b4e35a842..8f407ec134dc 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
@@ -20,6 +20,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
 #include <video/omapfb_dss.h>
@@ -529,7 +530,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
index 03292945b1d4..4ad219f522b9 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
 #include <video/omapfb_dss.h>
@@ -561,7 +562,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/venc.c b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
index c9d40e28a06f..0bd80d3b8f1b 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/venc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
@@ -24,6 +24,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/component.h>
 
 #include <video/omapfb_dss.h>
@@ -764,7 +765,7 @@ static int venc_probe_of(struct platform_device *pdev)
 	u32 channels;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index fa943612c4e2..2ef56fa28aff 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -2171,7 +2171,7 @@ static int of_get_pxafb_mode_info(struct device *dev,
 	u32 bus_width;
 	int ret, i;
 
-	np = of_graph_get_next_endpoint(dev->of_node, NULL);
+	np = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
 	if (!np) {
 		dev_err(dev, "could not find endpoint\n");
 		return -EINVAL;
diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
index e8eaac2cb7b8..a8c0c3eeeb5b 100644
--- a/include/video/omapfb_dss.h
+++ b/include/video/omapfb_dss.h
@@ -819,9 +819,6 @@ struct device_node *
 omapdss_of_get_next_endpoint(const struct device_node *parent,
 			     struct device_node *prev);
 
-struct device_node *
-omapdss_of_get_first_endpoint(const struct device_node *parent);
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node);
 #else
-- 
2.25.1

Re: [PATCH 4/4] video: fbdev: replace of_graph_get_next_endpoint()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, Feb 06, 2024 at 02:55:45AM +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@kernel.org
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/video/fbdev/amba-clcd.c               |  2 +-
>  drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |  3 ++-
>  drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 20 +------------------
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |  3 ++-
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |  3 ++-
>  drivers/video/fbdev/omap2/omapfb/dss/venc.c   |  3 ++-
>  drivers/video/fbdev/pxafb.c                   |  2 +-
>  include/video/omapfb_dss.h                    |  3 ---
>  8 files changed, 11 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
> index 0399db369e70..2371b204cfd2 100644
> --- a/drivers/video/fbdev/amba-clcd.c
> +++ b/drivers/video/fbdev/amba-clcd.c

This driver has been deleted in v6.8-rc1.

> @@ -691,7 +691,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
>  	/*
>  	 * Fetch the panel endpoint.
>  	 */
> -	endpoint = of_graph_get_next_endpoint(fb->dev->dev.of_node, NULL);
> +	endpoint = of_graph_get_endpoint_by_regs(fb->dev->dev.of_node, 0, -1);
>  	if (!endpoint)
>  		return -ENODEV;
>  
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> index b7eb17a16ec4..1f13bcf73da5 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> @@ -28,6 +28,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/of_platform.h>
>  #include <linux/component.h>
>  
> @@ -5079,7 +5080,7 @@ static int dsi_probe_of(struct platform_device *pdev)
>  	struct device_node *ep;
>  	struct omap_dsi_pin_config pin_cfg;
>  
> -	ep = omapdss_of_get_first_endpoint(node);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return 0;
>  
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> index 0282d4eef139..14965a3fd05b 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> @@ -130,24 +130,6 @@ static struct device_node *omapdss_of_get_remote_port(const struct device_node *
>  	return np;
>  }
>  
> -struct device_node *
> -omapdss_of_get_first_endpoint(const struct device_node *parent)
> -{
> -	struct device_node *port, *ep;
> -
> -	port = omapdss_of_get_next_port(parent, NULL);
> -
> -	if (!port)
> -		return NULL;
> -
> -	ep = omapdss_of_get_next_endpoint(port, NULL);
> -
> -	of_node_put(port);
> -
> -	return ep;
> -}
> -EXPORT_SYMBOL_GPL(omapdss_of_get_first_endpoint);
> -

I *think* replacing omapdss_of_get_first_endpoint() with
of_graph_get_endpoint_by_regs(0, -1) is functionally equivalent in all
cases, but a confirmation from Tomi would be nice. I wonder if it
wouldn't be time to drop the fbdev driver though...

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  struct omap_dss_device *
>  omapdss_of_find_source_for_first_ep(struct device_node *node)
>  {
> @@ -155,7 +137,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
>  	struct device_node *src_port;
>  	struct omap_dss_device *src;
>  
> -	ep = omapdss_of_get_first_endpoint(node);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return ERR_PTR(-EINVAL);
>  
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> index f05b4e35a842..8f407ec134dc 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> @@ -20,6 +20,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/component.h>
>  #include <video/omapfb_dss.h>
> @@ -529,7 +530,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
>  	struct device_node *ep;
>  	int r;
>  
> -	ep = omapdss_of_get_first_endpoint(node);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return 0;
>  
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
> index 03292945b1d4..4ad219f522b9 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
> @@ -25,6 +25,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/component.h>
>  #include <video/omapfb_dss.h>
> @@ -561,7 +562,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
>  	struct device_node *ep;
>  	int r;
>  
> -	ep = omapdss_of_get_first_endpoint(node);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return 0;
>  
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/venc.c b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
> index c9d40e28a06f..0bd80d3b8f1b 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/venc.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
> @@ -24,6 +24,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/component.h>
>  
>  #include <video/omapfb_dss.h>
> @@ -764,7 +765,7 @@ static int venc_probe_of(struct platform_device *pdev)
>  	u32 channels;
>  	int r;
>  
> -	ep = omapdss_of_get_first_endpoint(node);
> +	ep = of_graph_get_endpoint_by_regs(node, 0, -1);
>  	if (!ep)
>  		return 0;
>  
> diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
> index fa943612c4e2..2ef56fa28aff 100644
> --- a/drivers/video/fbdev/pxafb.c
> +++ b/drivers/video/fbdev/pxafb.c
> @@ -2171,7 +2171,7 @@ static int of_get_pxafb_mode_info(struct device *dev,
>  	u32 bus_width;
>  	int ret, i;
>  
> -	np = of_graph_get_next_endpoint(dev->of_node, NULL);
> +	np = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
>  	if (!np) {
>  		dev_err(dev, "could not find endpoint\n");
>  		return -EINVAL;
> diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
> index e8eaac2cb7b8..a8c0c3eeeb5b 100644
> --- a/include/video/omapfb_dss.h
> +++ b/include/video/omapfb_dss.h
> @@ -819,9 +819,6 @@ struct device_node *
>  omapdss_of_get_next_endpoint(const struct device_node *parent,
>  			     struct device_node *prev);
>  
> -struct device_node *
> -omapdss_of_get_first_endpoint(const struct device_node *parent);
> -
>  struct omap_dss_device *
>  omapdss_of_find_source_for_first_ep(struct device_node *node);
>  #else

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] video: fbdev: replace of_graph_get_next_endpoint()

[Kuninori Morimoto]

Hi Laurent

Thank you for the review

> > 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);
(snip)
> > --- a/drivers/video/fbdev/amba-clcd.c
> > +++ b/drivers/video/fbdev/amba-clcd.c
> 
> This driver has been deleted in v6.8-rc1.

Thank you for pointing it.
I will rebase to latest of branch in v2


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto