Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

[Maxime Ripard]

Hi Icenowy,

On Sun, Oct 16, 2016 at 10:29:17PM +0800, Icenowy Zheng wrote:
> Hi,
> 
> 14.10.2016, 20:58, "Maxime Ripard" <maxime.ripard@xxxxxxxxxxxxxxxxxx>:
> > Hi,
> >
> > On Wed, Oct 12, 2016 at 04:10:03PM +0800, Icenowy Zheng wrote:
> >>  > On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng <icenowy@xxxxxxxx> wrote:
> >>  > > The PWM controller in A31 is different with other Allwinner SoCs, with
> >>  > > a control register per channel (in other SoCs the control register is
> >>  > > shared), and each channel are allocated 16 bytes of address (but only 8
> >>  > > bytes are used.). The register map in one channel is just like a
> >>  > > single-channel A10 PWM controller, however, A31 have a different
> >>  > > prescaler table than other SoCs.
> >>  > >
> >>  > > In order to use the driver for all 4 channels, device nodes should be
> >>  > > created per channel.
> >>  >
> >>  > I think Maxime wants you to support the different register offsets
> >>  > in this driver, and have all 4 channels in the same device (node).
> >>
> >>  I think that will make the code much more complex... And in
> >>  hardware there may also be 4 controllers... as the register is
> >>  aligned at 0x10.
> >
> > You also have to think about it the other way around. This is exposed
> > everywhere as a single device. There may be some undocumented
> > registers hidden somewhere in the memory space of that device. How
> > would you deal with that without touching the device tree?
> 
> Is the reason only they're listed in the one chapter of user manual?

Well, yes, because it is one single device.

> >
> > Exposing this as a single device is the best solution both from the
> > philosophical point of view, but also from a maintainance aspect.
> 
> If we really do so, I will go back to the original patch (pwm-sun6i)
> and merge 4 channels.
> 
> No other PWM block of Allwinner devices uses 4 control registers, and it's
> a significant difference to make it a dedicated driver.
> 
> However, I still think we should have 4 nodes, since the 4 channels can work
> very dedicatedly, with different control register... This can be a reason to see
> them as 4 dedicated controllers.
> 
> (And as PWM uses only oscXX, we cannot judge it according to the clock tree,
> and Occam's Razor will apply to think it's 4 A10-like PWM controller...)
> 
> If we just think it's because it's a whole part, why don't we combine ehci and ohci
> to one driver? Just because we can reuse {e,o}hci-platform...
> 
> It's the same reason to see it as 4 controllers.

Duplicating code is usually not a good idea. In this case, this will
be easier for you to deal with the A31 PWM, but all the rest of the
code (how to enable, disable it, setup the rates, when to enable the
clocks, when to disable them) will be duplicated, even though part of
them are really non-trivial.

Have you looked at reg_field? It really allows you to deal quite
cleanly with those kinds of quirks.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature