Message-ID: <mu3ciykmtxoaa24mdw7mofpdapbii23qrubw6uzptszok43tta@tq3rguupehwe>
Date: Tue, 6 Jan 2026 17:27:34 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Richard GENOUD <richard.genoud@...tlin.com>
Cc: Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org, Joao Schim <joao@...imsalabim.eu>
Subject: Re: [PATCH v2 2/4] pwm: sun50i: Add H616 PWM support

Hello Richard,

On Tue, Jan 06, 2026 at 12:19:59PM +0100, Richard GENOUD wrote:
> Le 24/12/2025 à 10:54, Uwe Kleine-König a écrit :
> > this patch isn't checkpatch clean.
> 
> Yes, I've seen that.
> It's because checkpatch doesn't detect that PWM_XY_SRC_MUX/GATE/DIV are
> structures
> If I unwrap PWM_XY_SRC_MUX/GATE/DIV and PWM_X_DIV, checkpatch doesn't
> complain anymore (indeed, do/while loops are not allowed at file-scope)

At least add parenthesis around usages of _reg. (And if you know that
checkpatch points out things that you don't agree to, preempting the
critic in the cover letter is a good idea.)

> I can drop PWM_XY_SRC_MUX/GATE/DIV PWM_X_DIV and declare the structures
> directly under PWM_XY_CLK_SRC() and PWM_X_CLK() if you prefer, but I
> find it less readable than the current form.

No strong feeling here.
 
> > On Wed, Dec 17, 2025 at 09:25:02AM +0100, Richard Genoud wrote:
> > > +#define PWM_XY_SRC_GATE(_pair, _reg)			\
> > > +struct clk_gate gate_xy_##_pair = {			\
> > > +	.reg = (void *)_reg,				\
> > > +	.bit_idx = PWM_XY_CLK_CR_GATE_BIT,		\
> > > +	.hw.init = &(struct clk_init_data){		\
> > > +		.ops =  &clk_gate_ops,			\
> > > +	}						\
> > 
> > I would consider
> > 
> > 	.hw.init.ops = ...;
> > 
> > and
> > 
> > 	.hw = {
> > 		.init = {
> > 			.ops = ...;
> > 		},
> > 	},
> > 
> > natural here. The middleway you chose looks strange to me.
> 
> Actually, it's:
> 	.hw.init->ops = ... ;
> That's why I used this middleway construct.

Ah I see. Then it's fine for me.

> > > +#define PWM_XY_SRC_MUX(_pair, _reg)			\
> > > +struct clk_mux mux_xy_##_pair = {			\
> > > +	.reg = (void *)_reg,				\
> > > +	.shift = PWM_XY_CLK_CR_SRC_SHIFT,		\
> > > +	.mask = PWM_XY_CLK_CR_SRC_MASK,			\
> > 
> > Huh, why does this structure has both a shift and a mask value? What is
> > the difference between
> > 
> > 	.shift = 7,
> > 	.mask = 1,
> > 
> > and
> > 
> > 	.shift = 0,
> > 	.mask = 1 << 7,
> > 
> > ? If the latter is equivalent, you could just pass
> > H616_PWM_XY_CLK_CR_SRC and get rid of the extra definitions for _SHIFT
> > and _MASK.
> > 
> Unfortunately, struct clk_mux wants a shift and an unshifted mask, and
> according to:
> https://elixir.bootlin.com/linux/v6.18.3/source/drivers/clk/clk-mux.c#L93-L94
> using a 0 shift and mask = 1 << 7 won't work.

How annoying, I put that on my todo list ...

> > > +static inline struct h616_pwm_chip *to_h616_pwm_chip(struct pwm_chip *chip)
> > 
> > It probably doesn't help much, but conceptually this could be
> > 
> > 	static inline struct h616_pwm_chip *to_h616_pwm_chip(const struct pwm_chip *chip)
> > 
> Indeed, I'll add that.

If you rename it to h616_pwm_from_chip it even has the driver specific
prefix.

> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int h616_pwm_calc(struct pwm_chip *chip, unsigned int idx,
> > > +			 const struct pwm_state *state)
> > > +{
> > > +	struct h616_pwm_chip *h616chip = to_h616_pwm_chip(chip);
> > > +	struct h616_pwm_channel *chan = &h616chip->channels[idx];
> > > +	unsigned int cnt, duty_cnt;
> > > +	unsigned long max_rate;
> > > +	long calc_rate;
> > > +	u64 duty, period, freq;
> > > +
> > > +	duty = state->duty_cycle;
> > > +	period = state->period;
> > > +
> > > +	max_rate = clk_round_rate(chan->pwm_clk, UINT32_MAX);
> > 
> > Huh, is this an artificial limitation? Do you rely on how clk_round_rate
> > picks the return value? (I.e. nearest value? nearest time? round up?
> > round down?)
> 
> What I want to achieve here is to handle the lowest possible period case.
> Without the bypass, the lowest period is build with the highest clock,
> duty cycle = 1 and period cycle = 2 (0x10001 in period register)
> So, if the input clock is 100MHz, we have a lowest period of 20ns.
> Now, if we enable the bypass, the period register is ignored and we directly
> have the 100MHz clock as PWM output, that can act as a 10ns period with 5ns
> duty
> So the logic here is to detect this specific lowest period case that can be
> achieved by enabling the bypass.
> For that, the highest clock is retrieved and compared to the wanted period
> and duty.
> 
> Now, I learned the hard way that clk_round_rate() is actually limited to a
> 32 bits value:
> clk_round_rate() calls at one point clk_divider_bestdiv() that uses
> DIV_ROUND_UP_ULL() which in turn uses do_div(n,base):
>   do_div - returns 2 values: calculate remainder and update new dividend
>   @n: uint64_t dividend (will be updated)
>   @base: uint32_t divisor
> So, in order to get the exact highest clock rate, I use clk_round_rate()
> with U32_MAX.

IMHO this should be addressed in the clk framework. And until this
happend the code in the pwm driver needs a verbose comment.
 
> > > +	dev_dbg(pwmchip_parent(chip), "period=%llu cnt=%u duty=%llu duty_cnt=%u\n",
> > > +		period, cnt, duty, duty_cnt);
> > 
> > This is little helpful without the input parameters.
> > 
> as
> 	duty = state->duty_cycle;
> 	period = state->period;
> The input parameters are there, right?
> But I can add the missing idx.

Today I agree, don't know why I missed that. It would be nice to stick
to the format that e.g. pwm-stm32 uses for the input parameters to make
me notice even on busy days that the input parameters are there :-)

> > > +	for (unsigned int i = 0; i < data->npwm; i++) {
> > 
> > Huh, AFAIK we're not using declarations in for loops in the kernel.
>
> Actually, I've read somewhere (in LWN I guess) that Linus seems ok with that,
> but I'll remove it if you prefer.

If you find your source again, I'd be interested.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists