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

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

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/

> +- reg			: address range of the SATA registers.

Also their size...

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

> +- interrupts		: must consist of one interrupt specifier.

Is there only one interrupt generated by the device?

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

While the new compatible string may be preferred, the old string should
be kept for compatibility.

Thanks,
Mark.