On Mon, Jul 04, 2011 at 08:43:23PM +1000, Ryan Mallon wrote:

On 04/07/11 17:55, Sascha Hauer wrote:

quoted

I am tired of discussing this. It seems we can't agree and unless
someone else jumps in here we will probably have to wait for another
year until something moves in the PWM area.

If we are going to introduce a new framework for pwms then we should
create one which meets the needs of at least all of the in kernel
drivers. This patch series provides no solution for either the atmel or
ep93xx drivers, so it is not a complete solution. At some point the
api/framework _must_ be changed. If we can introduce transition layers
then we should do that now so they we can provide a common framework
along with some forward thinking about how the other drivers are going
to be migrated to the new framework. This patch series doesn't even
migrate _any_ of the existing drivers.

It doesn't have to be an all or nothing approach. Possibly Bill's series
is perhaps too involved by changing the api, introducing sysfs support
and reworking the pwm users. But your series is at the opposite end of
the spectrum. It does too little. It will take a few release cycles to
get all of the existing drivers migrated and since we can't change the
api until that happens the atmel and ep93xx drivers will take longer
still. At the very least your series should migrate some of the drivers.

The timeline argument is a bit poor. Yes, there has been discussion for
a lengthy time about how the pwm api should be developed, but I think
that is because it is non-trivial to come up with a framework which is
good enough to support all of the pwm hardware (some of which is already
in the mainline). Getting something merged now just because it can be
done quickly is not a good idea if it all has to get reworked in the
future so that it can support all the hardware.

The pwm framework needs to incorporate at least the following:
 - sysfs access (ep93xx driver)

The sysfs interface will likely raise smoe more discussion, that's why I
intentionally have no support for it. It can be added later, but right
now I see no reason why we should add artificial barriers to merge these
patches.

 - Multiple channels per device (atmel driver)

Again, this can be added later.

The mxs driver you introduce looks like it could be implemented as a
single device (continuous mmio space) with multiple channels rather than
the pwm core/driver approach you have. I also can't see anything in this
patch set which hooks up the mxs pwms to an actual board (i.e. nothing
calls mxs_add_mxs_pwm)?

Why should anyone register a device for which no driver is in the tree?

The other nice things to have for the pwm framework are:
 - More fine grained control of pwms: pwm_period_ns, period_duty_ns, etc
 - Polarity control
 - Synchronisation support for multi-channel devices
 - Interrupt handler support
 - Sleeping vs non-sleeping configuration api

The fine-grained control api could be added now. pwm_config could be
left as is for the time being (the new api could be a wrapper around it
to start with). Polarity control and interrupt handling apis could also
be defined without affecting the drivers which don't need to implement
them. Multiple channels and the sleeping/non-sleeping api are the more
difficult ones, but at least having some sort of indication about how
these plan to be solved would be useful.

Again, why should we add these *now*? It only raises the chance that
there's more discussion.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |