Date:	Thu, 04 Sep 2014 09:06:48 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Bart Tanghe <bart.tanghe@...masmore.be>,
	Thierry Reding <thierry.reding@...il.com>
CC:	tim.kryger@...aro.org, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org
Subject: Re: [resend rfc v3] pwm: add BCM2835 PWM driver

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@...masmore.be>

>>> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists