Re: [PATCH v2 03/32] clk: at91: clk-sam9x60-pll: use clk_parent_data

[Ryan Wanner <ryan.wanner@microchip.com>]

On 7/7/25 06:21, Claudiu Beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi, Ryan,
> 
> On 24.06.2025 18:08, Ryan.Wanner@microchip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>>
>> Use struct clk_parent_data instead of struct parent_hw as this leads
>> to less usage of __clk_get_hw() in SoC specific clock drivers and simpler
>> conversion of existing SoC specific clock drivers from parent_names to
>> modern clk_parent_data structures. As clk-sam9x60-pll need to know
>> parent's rate at initialization we pass it now from SoC specific drivers.
>> This will lead in the end at removing __clk_get_hw() in SoC specific
>> drivers (that will be solved by subsequent commits).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> [ryan.wanner@microchip.com: Remove SoC specific driver changes, those
>> will be added in subsequent commits.]
> 
> With this, series is not bisectable.
> 
> Also, building this patch throws:
> 
> ../drivers/clk/at91/sama7g5.c: In function ‘sama7g5_pmc_setup’:
> ../drivers/clk/at91/sama7g5.c:1054:12: warning: passing argument 5 of
> ‘sam9x60_clk_register_frac_pll’ makes integer from pointer without a cast
> [-Wint-conversion]
>  1054 |      NULL, parent_hw, i,
>       |            ^~~~~~~~~
>       |            |
>       |            struct clk_hw *
> In file included from ../drivers/clk/at91/sama7g5.c:17:
> ../drivers/clk/at91/pmc.h:260:24: note: expected ‘long unsigned int’ but
> argument is of type ‘struct clk_hw *’
>   260 |          unsigned long parent_rate, u8 id,
>       |          ~~~~~~~~~~~~~~^~~~~~~~~~~
> ../drivers/clk/at91/sama7d65.c: In function ‘sama7d65_pmc_setup’:
> ../drivers/clk/at91/sama7d65.c:1175:12: warning: passing argument 5 of
> ‘sam9x60_clk_register_frac_pll’ makes integer from pointer without a cast
> [-Wint-conversion]
>  1175 |      NULL, parent_hw, i,
>       |            ^~~~~~~~~
>       |            |
>       |            struct clk_hw *
> In file included from ../drivers/clk/at91/sama7d65.c:16:
> ../drivers/clk/at91/pmc.h:260:24: note: expected ‘long unsigned int’ but
> argument is of type ‘struct clk_hw *’
>   260 |          unsigned long parent_rate, u8 id,
>       |          ~~~~~~~~~~~~~~^~~~~~~~~~~
>   AR      drivers/clk/at91/built-in.a
> 
> 
> Same for the rest of patches in this series following the "Remove SoC
> specific driver changes" approach.

Would the best approach be to make every patch atomic and change every
SoC to match each clock function change or put back in the sama7g54
clock driver changes that where combined?

> 
>> Signed-off-by: Ryan Wanner <Ryan.Wanner@microchip.com>
>> ---
>>  drivers/clk/at91/clk-sam9x60-pll.c | 14 +++++---------
>>  drivers/clk/at91/pmc.h             |  5 +++--
>>  2 files changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c
>> index cefd9948e103..03a7d00dcc6d 100644
>> --- a/drivers/clk/at91/clk-sam9x60-pll.c
>> +++ b/drivers/clk/at91/clk-sam9x60-pll.c
>> @@ -630,19 +630,19 @@ static const struct clk_ops sam9x60_fixed_div_pll_ops = {
>>
>>  struct clk_hw * __init
>>  sam9x60_clk_register_frac_pll(struct regmap *regmap, spinlock_t *lock,
>> -                           const char *name, const char *parent_name,
>> -                           struct clk_hw *parent_hw, u8 id,
>> +                           const char *name, const struct clk_parent_data *parent_data,
>> +                           unsigned long parent_rate, u8 id,
>>                             const struct clk_pll_characteristics *characteristics,
>>                             const struct clk_pll_layout *layout, u32 flags)
>>  {
>>       struct sam9x60_frac *frac;
>>       struct clk_hw *hw;
>>       struct clk_init_data init = {};
>> -     unsigned long parent_rate, irqflags;
>> +     unsigned long irqflags;
>>       unsigned int val;
>>       int ret;
>>
>> -     if (id > PLL_MAX_ID || !lock || !parent_hw)
>> +     if (id > PLL_MAX_ID || !lock || !parent_data)
>>               return ERR_PTR(-EINVAL);
>>
>>       frac = kzalloc(sizeof(*frac), GFP_KERNEL);
>> @@ -650,10 +650,7 @@ sam9x60_clk_register_frac_pll(struct regmap *regmap, spinlock_t *lock,
>>               return ERR_PTR(-ENOMEM);
>>
>>       init.name = name;
>> -     if (parent_name)
>> -             init.parent_names = &parent_name;
>> -     else
>> -             init.parent_hws = (const struct clk_hw **)&parent_hw;
>> +     init.parent_data = (const struct clk_parent_data *)parent_data;
>>       init.num_parents = 1;
>>       if (flags & CLK_SET_RATE_GATE)
>>               init.ops = &sam9x60_frac_pll_ops;
>> @@ -684,7 +681,6 @@ sam9x60_clk_register_frac_pll(struct regmap *regmap, spinlock_t *lock,
>>                * its rate leading to enabling this PLL with unsupported
>>                * rate. This will lead to PLL not being locked at all.
>>                */
>> -             parent_rate = clk_hw_get_rate(parent_hw);
>>               if (!parent_rate) {
>>                       hw = ERR_PTR(-EINVAL);
>>                       goto free;
>> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
>> index 63d4c425bed5..b43f6652417f 100644
>> --- a/drivers/clk/at91/pmc.h
>> +++ b/drivers/clk/at91/pmc.h
>> @@ -255,8 +255,9 @@ sam9x60_clk_register_div_pll(struct regmap *regmap, spinlock_t *lock,
>>
>>  struct clk_hw * __init
>>  sam9x60_clk_register_frac_pll(struct regmap *regmap, spinlock_t *lock,
>> -                           const char *name, const char *parent_name,
>> -                           struct clk_hw *parent_hw, u8 id,
>> +                           const char *name,
>> +                           const struct clk_parent_data *parent_data,
>> +                           unsigned long parent_rate, u8 id,
>>                             const struct clk_pll_characteristics *characteristics,
>>                             const struct clk_pll_layout *layout, u32 flags);
>>
>