Re: [resend rfc v3] pwm: add BCM2835 PWM driver

From: Stephen Warren
Date: Thu Sep 04 2014 - 11:06:48 EST


On 09/04/2014 04:05 AM, Bart Tanghe wrote:
No problem. Thanks for the feedback.
I've got some question below.

On 2014-08-25 15:19, Thierry Reding wrote:
Sorry for taking so long to reply to this, I had completely forgotten.

On Mon, Apr 28, 2014 at 02:54:46PM +0200, Bart Tanghe wrote:
Add some better error handling and Device table support
Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt

Signed-off-by: Bart Tanghe <bart.tanghe@xxxxxxxxxxxxx>

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..20341a3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,18 @@ config PWM_ATMEL_TCB
To compile this driver as a module, choose M here: the module
will be called pwm-atmel-tcb.

+config PWM_BCM2835
+ tristate "BCM2835 PWM support"
+ depends on MACH_BCM2835 || MACH_BCM2708
+ help
+ PWM framework driver for BCM2835 controller (raspberry pi)

I think the correct capitalization would be "Raspberry Pi".

+ Only 1 channel is implemented.

How many can it take? Why haven't all been implemented?

BCM2835 can take 2 pwm channels.
I can implement 2 channels but can't physically test the second channel. Is that a problem?

I don't think that's a problem; I would expect the channels to be identical, so testing 1 should be fine.

I notice that you never prepare or enable the clock here. Perhaps this
isn't required because it's always on, but I think you should still call
clk_prepare_enable() here (and clk_disable_unprepare() in .remove()) to
make sure the driver is more portable.
The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by
a clock driver, as mentioned in a previous comment by Stephen Warren.

Any clock programming should be performed by a clock driver. We don't
have one of those upstream yet, mainly because it would rely on talking
to the firmware (running on the VideoCore) to manipulate the clocks, and
we don't have a firmware protocol driver either.

Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver.
The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental.
If anyone has some suggestions?

Oh dear. It sounds like we need at least some form of clock driver for the platform then. I still don't think there's complete documentation for the HW, even though a lot of register docs were published which presumably cover the clock HW? Equally, given that the VC firmware assumes it owns most of the HW, it seems best to manipulate the clocks through the firmware interface rather than directly touching the HW. Unfortunately, I don't believe there's any ABI guarantee on the firmware interface. Perhaps we can get one?

+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "bcrm,pwm-bcm2835", },

s/bcrm/brcm/

Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent with all the other HW on this SoC.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/