Re: [PATCH V3] clk: palmas: add clock driver for palmas

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Oct 08, 2013 at 03:39:54PM +0100, Laxman Dewangan wrote:
> Thanks Nishanth for review.
> 
> On Tuesday 08 October 2013 06:59 PM, Nishanth Menon wrote:
> > On 10/08/2013 08:21 AM, Laxman Dewangan wrote:
> >> Palmas devices has two clock output CLK32K_KG and CLK32K_KG_AUDIO
> > not all palmas devices have 2 clocks - example: tps659038
> 
> This is for generic palmas and I have seen it for TPS65913, TPS65914, 
> TPS80036. If the generic one is not compatible then it need to add  
> device specific and at that time, it is require to update the binding 
> document accordingly.

We've just been given an example of where it's not compatible.

> 
> >           |    7 +
> >   drivers/clk/Makefile                               |    1 +
> >   drivers/clk/clk-palmas.c                           |  340 ++++++++++++++++++++
> >
> > http://www.spinics.net/lists/devicetree/msg04855.html
> > Do we do 2 patches now? one seperate for binding and implementation?
> > What is our current preference now a days?
> 
> Currently it is implementation + binding doc in one patch.

I believe the general consensus is that separate DT patches are
preferred.

> 
> >
> >>   Palmas device has two clock output pins for 32KHz, KG and KG_AUDIO.
> >> +
> >> +This binding uses the common clock binding ./clock-bindings.txt.
> > proper link would be to provide
> > Documentation/devicetree/bindings/clock/clock-bindings.txt ?
> 
> Hmm, other patch I got feedback from DT maintainers to do not use the 
> absolute path as document directory may change

I'd not seen this, but I don't have a strong preference either way. This
document will go under Documentation/devicetree/bindings/clock, so a
relative path seems Ok.

> 
> >> +
> >> +Clock 32KHz KG is output 0 of the driver and clock 32KHz is output 1.
> >> +
> >> +Required properties:
> >> +- compatible : shall be "ti,palmas-clk".
> > To handle variants of Palmas chips in production, you'd want to be
> > specific here clk32k_kg and clk32k_kg_audio.
> 
> The compatible is the device sub module level, not the clock level. Same 
> thing we are following on regulators.

Yes, we don't need a separate compatible string for each clock within a
block providing those clocks.

> 
> 
> >
> >> +
> >> +     Optional subnode properties:
> >> +     ti,clock-boot-enable: Enable clock at the time of booting.
> > Dumb question: Why is this needed? should'nt relevant drivers do a
> > clk_get to enable the relevant clocks?
> 
> If some board needs this clock to be always available for rest of system 
> to work without any specific driver then this flag is useful.

Do we _actually_ need this right now, or is this hypothetical?

If we don't need it now, remove it. If you think we need it know, please
describe exactly why (i.e. what device needs the clock to work, why does
this affect the rest of the board if we don't ahve a driver for that
device, why don't we just write a driver for that device).

> 
> 
> >
> >> +     ti,external-sleep-control: The clock is enable/disabled by state
> >> +             of external enable input pins ENABLE, ENABLE2 and NSLEEP.
> >> +             The valid value for the external pins are:
> >> +                     1 for ENABLE1
> >> +                     2 for ENABLE2
> >> +                     3 for NSLEEP.

I asked this on the last version (before having noticed this one). What
actually drives those pins to control the clock(s)?

Is this for setting the clock to be controlled by the external pin, or
is the clock hard-wired to a particular pin?

Thanks,
Mark.