linux-kernel - Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

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: <a926924f-2033-acd6-67cb-210a5e487ebb@codeaurora.org>
Date:   Mon, 7 May 2018 16:18:06 +0530
From:   Taniya Das <tdas@...eaurora.org>
To:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Amit Nischal <anischal@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v8] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Hello Stephen,

Could you please let me know your comments on the below.

On 5/4/2018 10:21 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-05-04 03:02:38)
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..944fe04
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,334 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +};
>> +
>> +struct clk_rpmh_desc {
>> +       struct clk_hw **clks;
>> +       size_t num_clks;
>> +};
> 
> This could be replaced with the clk_hw_onecell_data struct and then the
> only problem becomes the const part which seems pretty impossible to fix
> at this point. One "workaround" is to memdup the structure. Ugh.
> 

Will be okay, if I can the following?

_probe...
{
	struct clk_rpmh_desc *hw_desc_data;
	....

	hw_desc_data = kmemdup(desc, sizeof(*desc), GFP_KERNEL);

	...
	ret = devm_of_clk_add_hw_provider(&pdev->dev, 	  of_clk_rpmh_hw_get, 
hw_desc_data);
         ....

}

And also I fix the "getter" function.

>> +
>> +static DEFINE_MUTEX(rpmh_clk_lock);
>> +
>> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,   \
>> +                         _res_en_offset, _res_on, _div)                \
>> +       static struct clk_rpmh _platform##_##_name_active;              \
>> +       static struct clk_rpmh _platform##_##_name = {                  \
> [..]
>> +
>> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
>> +                                       unsigned long prate)
>> +{
>> +       struct clk_rpmh *r = to_clk_rpmh(hw);
>> +       unsigned long rate = prate;
>> +
>> +       /*
>> +        * RPMh clocks have a fixed rate. Return static rate.
>> +        */
>> +       do_div(rate, r->div);
> 
> Integer division won't suffice?
> 
>> +       return rate;
>> +}
> [...]
>> +
>> +static struct clk_hw *of_clk_rpmh_hw_get(struct of_phandle_args *clkspec,
>> +                                            void *data)
>> +{
>> +       struct clk_hw *hw_clks = data;
>> +       unsigned int idx = clkspec->args[0];
>> +
>> +       if (idx < 0) {
> 
> It can't be less than 0 though because it's unsigned.
> 
>> +               pr_err("%s: invalid index %u\n", __func__, idx);
>> +               return ERR_PTR(-EINVAL);
>> +       }
>> +
>> +       return &hw_clks[idx];
>> +}

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--