linux-kernel - Re: [PATCH V2] clk: palmas: add clock driver for palmas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20131008141512.GE1412@e106331-lin.cambridge.arm.com>
Date:	Tue, 8 Oct 2013 15:15:12 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	"mturquette@...aro.org" <mturquette@...aro.org>,
	"broonie@...aro.org" <broonie@...aro.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"rob@...dley.net" <rob@...dley.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"swarren@...dia.com" <swarren@...dia.com>
Subject: Re: [PATCH V2] clk: palmas: add clock driver for palmas

On Mon, Oct 07, 2013 at 03:05:51PM +0100, Laxman Dewangan wrote:
> Palmas devices has two clock output CLK32K_KG and CLK32K_KG_AUDIO
> which can be nebale/disable through software.
> 
> Add clock driver support for the Palmas 32k clocks.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
> ---
> Changes from V1:
> - Call prepare if the clk is needed for boot-enable or sleep control.
> - change is_enabled to is_prepared.
> - Added ops palmas_clks_recalc_rate.
> - Added of_clk_add_provider().
> 
>  .../devicetree/bindings/clock/clk-palmas.txt       |   45 +++
>  drivers/clk/Kconfig                                |    7 +
>  drivers/clk/Makefile                               |    2 +
>  drivers/clk/clk-palmas.c                           |  336 ++++++++++++++++++++
>  4 files changed, 390 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/clk-palmas.txt
>  create mode 100644 drivers/clk/clk-palmas.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/clk-palmas.txt b/Documentation/devicetree/bindings/clock/clk-palmas.txt
> new file mode 100644
> index 0000000..c247538
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clk-palmas.txt
> @@ -0,0 +1,45 @@
> +* Palmas 32KHz clocks *
> +
> +Palmas device has two clock output pins for 32KHz, KG and KG_AUDIO.
> +
> +This binding uses the common clock binding ./clock-bindings.txt.
> +
> +Clock 32KHz KG is output 0 of the driver and clock 32KHz is output 1.
> +
> +Required properties:
> +- compatible : shall be "ti,palmas-clk".
> +- #clock-cells : from common clock binding; shall be set to 1.

It would make sense to describe the value values here, rather than
above.

> +
> +Optional subnode:
> +       The clock node has optional subnode to provide the init configuration of
> +       clocks like boot enable, sleep control.
> +
> +       The subnode name is fixed and it should be
> +               clk32k_kg for the 32KHz KG clock.
> +               clk32k_kg_audio for the 32KHz KG_AUDIO clock.
> +
> +       Optional subnode properties:
> +       ti,clock-boot-enable: Enable clock at the time of booting.

Why is this needed?

If drivers need clocks, they should request them. If they don't
currently, that's a bug in the driver.

> +       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.
> +               Option 0 or missing this property means the clock is
> +               enabled/disabled via register access and these pins do
> +               not have any control.

What actually drives these pins to control the clock?

> +
> +Example:
> +       clock {
> +               compatible = "ti,palmas-clk";
> +               #clock-cells = <1>;
> +               clk32k_kg {
> +                       ti,clock-boot-enable;
> +                       ti,external-sleep-control = <3>;
> +               };
> +
> +               clk32k_kg_audio {
> +                       ti,clock-boot-enable;
> +               };
> +       };

How does the palmas-clk driver control these? No description of
registers, gpio, or any other component that could be used for control
is dfined.

Is the palmas-clk intended to be a subnode of the palmas mfd (the
palmas-rtc binding example suggests this)? It seems odd to describe
components of a device separately with no linkage between them.

I also note that the palmas MFD appears to be an I2C slave, but the
binding doesn't define that (which makes it somewhat confuding for
someone unfamiliar with the hardware).

Thanks,
Mark.
--
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/