linux-kernel - RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

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:
 <PH7PR12MB72845D56BF4361441AA9CB9A8A852@PH7PR12MB7284.namprd12.prod.outlook.com>
Date: Mon, 12 Aug 2024 12:57:13 +0000
From: "Trivedi Manojbhai, Naman" <Naman.TrivediManojbhai@....com>
To: Stephen Boyd <sboyd@...nel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-clk@...r.kernel.org"
	<linux-clk@...r.kernel.org>, "Simek, Michal" <michal.simek@....com>,
	"mturquette@...libre.com" <mturquette@...libre.com>, "Thangaraj, Senthil
 Nathan" <SenthilNathan.Thangaraj@....com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

Hi Stephen,

Thanks for review. Please find my response inline.

>-----Original Message-----
>From: Stephen Boyd <sboyd@...nel.org>
>Sent: Friday, August 9, 2024 4:18 AM
>To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@....com>; linux-
>arm-kernel@...ts.infradead.org; linux-clk@...r.kernel.org; Simek, Michal
><michal.simek@....com>; mturquette@...libre.com; Thangaraj, Senthil
>Nathan <SenthilNathan.Thangaraj@....com>
>Cc: linux-kernel@...r.kernel.org; Trivedi Manojbhai, Naman
><Naman.TrivediManojbhai@....com>
>Subject: Re: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency
>
>Caution: This message originated from an External Source. Use proper caution
>when opening attachments, clicking links, or responding.
>
>
>Quoting Naman Trivedi (2024-07-22 05:19:10)
>> From: Naman Trivedi Manojbhai <naman.trivedimanojbhai@....com>
>>
>> Use struct clk_parent_data to register the clock parents with the
>> clock framework instead of parent name.
>>
>> Signed-off-by: Naman Trivedi Manojbhai
>> <naman.trivedimanojbhai@....com>
>
>This is great! Thanks for doing this.
>
>> diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> b/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> index b89e55737198..6bb9704ee1d3 100644
>> --- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> +++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> @@ -104,8 +104,8 @@ static const struct clk_ops zynqmp_clk_gate_ops = {
>>   *
>>   * Return: clock hardware of the registered clock gate
>>   */
>> -struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
>> -                                       const char * const *parents,
>> +struct clk_hw *zynqmp_clk_register_gate(struct device_node *np, const
>> +char *name, u32 clk_id,
>
>General comment: Please use 'struct device' instead so that this driver isn't DT
>specific. When you do that you can similarly use
>devm_clk_hw_register() instead and introduce a lot of automatic cleanup.
>If you want to do that in two steps that's fine. One patch that uses
>parent_data/parent_hws and one that uses devm_ APIs and struct device to
>register.

Thanks for suggestion. I will check and implement this.
>
>> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
>> index a91d98e238c2..b791a459280e 100644
>> --- a/drivers/clk/zynqmp/clkc.c
>> +++ b/drivers/clk/zynqmp/clkc.c
>> @@ -543,7 +554,7 @@ static int zynqmp_clock_get_parents(u32 clk_id,
>struct clock_parent *parents,
>>   * Return: 0 on success else error+reason
>>   */
>>  static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
>> -                                 const char **parent_list, u32 *num_parents)
>> +                                 struct clk_parent_data *parent_list,
>> + u32 *num_parents)
>>  {
>>         int i = 0, ret;
>>         u32 total_parents = clock[clk_id].num_parents; @@ -555,18
>> +566,30 @@ static int zynqmp_get_parent_list(struct device_node *np,
>> u32 clk_id,
>>
>>         for (i = 0; i < total_parents; i++) {
>>                 if (!parents[i].flag) {
>> -                       parent_list[i] = parents[i].name;
>> +                       ret = of_property_match_string(np, "clock-names",
>> +
>> + parents[i].name);
>
>You shouldn't need to match 'clock-names'. The order of that property is fixed
>in the binding, which means you can simply use the index that the name is at
>in the binding in 'struct parent_data'.

This driver is common across multiple device families, and each device has different set of clock names in device tree/binding.  This implementation seemed to be generic for all devices. 
To use index directly, I have to add if..else for matching compatible strings and more if..else inside each of them for matching clock names to find index. Please let me know if this is preferred approach.