Message-ID: <81fd218f-aa0f-4710-b832-cab927bfab9d@kernel.org>
Date: Tue, 27 Aug 2024 12:51:45 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: quic_dikshita@...cinc.com, Vikash Garodia <quic_vgarodia@...cinc.com>,
 Abhinav Kumar <quic_abhinavk@...cinc.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 04/29] media: iris: initialize power resources

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

Powered by blists - more mailing lists