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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
