Re: [PATCH v3 2/2] pwm: meson: Add support for Amlogic S7

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Alexandre,
    Thanks for your review.

On 2026/8/18 20:53, Alexandre Mergnat wrote:
> On Thu, 21 May 2026 08:26:59 +0000, Xianwei Zhao<xianwei.zhao@amlogic.com>  wrote:
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 8c6bf3d49753..66c41bf036de 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -503,6 +504,18 @@ static void meson_pwm_s4_put_clk(void *data)
>>        clk_put(clk);
>>   }
>>
>> +static int meson_pwm_init_channels_s7(struct pwm_chip *chip)
>> +{
>> +     struct device *dev = pwmchip_parent(chip);
>> +     struct meson_pwm *meson = to_meson_pwm(chip);
>> +
>> +     meson->channels[0].clk = devm_clk_get(dev, NULL);
>> +     if (IS_ERR(meson->channels[0].clk))
>> +             return dev_err_probe(dev, PTR_ERR(meson->channels[0].clk),
>> +                                  "Failed to get clk\n");
>> +     return 0;
>> +}
> This adds a fourth channels_init callback, and after the series
> pwm_s7_data ends up differing from pwm_s4_data only by .npwm and by this
> function. Would it make sense to share a single init between the two
> instead?
> 
> What blocks reusing meson_pwm_init_channels_s4() as it stands is that it
> needs an indexed clock lookup, and there is no devm variant of clk_get()
> taking an index, hence the of_clk_get() + devm_add_action_or_reset()
> dance. devm_clk_bulk_get_all() (include/linux/clk.h) is however both
> devm-managed and index-based, and of_clk_bulk_get_all() is implemented as
> clks[i].clk = of_clk_get(np, i) (drivers/clk/clk-bulk.c), so it performs
> exactly the lookup the S4 path already does, only with automatic cleanup.
> Something along these lines:
> 
>   static int meson_pwm_init_channels_per_channel_clk(struct pwm_chip *chip)
>   {
>        struct device *dev = pwmchip_parent(chip);
>        struct meson_pwm *meson = to_meson_pwm(chip);
>        struct clk_bulk_data *clks;
>        unsigned int i;
>        int num;
> 
>        num = devm_clk_bulk_get_all(dev, &clks);
>        if (num < 0)
>                return dev_err_probe(dev, num, "Failed to get clocks\n");
> 
>        if (num != chip->npwm)
>                return dev_err_probe(dev, -EINVAL,
>                                     "expected %u clocks, got %d\n",
>                                     chip->npwm, num);
> 
>        for (i = 0; i < chip->npwm; i++)
>                meson->channels[i].clk = clks[i].clk;
> 
>        return 0;
>   }
> 
> chip->npwm is usable here because devm_pwmchip_alloc() fills it in probe
> before channels_init() runs, so one function covers npwm = 2 and npwm = 1
> with no variant-specific code, and meson_pwm_s4_put_clk() goes away with
> it. A future variant would then only need its .npwm value.
> 
> The count check is worth keeping: of_clk_get(np, i) currently fails probe
> with a clear message when the DT node has fewer clocks than expected,
> while devm_clk_bulk_get_all() would simply return fewer clocks and leave
> channels[i].clk NULL. As clk_prepare_enable(NULL) succeeds and
> clk_round_rate(NULL, ...) returns 0, the problem would only surface later
> in .apply() as a confusing "invalid source clock frequency". The check
> just preserves the diagnostic you have today.
> 
> There is a similar use of this API, count check included, in
> drivers/pmdomain/amlogic/meson-ee-pwrc.c, in case it is useful as a
> reference.
> 
> Since this touches the existing S4 path, it would probably be easier to
> review split up, roughly:
> 
>     1. dt-bindings (unchanged, keeps its Reviewed-by tags)
>     2. pwm: meson: make the PWM count driver data -- add .npwm, fill the
>        existing entries, use it in probe, and switch the loop in
>        meson_pwm_init_clocks_meson8b() to chip->npwm. No functional change.
>     3. pwm: meson: get per-channel clocks with devm_clk_bulk_get_all() --
>        convert and rename meson_pwm_init_channels_s4(), drop
>        meson_pwm_s4_put_clk(). No functional change.
>     4. pwm: meson: Add support for Amlogic S7 -- reduced to pwm_s7_data
>        with .npwm = 1 and the of_device_id entry.
> 
> That is clearly more work than what you have, so please do push back if
> you think it is not worth it for this series. And if you are keeping a
> separate function on purpose because devm_clk_get() is where you would
> like the driver to head, that is a perfectly good answer too -- I may
> well be missing context here.
> 

Will do.

>> @@ -530,6 +543,7 @@ static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>        .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>        .channels_init = meson_pwm_init_channels_meson8b_legacy,
>> +     .npwm = MESON_NUM_PWMS,
> I suggest use "2" instead of "MESON_NUM_PWMS"
> 

Will do.

>> @@ -642,7 +672,11 @@ static const struct of_device_id meson_pwm_matches[] = {
>>                .compatible = "amlogic,meson-s4-pwm",
>>                .data = &pwm_s4_data
>>        },
>> -     {},
>> +     {
>> +             .compatible = "amlogic,s7-pwm",
>> +             .data = &pwm_s7_data
>> +     },
>> +     { }
> Small nit: changing the sentinel from "{}," to "{ }" is unrelated to
> adding S7 support. Patches tend to be easier to review when they carry only
> the functional change, so would you mind dropping it, or splitting it into
> its own trivial cleanup?

Will add commit msg fot this change.

> 
> --
> Alexandre Mergnat<amergnat@baylibre.com>