linux-kernel - Re: [PATCH 3/6] soc: qcom: geni-se: add desc struct to specify clocks from device match data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <16d6c50b-f7da-a2b3-0b95-726b843e50c0@linaro.org>
Date:   Wed, 16 Nov 2022 11:32:13 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Neil Armstrong <neil.armstrong@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>
Cc:     linux-i2c@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/6] soc: qcom: geni-se: add desc struct to specify clocks
 from device match data



On 16/11/2022 11:21, Neil Armstrong wrote:
> The I2C Master Hub is a stripped down version of the GENI Serial Engine
> QUP Wrapper Controller but only supporting I2C serial engines without
> DMA support.
> 
> This prepares support for the I2C Master Hub variant, by moving
> the required clocks list to a new desc struct then passing it through the
> compatible match data.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
> ---
>   drivers/soc/qcom/qcom-geni-se.c | 57 +++++++++++++++++++++++++++++++----------
>   1 file changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index a0ceeede450f..f4f54d92a01a 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -81,19 +81,31 @@
>    */
>   
>   #define MAX_CLK_PERF_LEVEL 32
> -#define NUM_AHB_CLKS 2
> +#define MAX_CLKS 2
>   
>   /**
>    * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
>    * @dev:		Device pointer of the QUP wrapper core
>    * @base:		Base address of this instance of QUP wrapper core
> - * @ahb_clks:		Handle to the primary & secondary AHB clocks
> + * @clks:		Handle to the primary & optional secondary AHB clocks
> + * @num_clks:		Count of clocks
>    * @to_core:		Core ICC path
>    */
>   struct geni_wrapper {
>   	struct device *dev;
>   	void __iomem *base;
> -	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> +	struct clk_bulk_data clks[MAX_CLKS];
> +	unsigned int num_clks;
> +};
> +
> +/**
> + * struct geni_se_desc - Data structure to represent the QUP Wrapper resources
> + * @clks:		Name of the primary & optional secondary AHB clocks
> + * @num_clks:		Count of clock names
> + */
> +struct geni_se_desc {
> +	unsigned int num_clks;
> +	const char * const *clks;
>   };
>   
>   static const char * const icc_path_names[] = {"qup-core", "qup-config",
> @@ -496,8 +508,7 @@ static void geni_se_clks_off(struct geni_se *se)
>   	struct geni_wrapper *wrapper = se->wrapper;
>   
>   	clk_disable_unprepare(se->clk);
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
> -						wrapper->ahb_clks);
> +	clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
>   }
>   
>   /**
> @@ -528,15 +539,13 @@ static int geni_se_clks_on(struct geni_se *se)
>   	int ret;
>   	struct geni_wrapper *wrapper = se->wrapper;
>   
> -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(wrapper->ahb_clks),
> -						wrapper->ahb_clks);
> +	ret = clk_bulk_prepare_enable(wrapper->num_clks, wrapper->clks);
>   	if (ret)
>   		return ret;
>   
>   	ret = clk_prepare_enable(se->clk);
>   	if (ret)
> -		clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
> -							wrapper->ahb_clks);
> +		clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
>   	return ret;
>   }
>   
> @@ -887,11 +896,21 @@ static int geni_se_probe(struct platform_device *pdev)
>   		return PTR_ERR(wrapper->base);
>   
>   	if (!has_acpi_companion(&pdev->dev)) {
> -		wrapper->ahb_clks[0].id = "m-ahb";
> -		wrapper->ahb_clks[1].id = "s-ahb";
> -		ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
> +		const struct geni_se_desc *desc;
> +		int i;
> +
> +		desc = device_get_match_data(&pdev->dev);
> +		if (!desc)
> +			return -EINVAL;
> +
> +		wrapper->num_clks = min_t(unsigned int, desc->num_clks, MAX_CLKS);
Maybe a warning would be suitable if num mismatch happens?

Other than that,
Reviewed-by: Konrad Dybcio <konrad.dybcio@...aro.org>


Konrad
> +
> +		for (i = 0; i < wrapper->num_clks; ++i)
> +			wrapper->clks[i].id = desc->clks[i];
> +
> +		ret = devm_clk_bulk_get(dev, wrapper->num_clks, wrapper->clks);
>   		if (ret) {
> -			dev_err(dev, "Err getting AHB clks %d\n", ret);
> +			dev_err(dev, "Err getting clks %d\n", ret);
>   			return ret;
>   		}
>   	}
> @@ -901,8 +920,18 @@ static int geni_se_probe(struct platform_device *pdev)
>   	return devm_of_platform_populate(dev);
>   }
>   
> +static const char * const qup_clks[] = {
> +	"m-ahb",
> +	"s-ahb",
> +};
> +
> +static const struct geni_se_desc qup_desc = {
> +	.clks = qup_clks,
> +	.num_clks = ARRAY_SIZE(qup_clks),
> +};
> +
>   static const struct of_device_id geni_se_dt_match[] = {
> -	{ .compatible = "qcom,geni-se-qup", },
> +	{ .compatible = "qcom,geni-se-qup", .data = &qup_desc },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, geni_se_dt_match);
>