Re: [PATCH v4 09/10] pwm: Add PXA support

On Thu, Mar 15, 2012 at 07:56:31AM +0100, Thierry Reding wrote:

* Ryan Mallon wrote:

quoted

On 15/03/12 02:56, Thierry Reding wrote:

quoted

Signed-off-by: Thierry Reding <redacted>
---
Changes in v3:
  - update PWM ops for changes in patch 2

Couple of quick notes, mostly for future work.

quoted

+	/* NOTE: the clock to PWM has to be enabled first
+	 * before writing to the registers
+	 */
+	clk_enable(pc->clk);

Should be fixed to also call clk_prepare (and clk_unprepare after
clk_disable). Could be done in a follow up patch.

quoted

+	__raw_writel(prescale, pc->mmio_base + offset + PWMCR);
+	__raw_writel(dc, pc->mmio_base + offset + PWMDCR);
+	__raw_writel(pv, pc->mmio_base + offset + PWMPCR);

Should we fix this driver to use readl/writel instead of the __raw
variants? The memory is properly ioremaped, and to my understanding the
__raw memory accessors should be avoided outside of core code. This
could be done in a follow up patch if you want to keep this patch as
mostly just a move of the code.

Actually I wasn't planning on keeping this patch at all. Sascha already has
the existing PWM providers converted to his original framework and offered to
rebase them onto this series once the dust settles. I only used them as
testbed to see how the driver interface works out for different hardware. But
I also think that if Sascha hasn't cleaned the driver up yet it should either
be done in his conversion patches or as follow ups.

All I have is a simple conversion to the new framework, nothing more.  I
rebased my patches yesterday onto this series (unfortunately due to the
additional argument pwm_device the patches do not look like just moving
the drivers anymore). I also skipped the PXA patch since I saw that you
already have this one. I just posted the patches to the list.

Sascha: how do you plan on going forward with this? It seems like the driver
interface is pretty much done now and I expect the next round to be the last,
unless I forget to properly work through the TODO list again. If you are busy
with other stuff I can probably find some time to help with porting your
converted drivers.

I haven't done drivers/mfd/twl6030-pwm.c. This one needs a bit more work
as it currently does not register itself as a subdriver to the twl6030
but just uses a globally available twl_i2c_write_u8() function. So if
you have some time to spare it would be great if you could do this.

I'm also wondering which tree this will go in through. Does it make sense to
have an extra tree just for the PWM framework or can it go in via some other
general purpose tree? Who do I need to prod?

Good question, I don't know.

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 |