On Mon, Oct 14, 2013 at 09:19:37PM +0400, Valentine wrote:
> Hi Mark,
> thanks for looking at this.
>
> On 10/14/2013 08:13 PM, Mark Rutland wrote:
> >On Mon, Oct 14, 2013 at 04:42:33PM +0100, Valentine Barshak wrote:
> >>This converts the R-Car SATA DT compatibility string to
> >>the <unit>-<soc> format which is the preferred one for
> >>all SH-Mobile devices. The DT bindings are documented.
> >>
> >>Signed-off-by: Valentine Barshak <valentine.barshak@xxxxxxxxxxxxxxxxxx>
> >>---
> >> Documentation/devicetree/bindings/ata/sata_rcar.txt | 16 ++++++++++++++++
> >> arch/arm/boot/dts/r8a7779.dtsi | 2 +-
> >> drivers/ata/sata_rcar.c | 2 +-
> >> 3 files changed, 18 insertions(+), 2 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>
> >>diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>new file mode 100644
> >>index 0000000..2465183
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>@@ -0,0 +1,16 @@
> >>+* Renesas R-Car SATA
> >>+
> >>+Required properties:
> >>+- compatible : must be "renesas,sata-r8a7779"
> >
> >s/must be/should include/
> >
>
> OK, thanks.
>
> >>+- reg : address range of the SATA registers.
> >
> >Also their size...
>
> Do you want me to rephrase it to "base address and size of the SATA
> registers"?
>
> Looks like a lot of other bindings examples available in
> Documentation/devicetree/bindings describe regs property
> as the "address range" which implies the address and size
> of the registers.
>
> >
> >>+- interrupt-parent : interrupt parent controller phandle
> >
> >This isn't required as such, as the interrupt-controller can be
> >inherited from the parent. As it's a standard auxiliary property I don't
> >think it needs to be documented here.
>
> OK, will drop it, thanks.
>
> >
> >>+- interrupts : must consist of one interrupt specifier.
> >
> >Is there only one interrupt generated by the device?
>
> Yes, there's just one IRQ.
>
> >
> >>+
> >>+Example:
> >>+
> >>+sata: sata@fc600000 {
> >>+ compatible = "renesas,sata-r8a7779";
> >>+ reg = <0xfc600000 0x2000>;
> >>+ interrupt-parent = <&gic>;
> >>+ interrupts = <0 100 0x4>;
> >>+};
> >>diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi
> >>index da61d27..b1747f5 100644
> >>--- a/arch/arm/boot/dts/r8a7779.dtsi
> >>+++ b/arch/arm/boot/dts/r8a7779.dtsi
> >>@@ -201,7 +201,7 @@
> >> };
> >>
> >> sata: sata@fc600000 {
> >>- compatible = "renesas,rcar-sata";
> >>+ compatible = "renesas,sata-r8a7779";
> >> reg = <0xfc600000 0x2000>;
> >> interrupt-parent = <&gic>;
> >> interrupts = <0 100 0x4>;
> >>diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> >>index c2d95e9..c4cd738 100644
> >>--- a/drivers/ata/sata_rcar.c
> >>+++ b/drivers/ata/sata_rcar.c
> >>@@ -893,7 +893,7 @@ static const struct dev_pm_ops sata_rcar_pm_ops = {
> >> #endif
> >>
> >> static struct of_device_id sata_rcar_match[] = {
> >>- { .compatible = "renesas,rcar-sata", },
> >>+ { .compatible = "renesas,sata-r8a7779", },
> >> {},
> >> };
> >
> >Why is an existing compatible string being removed?
> >
> >Please do not change existing strings, it will break any dts files that
> >are already in use.
>
> It won't break the dts files, since I'm changing the only dtsi file
> that uses the SATA node as well. It will only break binary
> compatibility with the older dtb files.
I believe that compatibility with older dtb files is the key issue.
> >While the new compatible string may be preferred, the old string should
> >be kept for compatibility.
>
> OK, I can keep the old string if it's needed.
FWIW I am doubtful that it is much of an issue in practice.
However, just to be safe, I think it would be bes tto leave
renesas,rcar-sata.
--
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
