Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Marek Vasut <marex@denx.de>]

On 10/15/24 5:10 PM, Gatien CHEVALLIER wrote:
> 
> 
> On 10/14/24 20:55, Marek Vasut wrote:
>> On 10/14/24 2:36 PM, Gatien CHEVALLIER wrote:
>>>
>>>
>>> On 10/14/24 10:52, Marek Vasut wrote:
>>>> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>>>>
>>>>>
>>>>> On 10/11/24 18:17, Marek Vasut wrote:
>>>>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>>>>> platform_device *ofdev)
>>>>>>>       priv->rng.read = stm32_rng_read;
>>>>>>>       priv->rng.quality = 900;
>>>>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>>>>> sizeof(*priv->clk_bulk),
>>>>>>> +                      GFP_KERNEL);
>>>>>>> +    if (!priv->clk_bulk)
>>>>>>> +        return -ENOMEM;
>>>>>>
>>>>>> Try this:
>>>>>>
>>>>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>>>>> ...
>>>>>> // Swap the clock if they are not in the right order:
>>>>>> if (priv->data->nb_clock == 2 &&
>>>>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>>>>> {
>>>>>>   const char *id = priv->clk_bulk[1].id;
>>>>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>>>>   priv->clk_bulk[0].id = id;
>>>>>>   priv->clk_bulk[0].clk = clk;
>>>>>> }
>>>>>>
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> This won't work as the name returned by this API is clk->core->name.
>>>>> AFAICT, it doesn't correspond to the names present in the device tree
>>>>> under the "clock-names" property.
>>>>> Any other idea or are you fine with what's below?
>>>> Hmmm, it is not great, but at least it reduces the changes 
>>>> throughout the driver, so that is an improvement.
>>>>
>>>> I guess one could do some of_clk_get() and clk_is_match() in probe 
>>>> to look up the clock in OF by name and then compare which clock is 
>>>> which before swapping them in clk_bulk[] array, but that might be 
>>>> too convoluted?
>>>
>>> Yes, probably too much. What's present in the patch is not close to
>>> perfection but has the advantage of being straightforward. If we agree
>>> on that, I'll send a V3 containing the modifications in the bindings
>>> file.
>> Errr, I'm sorry, maybe there is a way to do this better. Look at 
>> drivers/clk/clk-bulk.c :
>>
>>   15 static int __must_check of_clk_bulk_get(struct device_node *np, 
>> int num_clks,
>>   16                                         struct clk_bulk_data *clks)
>>   17 {
>>   18         int ret;
>>   19         int i;
>>   20
>>   21         for (i = 0; i < num_clks; i++) {
>>   22                 clks[i].id = NULL;
>>   23                 clks[i].clk = NULL;
>>   24         }
>>   25
>>   26         for (i = 0; i < num_clks; i++) {
>>   27                 of_property_read_string_index(np, "clock-names", 
>> i, &clks[i].id);
>>   28                 clks[i].clk = of_clk_get(np, i);
>>
>> If I read this right, then clks[i].id should be the DT clock name. So 
>> the swap conditional above could use .id to identify whether the first 
>> position is core clock or not, like this:
>>
>> if (priv->data->nb_clock == 2 &&
>>      strcmp(__clk_get_name(priv->clk_bulk[0].id), "core"))
>>                                              ^^
>>
>> You might need to use devm_clk_bulk_get_all() to access the 
>> of_clk_bulk_get() .
>>
>> Or am I missing something still ?
> 
> Oooooh I see, devm_clk_bulk_get() and devm_clk_bulk_get_all() use
> a different path. I don't understand why, to be honest... The doc
> doesn't state this difference either.

Indeed, but maybe git log could clarify that ? I learnt about this 
useful trick at last year Embedded Recipes:

$ git log -L:clk_bulk_get_all:drivers/clk/clk-bulk.c

> I'll give this a try while also correcting the issue that the robot
> highlighted.
Thank you !