Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms
From: Gatien CHEVALLIER
Date: Tue Oct 15 2024 - 11:14:03 EST
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.
I'll give this a try while also correcting the issue that the robot
highlighted.
Best regards,
Gatien