Re: [PATCH v2 1/3] pwm: jz4740: Use clocks from TCU driver

Uwe Kleine-König
2020-02-11T16:25:41Z

Hi Paul,

On Mon, Nov 18, 2019 at 02:42:54PM +0100, Paul Cercueil wrote:
> Le lun., nov. 18, 2019 at 12:19, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Mon, Nov 18, 2019 at 11:55:56AM +0100, Paul Cercueil wrote:
> > > Le lun., nov. 18, 2019 at 08:15, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > On Sun, Nov 17, 2019 at 11:58:43PM +0100, Paul Cercueil wrote:
> > > > > Le dim., nov. 17, 2019 at 21:20, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> a écrit :
> > > > > > On Sat, Nov 16, 2019 at 06:36:11PM +0100, Paul Cercueil wrote:
> > > > > > > struct jz4740_pwm_chip {
> > > > > > > struct pwm_chip chip;
> > > > > > > - struct clk *clk;
> > > > > >
> > > > > > What is the motivation to go away from this approach to store the clock?
> > > > >
> > > > > It's actually not the same clock. Instead of obtaining "ext" clock from the
> > > > > probe, we obtain "timerX" clocks (X being the PWM channel) from the request
> > > > > callback.
> > > >
> > > > Before you used driver data and container_of to get it, now you used
> > > > pwm_set_chip_data. I wondered why you changed the approach to store
> > > > data. That the actual data is different now is another thing (and
> > > > obviously ok).
> > >
> > > Thierry suggested it: https://lkml.org/lkml/2019/3/4/486
> >
> > If you motivate that in the commit log (preferably with a better
> > rationale than "Thierry suggested it") that's fine for. (Do I claim now
> > without having read the rationale :-)
>
> I don't really have a better rationale. The alternative was to have a
> "struct clk[NB_PWMS];" in the struct jz4740_pwm_chip, so this is arguably
> better. I'm not sure it's worth mentioning in the commit message, is it?

It depends what you want to achieve. An array in the chip data might be
too big, with per-pwm data your memory fragments. Still worth to mention
something like:

The new code now uses a clk pointer per PWM (instead of a clk
per pwm-chip before). So the pointer is stored in per-pwm data
now.

> They are actually all in the same register range. Each channel has 4 32-bit
> registers, the first one is the CTRL (aka. TCSR) register which is written
> to here. The following two configure the duty/period values, the last one is
> the counter. The 'timer enable' bit is however in the global TCU registers
> area.
>
> The clock bits of the TCSRs registers (including the TCSR registers of the
> watchdog and 64-bit OS timer) are controlled by the clocks driver. All
> register accesses are properly handled thanks to regmap, that we add in
> patch [2/3].

You reshuffled in v3 which obsoletes some of the statements here I
think. Will take a look there.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |