| Message ID | 1386244271-16127-1-git-send-email-andrew@lunn.ch |
|---|---|
| State | Superseded, archived |
| Headers | show |
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@lunn.ch> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lunn.ch> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lunn.ch> > > > --- > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>; }
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@lunn.ch> --- 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(+)