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

[Xianwei Zhao]

Hi Uwe,
   Thanks for your review.

On 2026/5/18 01:12, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Apr 02, 2026 at 02:40:16AM +0000, Xianwei Zhao via B4 Relay 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, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index 8c6bf3d49753..7a43c42ef3d6 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;
> Conceptually I'd prefer a `npwm` field here. That doesn't take more
> space in memory and simplifies the logic a bit. (At the cost of having
> to adapt all already existing meson_pwm_data instances, but that's fine
> in my book.)

I will use npwm(u8) instead of single_pwm.

> >   };
> >   
> >   struct meson_pwm {
> > @@ -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;
> > +}
> > +
> >   static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
> >   {
> >   	struct device *dev = pwmchip_parent(chip);
> > @@ -592,6 +605,13 @@ static const struct meson_pwm_data pwm_s4_data = {
> >   	.has_polarity = true,
> >   };
> >   
> > +static const struct meson_pwm_data pwm_s7_data = {
> > +	.channels_init = meson_pwm_init_channels_s7,
> > +	.has_constant = true,
> > +	.has_polarity = true,
> > +	.single_pwm = true,
> > +};
> > +
> >   static const struct of_device_id meson_pwm_matches[] = {
> >   	{
> >   		.compatible = "amlogic,meson8-pwm-v2",
> > @@ -642,6 +662,10 @@ 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
> > +	},
> >   	{},
> If you touch that array in the next revision, please make this line:
>
> 	{ }
>
> (I.e. add a space and drop the comma.)

Will do.

> Best regards
> Uwe