Re: [PATCH 1/3] clk: shmobile: Add DIV6 clock support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kumar,

Thank you for the review.

On Tuesday 29 October 2013 18:33:00 Kumar Gala wrote:
> On Oct 29, 2013, at 9:55 AM, Laurent Pinchart wrote:
> > DIV6 clocks are divider gate clocks controlled through a single
> > register. The divider is expressed on 6 bits, hence the name, and can
> > take values from 1/1 to 1/64.
> > 
> > Those clocks are found on Renesas ARM SoCs.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > .../bindings/clock/renesas,cpg-div6-clocks.txt     |  27 +++
> > drivers/clk/shmobile/Makefile                      |   4 +-
> > drivers/clk/shmobile/clk-div6.c                    | 185 +++++++++++++++++
> > 3 files changed, 215 insertions(+), 1 deletion(-)
> > create mode 100644
> > Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > create mode 100644 drivers/clk/shmobile/clk-div6.c
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt new
> > file mode 100644
> > index 0000000..8036f3c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> > @@ -0,0 +1,27 @@
> > +* Renesas CPG DIV6 Clock
> > +
> > +The CPG DIV6 clocks are variable factor clocks provided by the Clock
> > Pulse
> > +Generator (CPG). They clock input is divided by a configurable factor
> > from 1
> > +to 64.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must be one of the following
> > +    - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
> > +    - "renesas,cpg-div6-clock" for generic DIV6 clocks
> > +  - reg: Base address and length of the memory resource used by the DIV6
> > clock
> > +  - clocks: Reference to the parent clock
> > +  - #clock-cells: Must be 0
> > +  - clock-output-name: The name of the clock as a free-form string
> 
> Is this suppose to be 'clock-output-names' (missing a 's')?

Yes it is, my bad. I'll fix that.

> > +
> > +
> > +Example
> > +-------
> > +
> > +	sd2_clk: sd2_clk {
> 
> example should be sd2_clk@e6150078

You're absolutely right.

> > +		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-
clock";
> > +		reg = <0 0xe6150078 0 4>;
> 
> curious, do you really have a second register at 0?

No, but the r8a7790 has #address-cells and #size-cells set to 2.

> > +		clocks = <&pll1_div2_clk>;
> > +		#clock-cells = <0>;
> > +		clock-output-names = "sd2";
> > +	};

-- 
Regards,

Laurent Pinchart