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

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Oct 14, 2013 at 06:19:37PM +0100, 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@cogentembedded.com>
> >> ---
> >>   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.

Cheers.

> 
> >> +- 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.

This is unfortunately one area where there isn't any general consensus
on the terminology. I think "base address and size ..." is sufficient --
it maps well to the set of address-cells and size-cells. For reg values
describing MMIO register blocks, ePAPR says "the offsets and lengths of
the memory-mapped IO register blocks", and there's similar wording in
other bindings.

> 
> >
> >> +- 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.

Ok.

> 
> >
> >> +- interrupts		: must consist of one interrupt specifier.
> >
> > Is there only one interrupt generated by the device?
> 
> Yes, there's just one IRQ.

Ok.

> 
> >
> >> +
> >> +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.

Why should an existing dtb suddenly stop working?

What if someone has an out-of-tree dts?

> 
> >
> > 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.

Please do.

Thanks,
Mark.