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

[Martin Blumenstingl]

Hi Xianwei Zhao,

On Thu, Mar 26, 2026 at 7:35 AM Xianwei Zhao via B4 Relay
<devnull+xianwei.zhao.amlogic.com@xxxxxxxxxx> wrote:
>
> From: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
>
> Add support for Amlogic S7 PWM. Amlogic S7 different from the
> previous SoCs, a controller includes one pwm, at the same time,
> the controller has only one input clock source.
>
> Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-meson.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 8c6bf3d49753..3d16694e254e 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -113,6 +113,7 @@ struct meson_pwm_data {
>         int (*channels_init)(struct pwm_chip *chip);
>         bool has_constant;
>         bool has_polarity;
> +       bool single_pwm;
At first I wasn't sure about this and thought we should replace it
with a num_pwms (or similar) variable.
However, I think it will be hard to add a third (or even more)
channels to the PWM controller (not just from driver perspective but
also from hardware perspective). So I think this is good enough as the
choice will only be 1 or 2.

[...]
> +static const struct meson_pwm_data pwm_s7_data = {
> +       .channels_init = meson_pwm_init_channels_s7,
I think you can use .channels_init = meson_pwm_init_channels_s4, if
you change the code inside that function from:
    for (i = 0; i < MESON_NUM_PWMS; i++) {
to:
    for (i = 0; i < chip->npwm; i++) {

[...]
> @@ -650,9 +674,13 @@ static int meson_pwm_probe(struct platform_device *pdev)
>  {
>         struct pwm_chip *chip;
>         struct meson_pwm *meson;
> +       const struct meson_pwm_data *pdata = of_device_get_match_data(&pdev->dev);
>         int err;
>
> -       chip = devm_pwmchip_alloc(&pdev->dev, MESON_NUM_PWMS, sizeof(*meson));
> +       if (pdata->single_pwm)
> +               chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*meson));
> +       else
> +               chip = devm_pwmchip_alloc(&pdev->dev, MESON_NUM_PWMS, sizeof(*meson));
I don't think this code is too bad for now.
However, I'm wondering if you want to make "channels" from struct
meson_pwm a flexible array member in a future patch. In that case it
will be helpful to have an "unsigned int npwm = pdata->single_pwm ? 1
: MESON_NUM_PWMS;" (or similar) variable to future-proof your code.
What do you think?


Best regards,
Martin