[v3,04/29] media: iris: initialize power resources

  From: Dikshita Agarwal <quic_dikshita@quicinc.com>

Add support for initializing Iris "resources", which are clocks,
interconnects, power domains, reset clocks, and clock frequencies
used for iris hardware.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/iris/Makefile          |   5 +-
 drivers/media/platform/qcom/iris/iris_core.h       |  20 +++
 .../platform/qcom/iris/iris_platform_common.h      |  36 +++++
 .../platform/qcom/iris/iris_platform_sm8250.c      |  38 +++++
 .../platform/qcom/iris/iris_platform_sm8550.c      |  38 +++++
 drivers/media/platform/qcom/iris/iris_probe.c      |  23 ++-
 drivers/media/platform/qcom/iris/iris_resources.c  | 171 +++++++++++++++++++++
 drivers/media/platform/qcom/iris/iris_resources.h  |  19 +++
 8 files changed, 347 insertions(+), 3 deletions(-)
  

On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
> 
> Add support for initializing Iris "resources", which are clocks,
> interconnects, power domains, reset clocks, and clock frequencies
> used for iris hardware.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---

...

> +struct iris_platform_data sm8550_data = {
> +	.icc_tbl = sm8550_icc_table,
> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
> +	.clk_rst_tbl = sm8550_clk_reset_table,
> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
> +	.pmdomain_tbl = sm8550_pmdomain_table,
> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
> +	.opp_pd_tbl = sm8550_opp_pd_table,
> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
> +	.clk_tbl = sm8550_clk_table,
> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
> +};
> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index 0a54fdaa1ab5..2616a31224f9 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>  	if (core->irq < 0)
>  		return core->irq;
>  
> +	core->iris_platform_data = of_device_get_match_data(core->dev);
> +	if (!core->iris_platform_data) {
> +		ret = -ENODEV;
> +		dev_err_probe(core->dev, ret, "init platform failed\n");

That's not even possible. I would suggest dropping entire if. But if yoi
insist, then without this weird redundant code. return -EINVAL.

> +		return ret;
> +	}
> +
> +	ret = iris_init_resources(core);
> +	if (ret) {
> +		dev_err_probe(core->dev, ret, "init resource failed\n");
> +		return ret;

How many same errors are you printing? Not mentioning that syntax of
dev_errp_rpboe is different...


> +	}
> +
>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>  	if (ret)
>  		return ret;
> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id iris_dt_match[] = {
> -	{ .compatible = "qcom,sm8550-iris", },
> -	{ .compatible = "qcom,sm8250-venus", },
> +	{
> +		.compatible = "qcom,sm8550-iris",
> +		.data = &sm8550_data,
> +	},
> +	{
> +		.compatible = "qcom,sm8250-venus",
> +		.data = &sm8250_data,

You just added this. No, please do not add code which is immediatly
incorrect.

> +	},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, iris_dt_match);
> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
> new file mode 100644
> index 000000000000..57c6f9f3449b
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interconnect.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_opp.h>
> +#include <linux/reset.h>
> +
> +#include "iris_core.h"
> +#include "iris_resources.h"
> +
> +static int iris_init_icc(struct iris_core *core)
> +{
> +	const struct icc_info *icc_tbl;
> +	u32 ret, i = 0;
> +
> +	icc_tbl = core->iris_platform_data->icc_tbl;
> +
> +	core->icc_count = core->iris_platform_data->icc_tbl_size;
> +	core->icc_tbl = devm_kzalloc(core->dev,
> +				     sizeof(struct icc_bulk_data) * core->icc_count,
> +				     GFP_KERNEL);
> +	if (!core->icc_tbl)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < core->icc_count; i++) {
> +		core->icc_tbl[i].name = icc_tbl[i].name;
> +		core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps;
> +		core->icc_tbl[i].peak_bw = 0;
> +	}
> +
> +	ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl);
> +	if (ret)
> +		dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n");
> +
> +	return ret;
> +}
> +
> +static int iris_pd_get(struct iris_core *core)
> +{
> +	int ret;
> +
> +	struct dev_pm_domain_attach_data iris_pd_data = {
> +		.pd_names = core->iris_platform_data->pmdomain_tbl,
> +		.num_pd_names = core->iris_platform_data->pmdomain_tbl_size,
> +		.pd_flags = PD_FLAG_NO_DEV_LINK,
> +	};
> +
> +	ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int iris_opp_pd_get(struct iris_core *core)
> +{
> +	int ret;
> +
> +	struct dev_pm_domain_attach_data iris_opp_pd_data = {
> +		.pd_names = core->iris_platform_data->opp_pd_tbl,
> +		.num_pd_names = core->iris_platform_data->opp_pd_tbl_size,
> +		.pd_flags = PD_FLAG_DEV_LINK_ON,
> +	};
> +
> +	ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int iris_init_power_domains(struct iris_core *core)
> +{
> +	const struct platform_clk_data *clk_tbl;
> +	u32 clk_cnt, i;
> +	int ret;
> +
> +	ret = iris_pd_get(core);
> +	if (ret)
> +		return ret;
> +
> +	ret = iris_opp_pd_get(core);
> +	if (ret)
> +		return ret;
> +
> +	clk_tbl = core->iris_platform_data->clk_tbl;
> +	clk_cnt = core->iris_platform_data->clk_tbl_size;
> +
> +	for (i = 0; i < clk_cnt; i++) {
> +		if (clk_tbl[i].clk_type == IRIS_HW_CLK) {
> +			ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	ret = devm_pm_opp_of_add_table(core->dev);
> +	if (ret) {
> +		dev_err(core->dev, "failed to add opp table\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int iris_init_clocks(struct iris_core *core)
> +{
> +	int ret;
> +
> +	ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl);
> +	if (ret < 0) {
> +		dev_err(core->dev, "failed to get bulk clock\n");

Syntax is:
return dev_err_probe(). If this is probe path. Is it?

> +		return ret;
> +	}
> +
> +	core->clk_count = ret;
> +
> +	return 0;
> +}
> +
> +static int iris_init_resets(struct iris_core *core)
> +{
> +	const char * const *rst_tbl;
> +	u32 rst_tbl_size;
> +	u32 i = 0, ret;
> +
> +	rst_tbl = core->iris_platform_data->clk_rst_tbl;
> +	rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
> +
> +	core->resets = devm_kzalloc(core->dev,
> +				    sizeof(*core->resets) * rst_tbl_size,
> +				    GFP_KERNEL);
> +	if (rst_tbl_size && !core->resets)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < rst_tbl_size; i++)
> +		core->resets[i].id = rst_tbl[i];
> +
> +	ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets);
> +	if (ret) {
> +		dev_err(core->dev, "failed to get resets\n");

Syntax is:
return dev_err_probe(). If this is probe path. Is it?

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int iris_init_resources(struct iris_core *core)
> +{
> +	int ret;
> +
> +	ret = iris_init_icc(core);
> +	if (ret)
> +		return ret;
> +
> +	ret = iris_init_power_domains(core);
> +	if (ret)
> +		return ret;
> +
> +	ret = iris_init_clocks(core);
> +	if (ret)
> +		return ret;
> +
> +	ret = iris_init_resets(core);
> +
> +	return ret;
> +}

This should be just part of of main unit file, next to probe. It is
unusual to see probe parts not next to probe. Sorry, that's wrong.

Best regards,
Krzysztof
  

On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote:
> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>
>> Add support for initializing Iris "resources", which are clocks,
>> interconnects, power domains, reset clocks, and clock frequencies
>> used for iris hardware.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
> 
> ...
> 
>> +struct iris_platform_data sm8550_data = {
>> +	.icc_tbl = sm8550_icc_table,
>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>> +	.clk_rst_tbl = sm8550_clk_reset_table,
>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>> +	.clk_tbl = sm8550_clk_table,
>> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
>> +};
>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>> index 0a54fdaa1ab5..2616a31224f9 100644
>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>>  	if (core->irq < 0)
>>  		return core->irq;
>>  
>> +	core->iris_platform_data = of_device_get_match_data(core->dev);
>> +	if (!core->iris_platform_data) {
>> +		ret = -ENODEV;
>> +		dev_err_probe(core->dev, ret, "init platform failed\n");
> 
> That's not even possible. I would suggest dropping entire if. But if yoi
> insist, then without this weird redundant code. return -EINVAL.
> 
Its possible if platform data is not initialized and this is only place we
check it, there is no further NULL check for the same.
>> +		return ret;
>> +	}
>> +
>> +	ret = iris_init_resources(core);
>> +	if (ret) {
>> +		dev_err_probe(core->dev, ret, "init resource failed\n");
>> +		return ret;
> 
> How many same errors are you printing? Not mentioning that syntax of
> dev_errp_rpboe is different...
We have these errors at multiple points to know at what point the probe
failed which is useful while debugging probe failures.
But Sure we will revisit this code and fix the syntax of dev_err_probe.
> 
> 
>> +	}
>> +
>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>  	if (ret)
>>  		return ret;
>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>  }
>>  
>>  static const struct of_device_id iris_dt_match[] = {
>> -	{ .compatible = "qcom,sm8550-iris", },
>> -	{ .compatible = "qcom,sm8250-venus", },
>> +	{
>> +		.compatible = "qcom,sm8550-iris",
>> +		.data = &sm8550_data,
>> +	},
>> +	{
>> +		.compatible = "qcom,sm8250-venus",
>> +		.data = &sm8250_data,
> 
> You just added this. No, please do not add code which is immediatly
> incorrect.
It's not incorrect, in earlier patch we only added the compatible strings
and with this patch introducing the platform data and APIs to get it.
> 
>> +	},
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(of, iris_dt_match);
>> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
>> new file mode 100644
>> index 000000000000..57c6f9f3449b
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
>> @@ -0,0 +1,171 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interconnect.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/reset.h>
>> +
>> +#include "iris_core.h"
>> +#include "iris_resources.h"
>> +
>> +static int iris_init_icc(struct iris_core *core)
>> +{
>> +	const struct icc_info *icc_tbl;
>> +	u32 ret, i = 0;
>> +
>> +	icc_tbl = core->iris_platform_data->icc_tbl;
>> +
>> +	core->icc_count = core->iris_platform_data->icc_tbl_size;
>> +	core->icc_tbl = devm_kzalloc(core->dev,
>> +				     sizeof(struct icc_bulk_data) * core->icc_count,
>> +				     GFP_KERNEL);
>> +	if (!core->icc_tbl)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < core->icc_count; i++) {
>> +		core->icc_tbl[i].name = icc_tbl[i].name;
>> +		core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps;
>> +		core->icc_tbl[i].peak_bw = 0;
>> +	}
>> +
>> +	ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl);
>> +	if (ret)
>> +		dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int iris_pd_get(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	struct dev_pm_domain_attach_data iris_pd_data = {
>> +		.pd_names = core->iris_platform_data->pmdomain_tbl,
>> +		.num_pd_names = core->iris_platform_data->pmdomain_tbl_size,
>> +		.pd_flags = PD_FLAG_NO_DEV_LINK,
>> +	};
>> +
>> +	ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_opp_pd_get(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	struct dev_pm_domain_attach_data iris_opp_pd_data = {
>> +		.pd_names = core->iris_platform_data->opp_pd_tbl,
>> +		.num_pd_names = core->iris_platform_data->opp_pd_tbl_size,
>> +		.pd_flags = PD_FLAG_DEV_LINK_ON,
>> +	};
>> +
>> +	ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_init_power_domains(struct iris_core *core)
>> +{
>> +	const struct platform_clk_data *clk_tbl;
>> +	u32 clk_cnt, i;
>> +	int ret;
>> +
>> +	ret = iris_pd_get(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_opp_pd_get(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	clk_tbl = core->iris_platform_data->clk_tbl;
>> +	clk_cnt = core->iris_platform_data->clk_tbl_size;
>> +
>> +	for (i = 0; i < clk_cnt; i++) {
>> +		if (clk_tbl[i].clk_type == IRIS_HW_CLK) {
>> +			ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	ret = devm_pm_opp_of_add_table(core->dev);
>> +	if (ret) {
>> +		dev_err(core->dev, "failed to add opp table\n");
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int iris_init_clocks(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl);
>> +	if (ret < 0) {
>> +		dev_err(core->dev, "failed to get bulk clock\n");
> 
> Syntax is:
> return dev_err_probe(). If this is probe path. Is it?
> 
Sure will fix.
>> +		return ret;
>> +	}
>> +
>> +	core->clk_count = ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_init_resets(struct iris_core *core)
>> +{
>> +	const char * const *rst_tbl;
>> +	u32 rst_tbl_size;
>> +	u32 i = 0, ret;
>> +
>> +	rst_tbl = core->iris_platform_data->clk_rst_tbl;
>> +	rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
>> +
>> +	core->resets = devm_kzalloc(core->dev,
>> +				    sizeof(*core->resets) * rst_tbl_size,
>> +				    GFP_KERNEL);
>> +	if (rst_tbl_size && !core->resets)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < rst_tbl_size; i++)
>> +		core->resets[i].id = rst_tbl[i];
>> +
>> +	ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets);
>> +	if (ret) {
>> +		dev_err(core->dev, "failed to get resets\n");
> 
> Syntax is:
> return dev_err_probe(). If this is probe path. Is it?
> 
Sure, will fix
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int iris_init_resources(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	ret = iris_init_icc(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_power_domains(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_clocks(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_resets(core);
>> +
>> +	return ret;
>> +}
> 
> This should be just part of of main unit file, next to probe. It is
> unusual to see probe parts not next to probe. Sorry, that's wrong.
> 
All the APIs handling(init/enable/disable) the different resources (PM
domains, OPP, clocks, buses) are kept in this iris_resource.c file hence
this API to init all those resources is kept here to not load iris_probe.c
file.

> Best regards,
> Krzysztof
>
  

On 05/09/2024 13:53, Dikshita Agarwal wrote:
> 
> 
> On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote:
>> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
>>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>
>>> Add support for initializing Iris "resources", which are clocks,
>>> interconnects, power domains, reset clocks, and clock frequencies
>>> used for iris hardware.
>>>
>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>> ---
>>
>> ...
>>
>>> +struct iris_platform_data sm8550_data = {
>>> +	.icc_tbl = sm8550_icc_table,
>>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>>> +	.clk_rst_tbl = sm8550_clk_reset_table,
>>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
>>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>>> +	.clk_tbl = sm8550_clk_table,
>>> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
>>> +};
>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>>> index 0a54fdaa1ab5..2616a31224f9 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>>>  	if (core->irq < 0)
>>>  		return core->irq;
>>>  
>>> +	core->iris_platform_data = of_device_get_match_data(core->dev);
>>> +	if (!core->iris_platform_data) {
>>> +		ret = -ENODEV;
>>> +		dev_err_probe(core->dev, ret, "init platform failed\n");
>>
>> That's not even possible. I would suggest dropping entire if. But if yoi
>> insist, then without this weird redundant code. return -EINVAL.
>>
> Its possible if platform data is not initialized and this is only place we
> check it, there is no further NULL check for the same.

It is possible? Then point me to the code line. Or present some code
flow leading to it.

>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = iris_init_resources(core);
>>> +	if (ret) {
>>> +		dev_err_probe(core->dev, ret, "init resource failed\n");
>>> +		return ret;
>>
>> How many same errors are you printing? Not mentioning that syntax of
>> dev_errp_rpboe is different...
> We have these errors at multiple points to know at what point the probe
> failed which is useful while debugging probe failures.

Duplicating is not helpful.

> But Sure we will revisit this code and fix the syntax of dev_err_probe.

>>
>>
>>> +	}
>>> +
>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>  	if (ret)
>>>  		return ret;
>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>  }
>>>  
>>>  static const struct of_device_id iris_dt_match[] = {
>>> -	{ .compatible = "qcom,sm8550-iris", },
>>> -	{ .compatible = "qcom,sm8250-venus", },
>>> +	{
>>> +		.compatible = "qcom,sm8550-iris",
>>> +		.data = &sm8550_data,
>>> +	},
>>> +	{
>>> +		.compatible = "qcom,sm8250-venus",
>>> +		.data = &sm8250_data,
>>
>> You just added this. No, please do not add code which is immediatly
>> incorrect.
> It's not incorrect, in earlier patch we only added the compatible strings
> and with this patch introducing the platform data and APIs to get it.

It is incorrect to immediately remove it. You keep arguing on basic
stuff. Sorry, but that is not how it works. If you add code and
IMMEDIATELY remove it, then it means the code was not needed. Or was not
correct. Choose one.

...

>>
>> This should be just part of of main unit file, next to probe. It is
>> unusual to see probe parts not next to probe. Sorry, that's wrong.
>>
> All the APIs handling(init/enable/disable) the different resources (PM
> domains, OPP, clocks, buses) are kept in this iris_resource.c file hence
> this API to init all those resources is kept here to not load iris_probe.c
> file.

You introduce your own coding style and as an argument you use just "I
do this".

The expected is to see resource initialization next to probe. Repeating
what your code does, is not helping me to understand your design choice.

Best regards,
Krzysztof
  

Hi Krzysztof,

On 9/5/2024 5:27 PM, Krzysztof Kozlowski wrote:
> On 05/09/2024 13:53, Dikshita Agarwal wrote:
>>
>>
>> On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote:
>>> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
>>>> From: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>>
>>>> Add support for initializing Iris "resources", which are clocks,
>>>> interconnects, power domains, reset clocks, and clock frequencies
>>>> used for iris hardware.
>>>>
>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>> ---
>>>
>>> ...
>>>
>>>> +struct iris_platform_data sm8550_data = {
>>>> +	.icc_tbl = sm8550_icc_table,
>>>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>>>> +	.clk_rst_tbl = sm8550_clk_reset_table,
>>>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
>>>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>>>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>>>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>>>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>>>> +	.clk_tbl = sm8550_clk_table,
>>>> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
>>>> +};
>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>>>> index 0a54fdaa1ab5..2616a31224f9 100644
>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>>>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>>>>  	if (core->irq < 0)
>>>>  		return core->irq;
>>>>  
>>>> +	core->iris_platform_data = of_device_get_match_data(core->dev);
>>>> +	if (!core->iris_platform_data) {
>>>> +		ret = -ENODEV;
>>>> +		dev_err_probe(core->dev, ret, "init platform failed\n");
>>>
>>> That's not even possible. I would suggest dropping entire if. But if yoi
>>> insist, then without this weird redundant code. return -EINVAL.
>>>
>> Its possible if platform data is not initialized and this is only place we
>> check it, there is no further NULL check for the same.
> 
> It is possible? Then point me to the code line. Or present some code
> flow leading to it.
Lets go with return -EINVAL in this if block.

> 
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = iris_init_resources(core);
>>>> +	if (ret) {
>>>> +		dev_err_probe(core->dev, ret, "init resource failed\n");
>>>> +		return ret;
>>>
>>> How many same errors are you printing? Not mentioning that syntax of
>>> dev_errp_rpboe is different...
>> We have these errors at multiple points to know at what point the probe
>> failed which is useful while debugging probe failures.
> 
> Duplicating is not helpful.
> 
>> But Sure we will revisit this code and fix the syntax of dev_err_probe.
> 
>>>
>>>
>>>> +	}
>>>> +
>>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>>  	if (ret)
>>>>  		return ret;
>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>>  }
>>>>  
>>>>  static const struct of_device_id iris_dt_match[] = {
>>>> -	{ .compatible = "qcom,sm8550-iris", },
>>>> -	{ .compatible = "qcom,sm8250-venus", },
>>>> +	{
>>>> +		.compatible = "qcom,sm8550-iris",
>>>> +		.data = &sm8550_data,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "qcom,sm8250-venus",
>>>> +		.data = &sm8250_data,
>>>
>>> You just added this. No, please do not add code which is immediatly
>>> incorrect.
>> It's not incorrect, in earlier patch we only added the compatible strings
>> and with this patch introducing the platform data and APIs to get it.
> 
> It is incorrect to immediately remove it. You keep arguing on basic
> stuff. Sorry, but that is not how it works. If you add code and
> IMMEDIATELY remove it, then it means the code was not needed. Or was not
> correct. Choose one.
I think it is not removing it. It is adding platform data to compatibles
introduced in previous patch. Maybe it appears as if it is removing it.
> 
> ...
> 
>>>
>>> This should be just part of of main unit file, next to probe. It is
>>> unusual to see probe parts not next to probe. Sorry, that's wrong.
>>>
>> All the APIs handling(init/enable/disable) the different resources (PM
>> domains, OPP, clocks, buses) are kept in this iris_resource.c file hence
>> this API to init all those resources is kept here to not load iris_probe.c
>> file.
> 
> You introduce your own coding style and as an argument you use just "I
> do this".
> 
> The expected is to see resource initialization next to probe. Repeating
> what your code does, is not helping me to understand your design choice.
I see your point and it would be good to have the resources initialized as part
of probe.

> Best regards,
> Krzysztof

Regards,
Vikash
  

On 06/09/2024 13:21, Vikash Garodia wrote:
>>>>
>>>>> +	}
>>>>> +
>>>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>>>  }
>>>>>  
>>>>>  static const struct of_device_id iris_dt_match[] = {
>>>>> -	{ .compatible = "qcom,sm8550-iris", },
>>>>> -	{ .compatible = "qcom,sm8250-venus", },
>>>>> +	{
>>>>> +		.compatible = "qcom,sm8550-iris",
>>>>> +		.data = &sm8550_data,
>>>>> +	},
>>>>> +	{
>>>>> +		.compatible = "qcom,sm8250-venus",
>>>>> +		.data = &sm8250_data,
>>>>
>>>> You just added this. No, please do not add code which is immediatly
>>>> incorrect.
>>> It's not incorrect, in earlier patch we only added the compatible strings
>>> and with this patch introducing the platform data and APIs to get it.
>>
>> It is incorrect to immediately remove it. You keep arguing on basic
>> stuff. Sorry, but that is not how it works. If you add code and
>> IMMEDIATELY remove it, then it means the code was not needed. Or was not
>> correct. Choose one.
> I think it is not removing it. It is adding platform data to compatibles
> introduced in previous patch. Maybe it appears as if it is removing it.

I know how the diff works.

The way you avoid solving the problem with trivial responses is not
helping. We already have been there with another patchset from different
person and it lead to annoying all DT maintainers and (usually very
patient) some of networking folks. I ask you to approach to the review
seriously.

NAK.

Best regards,
Krzysztof
  

Hi,

On 9/6/2024 5:34 PM, Krzysztof Kozlowski wrote:
> On 06/09/2024 13:21, Vikash Garodia wrote:
>>>>>
>>>>>> +	}
>>>>>> +
>>>>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>>>>  	if (ret)
>>>>>>  		return ret;
>>>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>>>>  }
>>>>>>  
>>>>>>  static const struct of_device_id iris_dt_match[] = {
>>>>>> -	{ .compatible = "qcom,sm8550-iris", },
>>>>>> -	{ .compatible = "qcom,sm8250-venus", },
>>>>>> +	{
>>>>>> +		.compatible = "qcom,sm8550-iris",
>>>>>> +		.data = &sm8550_data,
>>>>>> +	},
>>>>>> +	{
>>>>>> +		.compatible = "qcom,sm8250-venus",
>>>>>> +		.data = &sm8250_data,
>>>>>
>>>>> You just added this. No, please do not add code which is immediatly
>>>>> incorrect.
>>>> It's not incorrect, in earlier patch we only added the compatible strings
>>>> and with this patch introducing the platform data and APIs to get it.
>>>
>>> It is incorrect to immediately remove it. You keep arguing on basic
>>> stuff. Sorry, but that is not how it works. If you add code and
>>> IMMEDIATELY remove it, then it means the code was not needed. Or was not
>>> correct. Choose one.
>> I think it is not removing it. It is adding platform data to compatibles
>> introduced in previous patch. Maybe it appears as if it is removing it.
> 
> I know how the diff works.
Perhaps, i have misunderstood. Are you suggesting to add compat data and
compatible string together in single patch rather than splitting it in 2 patches
? If so, that would essentially end up squashing patch #3 and #4. Let me know if
that would address your comment and we will plan to do that.

> The way you avoid solving the problem with trivial responses is not
> helping. We already have been there with another patchset from different
> person and it lead to annoying all DT maintainers and (usually very
> patient) some of networking folks. I ask you to approach to the review
> seriously.
Please be assured that all comments are valued upon and are being taken seriously.

> NAK.
> 
> Best regards,
> Krzysztof
> 
Regards,
Vikash
  

On 06/09/2024 21:47, Vikash Garodia wrote:
> Hi,
> 
> On 9/6/2024 5:34 PM, Krzysztof Kozlowski wrote:
>> On 06/09/2024 13:21, Vikash Garodia wrote:
>>>>>>
>>>>>>> +	}
>>>>>>> +
>>>>>>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>>>>>>  	if (ret)
>>>>>>>  		return ret;
>>>>>>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>>>>>>  }
>>>>>>>  
>>>>>>>  static const struct of_device_id iris_dt_match[] = {
>>>>>>> -	{ .compatible = "qcom,sm8550-iris", },
>>>>>>> -	{ .compatible = "qcom,sm8250-venus", },
>>>>>>> +	{
>>>>>>> +		.compatible = "qcom,sm8550-iris",
>>>>>>> +		.data = &sm8550_data,
>>>>>>> +	},
>>>>>>> +	{
>>>>>>> +		.compatible = "qcom,sm8250-venus",
>>>>>>> +		.data = &sm8250_data,
>>>>>>
>>>>>> You just added this. No, please do not add code which is immediatly
>>>>>> incorrect.
>>>>> It's not incorrect, in earlier patch we only added the compatible strings
>>>>> and with this patch introducing the platform data and APIs to get it.
>>>>
>>>> It is incorrect to immediately remove it. You keep arguing on basic
>>>> stuff. Sorry, but that is not how it works. If you add code and
>>>> IMMEDIATELY remove it, then it means the code was not needed. Or was not
>>>> correct. Choose one.
>>> I think it is not removing it. It is adding platform data to compatibles
>>> introduced in previous patch. Maybe it appears as if it is removing it.
>>
>> I know how the diff works.
> Perhaps, i have misunderstood. Are you suggesting to add compat data and
> compatible string together in single patch rather than splitting it in 2 patches
> ? If so, that would essentially end up squashing patch #3 and #4. Let me know if
> that would address your comment and we will plan to do that.

You are supposed to organize your patches so they have logical order. I
already explained why this order is wrong. What's more, previous patch
of two equal compatibles does not have much value. Devices cannot work
and code is confusing.

Best regards,
Krzysztof