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

[Uwe Kleine-König]

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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/ |