Re: [PATCH 1/2] sata_rcar: Adjust and document device tree bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux