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

[Krzysztof Kozlowski]

On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
> From: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> ---

...

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