Re: [PATCH] pwm: meson: external clock configuration for s4

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

Hello,

I am adding my thoughts in this email which are in addition to Uwe's comments.

Please always Cc the linux-amlogic mailing list when you are sending patches.
Currently this patch cannot be found in the mailing list archives,
patchwork, ...

On Thu, Dec 23, 2021 at 6:58 AM Jiayi Zhou <Jiayi.Zhou@amlogic.com> wrote:
>
> From: "jiayi.zhou" <jiayi.zhou@amlogic.com>
>
> For PWM controller in the Meson-S4 SoC,
> PWM needs to obtain an external clock source.
> This patch tries to describe them in the DT compatible data.
I think this patch does more than what is described here.
It adds support for the constant high/low output, which is a feature
which I think has been introduced on the G12A SoC. I suggest having a
separate patch for this and also enabling it on supported SoCs (not
just the S4).

[...]
> +#define PWM_DISABLE            0
I don't see this being used anywhere so I think it can be dropped

[...]
> +       /*
> +        * duty_cycle equal 0% and 100%,constant should be enabled,
> +        * high and low count will not incease one;
typo: incease -> increase

[...]
> +       if (meson->data->extern_clk) {
> +               set_clk = clk_get_rate(channel->clk);
> +               if (set_clk == 0)
> +                       dev_err(meson->chip.dev, "invalid source clock frequency\n");
> +               set_clk /= (channel->pre_div + 1);
> +               err = clk_set_rate(channel->clk, set_clk);
> +               if (err)
> +                       dev_err(meson->chip.dev, "%s: error in setting pwm rate!\n", __func__);
> +       }
> +
>         value = readl(meson->base + REG_MISC_AB);
>         value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift);
>         value |= channel->pre_div << channel_data->clk_div_shift;
Why does this new controller version require clk_set_rate with a rate
that's calculated based on the pre-divider and then also configuring
the pre-divider in the MISC_AB register?
This seems odd, can't we use only one of them (either clk_set_rate or
setting MISC_AB)?

Also if clk_set_rate is the way to go here I suggest refactoring the
existing code so the pre-divider is a clock as well so we can use
clk_set_rate for both, the "old" and "new" IP version.

[...]
> +static const struct meson_pwm_data pwm_meson_data = {
> +       .extern_clk = true,
>  };

[...]
> +static int meson_pwm_v2_init_channels(struct meson_pwm *meson)
> +{
> +       struct meson_pwm_channel *channels = meson->channels;
> +       struct device *dev = meson->chip.dev;
> +       unsigned int i;
> +       char name[255];
> +
> +       for (i = 0; i < (meson->chip.npwm / 2); i++) {
> +               snprintf(name, sizeof(name), "clkin%u", i);
> +               (channels + i)->clk = devm_clk_get(dev, name);
> +               if (IS_ERR((channels + i)->clk)) {
> +                       dev_err(meson->chip.dev, "can't get device clock\n");
> +                       return PTR_ERR((channels + i)->clk);
> +               }
> +               (channels + i + 2)->clk = (channels + i)->clk;
> +       }
I think this is whole function is broken.
The vendor driver uses four channels per PWM controller on newer SoCs.
In the mainline driver we only support two PWM channels per controller.

Now the whole loop starts with npwm / 2 which is always 1 in the
mainline driver.
This is the first problem: this would never initialize the second PWM.
The second problem is that "channels + i + 2" would access channels[0
+ 2] which is out of bounds for this array (that array only holds two
values so only index 0 and 1 are valid).


Best regards,
Martin

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