[2/4] media: i2c: replace of_graph_get_next_endpoint()
Commit Message
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(-)
Comments
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;
>
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;
>>
>
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
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.
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.
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 ?
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
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.
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;
@@ -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;