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
