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 <[email protected]>
> > > ---
> > > 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
