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

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:

quoted

On Thu, Dec 05, 2013 at 11:51:11AM +0000, Andrew Lunn wrote:

quoted

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 <redacted>
---
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!

quoted

+- 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.

quoted

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