[PATCH] DT: ATA: Add missing documentation of clocks to marvell binding.

[PATCH] DT: ATA: Add missing documentation of clocks to marvell binding.

[Andrew Lunn]

The marvell SATA driver can optionally make use of clocks specified in
the DT node. This has been available in the driver since Febuary 2012,
but the documentation is missing from the binding. Add it.

Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
---
FYI: This will cause merge conflicts with the SATA PHY driver comming soon.
---
 Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
index b5cdd20cde9c..f6c68d157749 100644
--- a/Documentation/devicetree/bindings/ata/marvell.txt
+++ b/Documentation/devicetree/bindings/ata/marvell.txt
@@ -6,11 +6,17 @@ Required Properties:
 - interrupts    : Interrupt controller is using
 - nr-ports      : Number of SATA ports in use.
 
+Optional Properties:
+- clocks        : List of pHandles to clocks
+- clock-names   : Must be "0", "1", mapping port number to clock.
+
 Example:
 
 	sata@80000 {
 		compatible = "marvell,orion-sata";
 		reg = <0x80000 0x5000>;
 		interrupts = <21>;
+		clocks = <&gate_clk 14>, <&gate_clk 15>;
+		clock-names = "0", "1";
 		nr-ports = <2>;
 	}
-- 
1.8.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DT: ATA: Add missing documentation of clocks to marvell binding.

[Mark Rutland]

On Thu, Dec 05, 2013 at 11:51:11AM +0000, Andrew Lunn wrote:
> The marvell SATA driver can optionally make use of clocks specified in
> the DT node. This has been available in the driver since Febuary 2012,
> but the documentation is missing from the binding. Add it.
> 
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> ---
> FYI: This will cause merge conflicts with the SATA PHY driver comming soon.
> ---
>  Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> index b5cdd20cde9c..f6c68d157749 100644
> --- a/Documentation/devicetree/bindings/ata/marvell.txt
> +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> @@ -6,11 +6,17 @@ Required Properties:
>  - interrupts    : Interrupt controller is using
>  - nr-ports      : Number of SATA ports in use.
>  
> +Optional Properties:
> +- clocks        : List of pHandles to clocks

s/pHandle/phandle/. Don't forget the clock-specifier too!

> +- clock-names   : Must be "0", "1", mapping port number to clock.

This line leads to the erroneous impression that the clocks can be in
arbitrary order (as is generally true for bindings with clock-names
properties). The driver doesn't request clocks by name, and doesn't even
look at clock-names.

Please document the required order in the description of the clocks
property. If you want to add clock-names, please add support to the
driver or it's somewhat pointless.

I also think the names could be improved, albeit slightly ("port0" is a
little clearer than "0").

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DT: ATA: Add missing documentation of clocks to marvell binding.

[Andrew Lunn]

On Thu, Dec 05, 2013 at 12:22:54PM +0000, Mark Rutland wrote:
> On Thu, Dec 05, 2013 at 11:51:11AM +0000, Andrew Lunn wrote:
> > The marvell SATA driver can optionally make use of clocks specified in
> > the DT node. This has been available in the driver since Febuary 2012,
> > but the documentation is missing from the binding. Add it.
> > 
> > Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> > ---
> > FYI: This will cause merge conflicts with the SATA PHY driver comming soon.
> > ---
> >  Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> > index b5cdd20cde9c..f6c68d157749 100644
> > --- a/Documentation/devicetree/bindings/ata/marvell.txt
> > +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> > @@ -6,11 +6,17 @@ Required Properties:
> >  - interrupts    : Interrupt controller is using
> >  - nr-ports      : Number of SATA ports in use.
> >  
> > +Optional Properties:
> > +- clocks        : List of pHandles to clocks
> 
> s/pHandle/phandle/. Don't forget the clock-specifier too!
> 
> > +- clock-names   : Must be "0", "1", mapping port number to clock.
> 
> This line leads to the erroneous impression that the clocks can be in
> arbitrary order (as is generally true for bindings with clock-names
> properties). The driver doesn't request clocks by name, and doesn't even
> look at clock-names.

The driver does:

                sprintf(port_number, "%d", port);
                hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);

and:

153 struct clk *clk_get(struct device *dev, const char *con_id)
154 {
155         const char *dev_id = dev ? dev_name(dev) : NULL;
156         struct clk *clk;
157 
158         if (dev) {
159                 clk = of_clk_get_by_name(dev->of_node, con_id);
160                 if (!IS_ERR(clk) && __clk_get(clk))
161                         return clk;
162         }
163 
164         return clk_get_sys(dev_id, con_id);
165 }

So the names are used. And since names are used, the ordering is
arbitrary.

> I also think the names could be improved, albeit slightly ("port0" is a
> little clearer than "0").

I could do this, but this binding has been in use for well over a year
now. I'm just retro-actively documenting it. I can add support for
both "0" and "port0", in order to keep backwards compatibility, but it
obviously makes the driver messy. Is it worth it?

Thanks
	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] DT: ATA: Add missing documentation of clocks to marvell binding.

[Andrew Lunn]

The marvell SATA driver can optionally make use of clocks specified in
the DT node. This has been available in the driver since Febuary 2012,
but the documentation is missing from the binding. Add it.

Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
---
FYI: This will cause merge conflicts with the SATA PHY driver comming soon.

v1->v2:
s/pHandle/phandle/
Add specifier.

Not addressed yet is changing "0" to "port0", etc. Waiting for feedback
from Mark Rutland.
---
 Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
index b5cdd20cde9c..acfdb439bb6c 100644
--- a/Documentation/devicetree/bindings/ata/marvell.txt
+++ b/Documentation/devicetree/bindings/ata/marvell.txt
@@ -6,11 +6,17 @@ Required Properties:
 - interrupts    : Interrupt controller is using
 - nr-ports      : Number of SATA ports in use.
 
+Optional Properties:
+- clocks        : List of phandle and specifier to clocks
+- clock-names   : Must be "0", "1", mapping port number to clock.
+
 Example:
 
 	sata@80000 {
 		compatible = "marvell,orion-sata";
 		reg = <0x80000 0x5000>;
 		interrupts = <21>;
+		clocks = <&gate_clk 14>, <&gate_clk 15>;
+		clock-names = "0", "1";
 		nr-ports = <2>;
 	}
-- 
1.8.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DT: ATA: Add missing documentation of clocks to marvell binding.

[Mark Rutland]

On Thu, Dec 05, 2013 at 12:50:03PM +0000, Andrew Lunn wrote:
> On Thu, Dec 05, 2013 at 12:22:54PM +0000, Mark Rutland wrote:
> > On Thu, Dec 05, 2013 at 11:51:11AM +0000, Andrew Lunn wrote:
> > > The marvell SATA driver can optionally make use of clocks specified in
> > > the DT node. This has been available in the driver since Febuary 2012,
> > > but the documentation is missing from the binding. Add it.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> > > ---
> > > FYI: This will cause merge conflicts with the SATA PHY driver comming soon.
> > > ---
> > >  Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> > > index b5cdd20cde9c..f6c68d157749 100644
> > > --- a/Documentation/devicetree/bindings/ata/marvell.txt
> > > +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> > > @@ -6,11 +6,17 @@ Required Properties:
> > >  - interrupts    : Interrupt controller is using
> > >  - nr-ports      : Number of SATA ports in use.
> > >  
> > > +Optional Properties:
> > > +- clocks        : List of pHandles to clocks
> > 
> > s/pHandle/phandle/. Don't forget the clock-specifier too!
> > 
> > > +- clock-names   : Must be "0", "1", mapping port number to clock.
> > 
> > This line leads to the erroneous impression that the clocks can be in
> > arbitrary order (as is generally true for bindings with clock-names
> > properties). The driver doesn't request clocks by name, and doesn't even
> > look at clock-names.
> 
> The driver does:
> 
>                 sprintf(port_number, "%d", port);
>                 hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);
> 

Apologies, I'd misread the code.

> and:
> 
> 153 struct clk *clk_get(struct device *dev, const char *con_id)
> 154 {
> 155         const char *dev_id = dev ? dev_name(dev) : NULL;
> 156         struct clk *clk;
> 157 
> 158         if (dev) {
> 159                 clk = of_clk_get_by_name(dev->of_node, con_id);
> 160                 if (!IS_ERR(clk) && __clk_get(clk))
> 161                         return clk;
> 162         }
> 163 
> 164         return clk_get_sys(dev_id, con_id);
> 165 }
> 
> So the names are used. And since names are used, the ordering is
> arbitrary.
> 
> > I also think the names could be improved, albeit slightly ("port0" is a
> > little clearer than "0").
> 
> I could do this, but this binding has been in use for well over a year
> now. I'm just retro-actively documenting it. I can add support for
> both "0" and "port0", in order to keep backwards compatibility, but it
> obviously makes the driver messy. Is it worth it?

No, there's no point adding more names. Apologies for the confusion.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] DT: ATA: Add missing documentation of clocks to marvell binding.

[Mark Rutland]

On Thu, Dec 05, 2013 at 03:50:19PM +0000, Andrew Lunn wrote:
> The marvell SATA driver can optionally make use of clocks specified in
> the DT node. This has been available in the driver since Febuary 2012,
> but the documentation is missing from the binding. Add it.
> 
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> ---
> FYI: This will cause merge conflicts with the SATA PHY driver comming soon.
> 
> v1->v2:
> s/pHandle/phandle/
> Add specifier.
> 
> Not addressed yet is changing "0" to "port0", etc. Waiting for feedback
> from Mark Rutland.

I'm happy with the clock-names as they are, thanks for applying the
other changes.

Thanks for fixing up the documentation!

Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

> ---
>  Documentation/devicetree/bindings/ata/marvell.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> index b5cdd20cde9c..acfdb439bb6c 100644
> --- a/Documentation/devicetree/bindings/ata/marvell.txt
> +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> @@ -6,11 +6,17 @@ Required Properties:
>  - interrupts    : Interrupt controller is using
>  - nr-ports      : Number of SATA ports in use.
>  
> +Optional Properties:
> +- clocks        : List of phandle and specifier to clocks
> +- clock-names   : Must be "0", "1", mapping port number to clock.
> +
>  Example:
>  
>  	sata@80000 {
>  		compatible = "marvell,orion-sata";
>  		reg = <0x80000 0x5000>;
>  		interrupts = <21>;
> +		clocks = <&gate_clk 14>, <&gate_clk 15>;
> +		clock-names = "0", "1";
>  		nr-ports = <2>;
>  	}
> -- 
> 1.8.5
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html