Message-ID: <CAGb2v67O8xKUfXg2Xr0+G72upPsp6V=OpXeej8T8JeJf5sWk=A@mail.gmail.com>
Date: Wed, 7 Jan 2026 00:58:20 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Richard GENOUD <richard.genoud@...tlin.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.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

On Wed, Jan 7, 2026 at 12:27 AM Uwe Kleine-König
<u.kleine-koenig@...libre.com> wrote:
>
> 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.

IIRC Linus said this is the one case that made sense for "declare anywhere".

I dug up this one:

https://lore.kernel.org/all/CAHk-=wgLYqYcw0xv65xrLSR7KDpS_6M+S9737m6NQorHGWsXYQ@mail.gmail.com/

 - quote -

The actual C99 version is the sane one which actually makes it easier
and clearer to have loop iterators that are clearly just in loop
scope.

That's a good idea in general, and I have wanted to start using that
in the kernel even aside from some of the loop construct macros.
Because putting variables in natural minimal scope is a GoodThing(tm).

 - end quote -

If you look around the kernel, you will see many for loops that are in
this style now.


ChenYu

Powered by blists - more mailing lists