Re: [PATCH 1/2] pwm: meson: make full use of common clock framework

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hi Heiner,

On Sat, Apr 8, 2023 at 10:43 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
[...]
> +               init.name = name;
> +               init.ops = &clk_divider_ops;
> +               init.flags = CLK_SET_RATE_PARENT;
> +               parent_hws[0] = &channel->mux.hw;
> +               init.parent_hws = parent_hws;
> +               init.num_parents = 1;
There's a very subtle bug in this code:
You're re-using the same struct clk_init_data for all clocks.
The mux above sets
- init.parent_names
- init.num_parents
- (amongst others)

but for the divider and gate you're only overwriting init.num_parents,
which is why I end up with:
 xtal                                 8        8        1    24000000
        0     0  50000         Y
   [...]
   c1108650.pwm#gate1                1        1        0    24000000
       0     0  50000         Y
   c1108650.pwm#div1                 0        0        0    24000000
       0     0  50000         Y
   c1108650.pwm#mux1                 0        0        0    24000000
       0     0  50000         Y
   c1108650.pwm#gate0                1        1        0    24000000
       0     0  50000         Y
   c1108650.pwm#div0                 0        0        0    24000000
       0     0  50000         Y
   c1108650.pwm#mux0                 0        0        0    24000000
       0     0  50000         Y

My suggestion is to switch to struct clk_parent_data entirely (for the
mux this could be done with a separate commit before this one).
Then keep it consistent and don't mix parent_names/parent_hws and
parent_data (just stick to parent_data, which can manage the previous
two and more).
I *think* struct clk_parent_data can even be used to simplify the
second patch (by just having an "empty" entry - with index = -1 - in
the array).

[...]
> +               channel->gate.bit_idx = __ffs(meson_pwm_per_channel_data[i].clk_en_mask);
I's the only place where clk_en_mask is now used. So I think it's
valid to just rename clk_en_mask to clk_en_idx and pass the value
without the BIT macro during initialization.


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic