Re: [PATCH v4 3/4] pwm: meson: use bulk API for per-channel clocks - Uwe Kleine-König

From: "Uwe Kleine-König" <ukleinek@kernel.org>
To: Alexandre Mergnat <amergnat@baylibre.com>
Cc: Xianwei Zhao <xianwei.zhao@amlogic.com>,
	Rob Herring <robh@kernel.org>,
	 Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 Heiner Kallweit <hkallweit1@gmail.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	 Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	 Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
	 linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	 linux-amlogic@lists.infradead.org
Subject: Re: [PATCH v4 3/4] pwm: meson: use bulk API for per-channel clocks
Date: Mon, 7 Sep 2026 20:44:56 +0200	[thread overview]
Message-ID: <ap8FuWnIRVgcP98G@monoceros> (raw)
In-Reply-To: <178879456435.2256446.2005394820666057840.b4-review@b4>

[-- Attachment #1.1: Type: text/plain, Size: 2679 bytes --]

Hello,

On Mon, Sep 07, 2026 at 05:22:44PM +0200, Alexandre Mergnat wrote:
> > The S4 PWM driver currently gets each channel clock individually with
> > of_clk_get() and uses a custom cleanup action to release the clocks.
> > 
> > Use devm_clk_bulk_get_all() instead to retrieve all per-channel clocks
> > at once. Verify that the number of clocks matches the number of PWM
> > channels and assign each clock to its corresponding channel.
> > 
> > This also allows the per-channel clock initialization code to be shared
> > by Meson PWM variants using one clock per channel.
> > 
> > Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> >
> > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> > index 22cefc6d5dd0..6f151464d7cc 100644
> > --- a/drivers/pwm/pwm-meson.c
> > +++ b/drivers/pwm/pwm-meson.c
> > @@ -497,33 +497,25 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
> >  	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
> >  }
> >  
> > -static void meson_pwm_s4_put_clk(void *data)
> > -{
> > -	struct clk *clk = data;
> > -
> > -	clk_put(clk);
> > -}
> > -
> > -static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
> > +static int meson_pwm_init_channels_per_channel_clk(struct pwm_chip *chip)
> >  {
> >  	struct device *dev = pwmchip_parent(chip);
> > -	struct device_node *np = dev->of_node;
> >  	struct meson_pwm *meson = to_meson_pwm(chip);
> > -	int i, ret;
> > +	struct clk_bulk_data *clks;
> > +	unsigned int i;
> > +	int num;
> >  
> > -	for (i = 0; i < chip->npwm; i++) {
> > -		meson->channels[i].clk = of_clk_get(np, i);
> > -		if (IS_ERR(meson->channels[i].clk))
> > -			return dev_err_probe(dev,
> > -					     PTR_ERR(meson->channels[i].clk),
> > -					     "Failed to get clk\n");
> > -
> > -		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
> > -					       meson->channels[i].clk);
> > -		if (ret)
> > -			return dev_err_probe(dev, ret,
> > -					     "Failed to add clk_put action\n");
> > -	}
> > +	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",
> 
> Small style nit: the continuation arguments are not aligned with the open
> parenthesis, whereas the dev_err_probe() just above and the rest of the driver
> do align them. Can you fix it please ?

No need to resend for that, I can fix that up while applying (after
checking I don't have further concerns).

Thanks Alex for your review!

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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