[RFC 0/2] ARM: Tegra: Device Tree: Audio

[RFC 0/2] ARM: Tegra: Device Tree: Audio

[John Bonesio]

This patch series is continuing work to get board specific devices initialized
from device tree nodes rather than using the current static platform_device
registration.

This patch series is for comments only. It is not intended to be submitted as a
patch at this time. I am looking for comments on the overall approach.

This series is based upon an earlier RFC series I submitted around May 11th
beginning with, "[RFC 0/3] ARM: Tegra: Device Tree: i2c & wm8903".
I haven't yet addressed the replies to that earlier series, but I intend to.

I am also aware that some of these files have changed since I've started on
this work. I haven't yet rebased to the latest.

---

John Bonesio (2):
      ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.
      ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.


 arch/arm/boot/dts/tegra-harmony.dts |   10 ++++++++
 arch/arm/mach-tegra/board-dt.c      |    3 ++
 sound/soc/codecs/wm8903.c           |    5 ++++
 sound/soc/tegra/harmony.c           |   46 ++++++++++++++++++++++++++++++++---
 4 files changed, 60 insertions(+), 4 deletions(-)

-- 
- John

[RFC 0/2] ARM: Tegra: Device Tree: Audio

[John Bonesio]

This patch series is continuing work to get board specific devices initialized
from device tree nodes rather than using the current static platform_device
registration.

This patch series is for comments only. It is not intended to be submitted as a
patch at this time. I am looking for comments on the overall approach.

This series is based upon an earlier RFC series I submitted around May 11th
beginning with, "[RFC 0/3] ARM: Tegra: Device Tree: i2c & wm8903".
I haven't yet addressed the replies to that earlier series, but I intend to.

I am also aware that some of these files have changed since I've started on
this work. I haven't yet rebased to the latest.

---

John Bonesio (2):
      ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.
      ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.


 arch/arm/boot/dts/tegra-harmony.dts |   10 ++++++++
 arch/arm/mach-tegra/board-dt.c      |    3 ++
 sound/soc/codecs/wm8903.c           |    5 ++++
 sound/soc/tegra/harmony.c           |   46 ++++++++++++++++++++++++++++++++---
 4 files changed, 60 insertions(+), 4 deletions(-)

-- 
- John

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[John Bonesio]

This patch makes it so the top level audio card is initialized from the device
tree. This is just the first step getting the audio complex of devices
iniialized from device tree nodes.

Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/boot/dts/tegra-harmony.dts |    4 +++
 arch/arm/mach-tegra/board-dt.c      |    3 ++
 sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
index 05521a5..217a7f0 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -53,6 +53,10 @@
 		clock-frequency = <400000>;
 	};
 
+	harmony_audio: audio_card {
+		compatible = "nvidia,harmony-audio";
+	};
+
 	serial@70006300 {
 		status = "ok";
 		clock-frequency = < 216000000 >;
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index c498e84..a569ad9 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
 	&tegra_i2c_device2,
 	&tegra_i2c_device3,
 	&tegra_i2c_device4,
+	&tegra_i2s_device1,
+	&tegra_das_device,
+	&tegra_pcm_device,
 };
 
 static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
index f225087..faeec14 100644
--- a/sound/soc/tegra/harmony.c
+++ b/sound/soc/tegra/harmony.c
@@ -52,6 +52,18 @@
 
 #define DRV_NAME "tegra-snd-harmony"
 
+/* temporary - info will go into device tree */
+#define TEGRA_GPIO_PW2          178
+#define TEGRA_GPIO_PX0          184
+#define TEGRA_GPIO_PX1          185
+
+#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
+#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
+#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
+#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
+#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
+/* end temporary */
+
 #define GPIO_SPKR_EN    BIT(0)
 #define GPIO_INT_MIC_EN BIT(1)
 #define GPIO_EXT_MIC_EN BIT(2)
@@ -287,6 +299,14 @@ static struct snd_soc_card snd_soc_harmony = {
 	.num_links = 1,
 };
 
+/* temporary - put this into the device tree */
+static struct harmony_audio_platform_data harmony_audio_pdata = {
+	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
+	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
+	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
+	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
+};
+
 static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = &snd_soc_harmony;
@@ -307,10 +327,15 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 	}
 #endif
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data supplied\n");
-		return -EINVAL;
+	if (pdev->dev.of_node) {
+		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
+		pdata = pdev->dev.platform_data;
+	} else {
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			dev_err(&pdev->dev, "no platform data supplied\n");
+			return -EINVAL;
+		}
 	}
 
 	harmony = kzalloc(sizeof(struct tegra_harmony), GFP_KERNEL);
@@ -374,11 +399,23 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+/* Match table for of_platform binding */
+static const struct of_device_id harmony_of_match[] __devinitconst = {
+	{ .compatible = "nvidia,harmony-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, harmony_of_match);
+#endif
+
 static struct platform_driver tegra_snd_harmony_driver = {
 	.driver = {
 		.name = DRV_NAME,
 		.owner = THIS_MODULE,
 		.pm = &snd_soc_pm_ops,
+#if defined(CONFIG_OF)
+		.of_match_table = harmony_of_match,
+#endif
 	},
 	.probe = tegra_snd_harmony_probe,
 	.remove = __devexit_p(tegra_snd_harmony_remove),

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Fri, May 27, 2011 at 01:57:13PM -0700, John Bonesio wrote:
> This patch makes it so the top level audio card is initialized from the device
> tree. This is just the first step getting the audio complex of devices
> iniialized from device tree nodes.
> 
> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
>  arch/arm/mach-tegra/board-dt.c      |    3 ++
>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
>  3 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 05521a5..217a7f0 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -53,6 +53,10 @@
>  		clock-frequency = <400000>;
>  	};
>  
> +	harmony_audio: audio_card {
> +		compatible = "nvidia,harmony-audio";
> +	};
> +

By convention, '_' is discouraged in the device tree names.  Following
the generic names recommended practice, this node should be named
simply 'sound'.

You can also squash these two patches together I think; it will make
for a simpler patch overall.  Doing so would get rid of the temporary
code that you need to resort to here.

>  	serial@70006300 {
>  		status = "ok";
>  		clock-frequency = < 216000000 >;
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> index c498e84..a569ad9 100644
> --- a/arch/arm/mach-tegra/board-dt.c
> +++ b/arch/arm/mach-tegra/board-dt.c
> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>  	&tegra_i2c_device2,
>  	&tegra_i2c_device3,
>  	&tegra_i2c_device4,
> +	&tegra_i2s_device1,
> +	&tegra_das_device,
> +	&tegra_pcm_device,
>  };
>  
>  static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index f225087..faeec14 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -52,6 +52,18 @@
>  
>  #define DRV_NAME "tegra-snd-harmony"
>  
> +/* temporary - info will go into device tree */
> +#define TEGRA_GPIO_PW2          178
> +#define TEGRA_GPIO_PX0          184
> +#define TEGRA_GPIO_PX1          185
> +
> +#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
> +#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
> +#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
> +#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
> +#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
> +/* end temporary */
> +
>  #define GPIO_SPKR_EN    BIT(0)
>  #define GPIO_INT_MIC_EN BIT(1)
>  #define GPIO_EXT_MIC_EN BIT(2)
> @@ -287,6 +299,14 @@ static struct snd_soc_card snd_soc_harmony = {
>  	.num_links = 1,
>  };
>  
> +/* temporary - put this into the device tree */
> +static struct harmony_audio_platform_data harmony_audio_pdata = {
> +	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
> +	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
> +	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
> +	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
> +};
> +

Yes, you should not need a static structure here.  All the data is
dynamic.

>  static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  {
>  	struct snd_soc_card *card = &snd_soc_harmony;
> @@ -307,10 +327,15 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  	}
>  #endif
>  
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data supplied\n");
> -		return -EINVAL;
> +	if (pdev->dev.of_node) {
> +		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> +		pdata = pdev->dev.platform_data;

This is actually a bit dangerous, or at least a bad example.  In the
dt case, the platform_data structure should be allocated per-instance.

> +	} else {
> +		pdata = pdev->dev.platform_data;
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "no platform data supplied\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	harmony = kzalloc(sizeof(struct tegra_harmony), GFP_KERNEL);
> @@ -374,11 +399,23 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OF)
> +/* Match table for of_platform binding */
> +static const struct of_device_id harmony_of_match[] __devinitconst = {
> +	{ .compatible = "nvidia,harmony-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, harmony_of_match);
> +#endif
> +
>  static struct platform_driver tegra_snd_harmony_driver = {
>  	.driver = {
>  		.name = DRV_NAME,
>  		.owner = THIS_MODULE,
>  		.pm = &snd_soc_pm_ops,
> +#if defined(CONFIG_OF)
> +		.of_match_table = harmony_of_match,
> +#endif

The #ifdef() is no longer necessary here if you add an #else clause
defining harmony_of_match to NULL above.

In general, it is a good start.

g.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Fri, May 27, 2011 at 01:57:13PM -0700, John Bonesio wrote:
> This patch makes it so the top level audio card is initialized from the device
> tree. This is just the first step getting the audio complex of devices
> iniialized from device tree nodes.
> 
> Signed-off-by: John Bonesio<bones@secretlab.ca>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
>  arch/arm/mach-tegra/board-dt.c      |    3 ++
>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
>  3 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 05521a5..217a7f0 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -53,6 +53,10 @@
>  		clock-frequency = <400000>;
>  	};
>  
> +	harmony_audio: audio_card {
> +		compatible = "nvidia,harmony-audio";
> +	};
> +

By convention, '_' is discouraged in the device tree names.  Following
the generic names recommended practice, this node should be named
simply 'sound'.

You can also squash these two patches together I think; it will make
for a simpler patch overall.  Doing so would get rid of the temporary
code that you need to resort to here.

>  	serial at 70006300 {
>  		status = "ok";
>  		clock-frequency = < 216000000 >;
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> index c498e84..a569ad9 100644
> --- a/arch/arm/mach-tegra/board-dt.c
> +++ b/arch/arm/mach-tegra/board-dt.c
> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>  	&tegra_i2c_device2,
>  	&tegra_i2c_device3,
>  	&tegra_i2c_device4,
> +	&tegra_i2s_device1,
> +	&tegra_das_device,
> +	&tegra_pcm_device,
>  };
>  
>  static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index f225087..faeec14 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -52,6 +52,18 @@
>  
>  #define DRV_NAME "tegra-snd-harmony"
>  
> +/* temporary - info will go into device tree */
> +#define TEGRA_GPIO_PW2          178
> +#define TEGRA_GPIO_PX0          184
> +#define TEGRA_GPIO_PX1          185
> +
> +#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
> +#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
> +#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
> +#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
> +#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
> +/* end temporary */
> +
>  #define GPIO_SPKR_EN    BIT(0)
>  #define GPIO_INT_MIC_EN BIT(1)
>  #define GPIO_EXT_MIC_EN BIT(2)
> @@ -287,6 +299,14 @@ static struct snd_soc_card snd_soc_harmony = {
>  	.num_links = 1,
>  };
>  
> +/* temporary - put this into the device tree */
> +static struct harmony_audio_platform_data harmony_audio_pdata = {
> +	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
> +	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
> +	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
> +	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
> +};
> +

Yes, you should not need a static structure here.  All the data is
dynamic.

>  static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  {
>  	struct snd_soc_card *card = &snd_soc_harmony;
> @@ -307,10 +327,15 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  	}
>  #endif
>  
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data supplied\n");
> -		return -EINVAL;
> +	if (pdev->dev.of_node) {
> +		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> +		pdata = pdev->dev.platform_data;

This is actually a bit dangerous, or at least a bad example.  In the
dt case, the platform_data structure should be allocated per-instance.

> +	} else {
> +		pdata = pdev->dev.platform_data;
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "no platform data supplied\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	harmony = kzalloc(sizeof(struct tegra_harmony), GFP_KERNEL);
> @@ -374,11 +399,23 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OF)
> +/* Match table for of_platform binding */
> +static const struct of_device_id harmony_of_match[] __devinitconst = {
> +	{ .compatible = "nvidia,harmony-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, harmony_of_match);
> +#endif
> +
>  static struct platform_driver tegra_snd_harmony_driver = {
>  	.driver = {
>  		.name = DRV_NAME,
>  		.owner = THIS_MODULE,
>  		.pm = &snd_soc_pm_ops,
> +#if defined(CONFIG_OF)
> +		.of_match_table = harmony_of_match,
> +#endif

The #ifdef() is no longer necessary here if you add an #else clause
defining harmony_of_match to NULL above.

In general, it is a good start.

g.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Mark Brown]

On Fri, May 27, 2011 at 01:57:13PM -0700, John Bonesio wrote:

> +	harmony_audio: audio_card {
> +		compatible = "nvidia,harmony-audio";
> +	};
> +

Since the machine driver has been renamed to tegra_wm8903 to reflect the
fact that many of the nVidia designs are simple clones of each other we
ought to be using a similar name for the device tree binding for the
driver.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Mark Brown]

On Fri, May 27, 2011 at 01:57:13PM -0700, John Bonesio wrote:

> +	harmony_audio: audio_card {
> +		compatible = "nvidia,harmony-audio";
> +	};
> +

Since the machine driver has been renamed to tegra_wm8903 to reflect the
fact that many of the nVidia designs are simple clones of each other we
ought to be using a similar name for the device tree binding for the
driver.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/5/28 John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> This patch makes it so the top level audio card is initialized from the device
> tree. This is just the first step getting the audio complex of devices
> iniialized from device tree nodes.
>
> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
>
>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
>  arch/arm/mach-tegra/board-dt.c      |    3 ++
>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
>  3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 05521a5..217a7f0 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -53,6 +53,10 @@
>                clock-frequency = <400000>;
>        };
>
> +       harmony_audio: audio_card {
> +               compatible = "nvidia,harmony-audio";
> +       };
> +
>        serial@70006300 {
>                status = "ok";
>                clock-frequency = < 216000000 >;
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> index c498e84..a569ad9 100644
> --- a/arch/arm/mach-tegra/board-dt.c
> +++ b/arch/arm/mach-tegra/board-dt.c
> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>        &tegra_i2c_device2,
>        &tegra_i2c_device3,
>        &tegra_i2c_device4,
> +       &tegra_i2s_device1,
> +       &tegra_das_device,
> +       &tegra_pcm_device,

i am thinking whether platform_device should be created by
of_platform_device_create after scanning device tree, but not by
hard-code? looks like the two patches are only trying to let device
tree deliver gpio to drivers?

>  };
>
>  static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index f225087..faeec14 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -52,6 +52,18 @@
>
>  #define DRV_NAME "tegra-snd-harmony"
>
> +/* temporary - info will go into device tree */
> +#define TEGRA_GPIO_PW2          178
> +#define TEGRA_GPIO_PX0          184
> +#define TEGRA_GPIO_PX1          185
> +
> +#define HARMONY_GPIO_WM8903(_x_)       (TEGRA_NR_GPIOS + (_x_))
> +#define TEGRA_GPIO_SPKR_EN             HARMONY_GPIO_WM8903(2)
> +#define TEGRA_GPIO_HP_DET              TEGRA_GPIO_PW2
> +#define TEGRA_GPIO_INT_MIC_EN          TEGRA_GPIO_PX0
> +#define TEGRA_GPIO_EXT_MIC_EN          TEGRA_GPIO_PX1
> +/* end temporary */
> +
>  #define GPIO_SPKR_EN    BIT(0)
>  #define GPIO_INT_MIC_EN BIT(1)
>  #define GPIO_EXT_MIC_EN BIT(2)
> @@ -287,6 +299,14 @@ static struct snd_soc_card snd_soc_harmony = {
>        .num_links = 1,
>  };
>
> +/* temporary - put this into the device tree */
> +static struct harmony_audio_platform_data harmony_audio_pdata = {
> +       .gpio_spkr_en           = TEGRA_GPIO_SPKR_EN,
> +       .gpio_hp_det            = TEGRA_GPIO_HP_DET,
> +       .gpio_int_mic_en        = TEGRA_GPIO_INT_MIC_EN,
> +       .gpio_ext_mic_en        = TEGRA_GPIO_EXT_MIC_EN,
> +};
> +
>  static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  {
>        struct snd_soc_card *card = &snd_soc_harmony;
> @@ -307,10 +327,15 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>        }
>  #endif
>
> -       pdata = pdev->dev.platform_data;
> -       if (!pdata) {
> -               dev_err(&pdev->dev, "no platform data supplied\n");
> -               return -EINVAL;
> +       if (pdev->dev.of_node) {
> +               pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> +               pdata = pdev->dev.platform_data;
> +       } else {
> +               pdata = pdev->dev.platform_data;
> +               if (!pdata) {
> +                       dev_err(&pdev->dev, "no platform data supplied\n");
> +                       return -EINVAL;
> +               }
>        }
>
>        harmony = kzalloc(sizeof(struct tegra_harmony), GFP_KERNEL);
> @@ -374,11 +399,23 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
>        return 0;
>  }
>
> +#if defined(CONFIG_OF)
> +/* Match table for of_platform binding */
> +static const struct of_device_id harmony_of_match[] __devinitconst = {
> +       { .compatible = "nvidia,harmony-audio", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, harmony_of_match);
> +#endif
> +
>  static struct platform_driver tegra_snd_harmony_driver = {
>        .driver = {
>                .name = DRV_NAME,
>                .owner = THIS_MODULE,
>                .pm = &snd_soc_pm_ops,
> +#if defined(CONFIG_OF)
> +               .of_match_table = harmony_of_match,
> +#endif
>        },
>        .probe = tegra_snd_harmony_probe,
>        .remove = __devexit_p(tegra_snd_harmony_remove),
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/5/28 John Bonesio <bones@secretlab.ca>:
> This patch makes it so the top level audio card is initialized from the device
> tree. This is just the first step getting the audio complex of devices
> iniialized from device tree nodes.
>
> Signed-off-by: John Bonesio<bones@secretlab.ca>
> ---
>
> ?arch/arm/boot/dts/tegra-harmony.dts | ? ?4 +++
> ?arch/arm/mach-tegra/board-dt.c ? ? ?| ? ?3 ++
> ?sound/soc/tegra/harmony.c ? ? ? ? ? | ? 45 ++++++++++++++++++++++++++++++++---
> ?3 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 05521a5..217a7f0 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -53,6 +53,10 @@
> ? ? ? ? ? ? ? ?clock-frequency = <400000>;
> ? ? ? ?};
>
> + ? ? ? harmony_audio: audio_card {
> + ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
> + ? ? ? };
> +
> ? ? ? ?serial at 70006300 {
> ? ? ? ? ? ? ? ?status = "ok";
> ? ? ? ? ? ? ? ?clock-frequency = < 216000000 >;
> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> index c498e84..a569ad9 100644
> --- a/arch/arm/mach-tegra/board-dt.c
> +++ b/arch/arm/mach-tegra/board-dt.c
> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
> ? ? ? ?&tegra_i2c_device2,
> ? ? ? ?&tegra_i2c_device3,
> ? ? ? ?&tegra_i2c_device4,
> + ? ? ? &tegra_i2s_device1,
> + ? ? ? &tegra_das_device,
> + ? ? ? &tegra_pcm_device,

i am thinking whether platform_device should be created by
of_platform_device_create after scanning device tree, but not by
hard-code? looks like the two patches are only trying to let device
tree deliver gpio to drivers?

> ?};
>
> ?static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index f225087..faeec14 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -52,6 +52,18 @@
>
> ?#define DRV_NAME "tegra-snd-harmony"
>
> +/* temporary - info will go into device tree */
> +#define TEGRA_GPIO_PW2 ? ? ? ? ?178
> +#define TEGRA_GPIO_PX0 ? ? ? ? ?184
> +#define TEGRA_GPIO_PX1 ? ? ? ? ?185
> +
> +#define HARMONY_GPIO_WM8903(_x_) ? ? ? (TEGRA_NR_GPIOS + (_x_))
> +#define TEGRA_GPIO_SPKR_EN ? ? ? ? ? ? HARMONY_GPIO_WM8903(2)
> +#define TEGRA_GPIO_HP_DET ? ? ? ? ? ? ?TEGRA_GPIO_PW2
> +#define TEGRA_GPIO_INT_MIC_EN ? ? ? ? ?TEGRA_GPIO_PX0
> +#define TEGRA_GPIO_EXT_MIC_EN ? ? ? ? ?TEGRA_GPIO_PX1
> +/* end temporary */
> +
> ?#define GPIO_SPKR_EN ? ?BIT(0)
> ?#define GPIO_INT_MIC_EN BIT(1)
> ?#define GPIO_EXT_MIC_EN BIT(2)
> @@ -287,6 +299,14 @@ static struct snd_soc_card snd_soc_harmony = {
> ? ? ? ?.num_links = 1,
> ?};
>
> +/* temporary - put this into the device tree */
> +static struct harmony_audio_platform_data harmony_audio_pdata = {
> + ? ? ? .gpio_spkr_en ? ? ? ? ? = TEGRA_GPIO_SPKR_EN,
> + ? ? ? .gpio_hp_det ? ? ? ? ? ?= TEGRA_GPIO_HP_DET,
> + ? ? ? .gpio_int_mic_en ? ? ? ?= TEGRA_GPIO_INT_MIC_EN,
> + ? ? ? .gpio_ext_mic_en ? ? ? ?= TEGRA_GPIO_EXT_MIC_EN,
> +};
> +
> ?static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
> ?{
> ? ? ? ?struct snd_soc_card *card = &snd_soc_harmony;
> @@ -307,10 +327,15 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
> ? ? ? ?}
> ?#endif
>
> - ? ? ? pdata = pdev->dev.platform_data;
> - ? ? ? if (!pdata) {
> - ? ? ? ? ? ? ? dev_err(&pdev->dev, "no platform data supplied\n");
> - ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? if (pdev->dev.of_node) {
> + ? ? ? ? ? ? ? pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> + ? ? ? ? ? ? ? pdata = pdev->dev.platform_data;
> + ? ? ? } else {
> + ? ? ? ? ? ? ? pdata = pdev->dev.platform_data;
> + ? ? ? ? ? ? ? if (!pdata) {
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&pdev->dev, "no platform data supplied\n");
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> ? ? ? ?}
>
> ? ? ? ?harmony = kzalloc(sizeof(struct tegra_harmony), GFP_KERNEL);
> @@ -374,11 +399,23 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
> ? ? ? ?return 0;
> ?}
>
> +#if defined(CONFIG_OF)
> +/* Match table for of_platform binding */
> +static const struct of_device_id harmony_of_match[] __devinitconst = {
> + ? ? ? { .compatible = "nvidia,harmony-audio", },
> + ? ? ? {},
> +};
> +MODULE_DEVICE_TABLE(of, harmony_of_match);
> +#endif
> +
> ?static struct platform_driver tegra_snd_harmony_driver = {
> ? ? ? ?.driver = {
> ? ? ? ? ? ? ? ?.name = DRV_NAME,
> ? ? ? ? ? ? ? ?.owner = THIS_MODULE,
> ? ? ? ? ? ? ? ?.pm = &snd_soc_pm_ops,
> +#if defined(CONFIG_OF)
> + ? ? ? ? ? ? ? .of_match_table = harmony_of_match,
> +#endif
> ? ? ? ?},
> ? ? ? ?.probe = tegra_snd_harmony_probe,
> ? ? ? ?.remove = __devexit_p(tegra_snd_harmony_remove),
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2011/5/28 John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
>> This patch makes it so the top level audio card is initialized from the device
>> tree. This is just the first step getting the audio complex of devices
>> iniialized from device tree nodes.
>>
>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> ---
>>
>>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
>>  arch/arm/mach-tegra/board-dt.c      |    3 ++
>>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
>>  3 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
>> index 05521a5..217a7f0 100644
>> --- a/arch/arm/boot/dts/tegra-harmony.dts
>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
>> @@ -53,6 +53,10 @@
>>                clock-frequency = <400000>;
>>        };
>>
>> +       harmony_audio: audio_card {
>> +               compatible = "nvidia,harmony-audio";
>> +       };
>> +
>>        serial@70006300 {
>>                status = "ok";
>>                clock-frequency = < 216000000 >;
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>> index c498e84..a569ad9 100644
>> --- a/arch/arm/mach-tegra/board-dt.c
>> +++ b/arch/arm/mach-tegra/board-dt.c
>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>>        &tegra_i2c_device2,
>>        &tegra_i2c_device3,
>>        &tegra_i2c_device4,
>> +       &tegra_i2s_device1,
>> +       &tegra_das_device,
>> +       &tegra_pcm_device,
>
> i am thinking whether platform_device should be created by
> of_platform_device_create after scanning device tree, but not by
> hard-code? looks like the two patches are only trying to let device
> tree deliver gpio to drivers?

Can you please restate your question?  I don't think I understand what
you are trying to ask.

g.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao@gmail.com> wrote:
> 2011/5/28 John Bonesio <bones@secretlab.ca>:
>> This patch makes it so the top level audio card is initialized from the device
>> tree. This is just the first step getting the audio complex of devices
>> iniialized from device tree nodes.
>>
>> Signed-off-by: John Bonesio<bones@secretlab.ca>
>> ---
>>
>> ?arch/arm/boot/dts/tegra-harmony.dts | ? ?4 +++
>> ?arch/arm/mach-tegra/board-dt.c ? ? ?| ? ?3 ++
>> ?sound/soc/tegra/harmony.c ? ? ? ? ? | ? 45 ++++++++++++++++++++++++++++++++---
>> ?3 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
>> index 05521a5..217a7f0 100644
>> --- a/arch/arm/boot/dts/tegra-harmony.dts
>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
>> @@ -53,6 +53,10 @@
>> ? ? ? ? ? ? ? ?clock-frequency = <400000>;
>> ? ? ? ?};
>>
>> + ? ? ? harmony_audio: audio_card {
>> + ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
>> + ? ? ? };
>> +
>> ? ? ? ?serial at 70006300 {
>> ? ? ? ? ? ? ? ?status = "ok";
>> ? ? ? ? ? ? ? ?clock-frequency = < 216000000 >;
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>> index c498e84..a569ad9 100644
>> --- a/arch/arm/mach-tegra/board-dt.c
>> +++ b/arch/arm/mach-tegra/board-dt.c
>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>> ? ? ? ?&tegra_i2c_device2,
>> ? ? ? ?&tegra_i2c_device3,
>> ? ? ? ?&tegra_i2c_device4,
>> + ? ? ? &tegra_i2s_device1,
>> + ? ? ? &tegra_das_device,
>> + ? ? ? &tegra_pcm_device,
>
> i am thinking whether platform_device should be created by
> of_platform_device_create after scanning device tree, but not by
> hard-code? looks like the two patches are only trying to let device
> tree deliver gpio to drivers?

Can you please restate your question?  I don't think I understand what
you are trying to ask.

g.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/2 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 2011/5/28 John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
>>> This patch makes it so the top level audio card is initialized from the device
>>> tree. This is just the first step getting the audio complex of devices
>>> iniialized from device tree nodes.
>>>
>>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>>> ---
>>>
>>>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
>>>  arch/arm/mach-tegra/board-dt.c      |    3 ++
>>>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
>>>  3 files changed, 48 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
>>> index 05521a5..217a7f0 100644
>>> --- a/arch/arm/boot/dts/tegra-harmony.dts
>>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
>>> @@ -53,6 +53,10 @@
>>>                clock-frequency = <400000>;
>>>        };
>>>
>>> +       harmony_audio: audio_card {
>>> +               compatible = "nvidia,harmony-audio";
>>> +       };
>>> +
>>>        serial@70006300 {
>>>                status = "ok";
>>>                clock-frequency = < 216000000 >;
>>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>>> index c498e84..a569ad9 100644
>>> --- a/arch/arm/mach-tegra/board-dt.c
>>> +++ b/arch/arm/mach-tegra/board-dt.c
>>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>>>        &tegra_i2c_device2,
>>>        &tegra_i2c_device3,
>>>        &tegra_i2c_device4,
>>> +       &tegra_i2s_device1,
>>> +       &tegra_das_device,
>>> +       &tegra_pcm_device,
>>
>> i am thinking whether platform_device should be created by
>> of_platform_device_create after scanning device tree, but not by
>> hard-code? looks like the two patches are only trying to let device
>> tree deliver gpio to drivers?
>
> Can you please restate your question?  I don't think I understand what
> you are trying to ask.

i mean we don't need to have this platform device registerred by the
old way here. we may get it by matching information in dts and delete
all platform_device and related codes in arch/arm. For example:
dts:
        amba {
                compatible = "simple-bus";
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                harmony_audio: audio_card {
                       compatible = "nvidia,harmony-audio";
                       .....
                };
                ....
                ....
       }
then in arch/arm board file, let the platform device registerred automatically:

static struct of_device_id cell_bus_ids[] __initdata = {
        { .compatible = "simple-bus", },
        {},
};

static int __init cell_publish_devices(void)
{
        of_platform_bus_probe(NULL, cell_bus_ids, NULL);

        return 0;
}


>
> g.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/2 Grant Likely <grant.likely@secretlab.ca>:
> On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao@gmail.com> wrote:
>> 2011/5/28 John Bonesio <bones@secretlab.ca>:
>>> This patch makes it so the top level audio card is initialized from the device
>>> tree. This is just the first step getting the audio complex of devices
>>> iniialized from device tree nodes.
>>>
>>> Signed-off-by: John Bonesio<bones@secretlab.ca>
>>> ---
>>>
>>> ?arch/arm/boot/dts/tegra-harmony.dts | ? ?4 +++
>>> ?arch/arm/mach-tegra/board-dt.c ? ? ?| ? ?3 ++
>>> ?sound/soc/tegra/harmony.c ? ? ? ? ? | ? 45 ++++++++++++++++++++++++++++++++---
>>> ?3 files changed, 48 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
>>> index 05521a5..217a7f0 100644
>>> --- a/arch/arm/boot/dts/tegra-harmony.dts
>>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
>>> @@ -53,6 +53,10 @@
>>> ? ? ? ? ? ? ? ?clock-frequency = <400000>;
>>> ? ? ? ?};
>>>
>>> + ? ? ? harmony_audio: audio_card {
>>> + ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
>>> + ? ? ? };
>>> +
>>> ? ? ? ?serial at 70006300 {
>>> ? ? ? ? ? ? ? ?status = "ok";
>>> ? ? ? ? ? ? ? ?clock-frequency = < 216000000 >;
>>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
>>> index c498e84..a569ad9 100644
>>> --- a/arch/arm/mach-tegra/board-dt.c
>>> +++ b/arch/arm/mach-tegra/board-dt.c
>>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
>>> ? ? ? ?&tegra_i2c_device2,
>>> ? ? ? ?&tegra_i2c_device3,
>>> ? ? ? ?&tegra_i2c_device4,
>>> + ? ? ? &tegra_i2s_device1,
>>> + ? ? ? &tegra_das_device,
>>> + ? ? ? &tegra_pcm_device,
>>
>> i am thinking whether platform_device should be created by
>> of_platform_device_create after scanning device tree, but not by
>> hard-code? looks like the two patches are only trying to let device
>> tree deliver gpio to drivers?
>
> Can you please restate your question? ?I don't think I understand what
> you are trying to ask.

i mean we don't need to have this platform device registerred by the
old way here. we may get it by matching information in dts and delete
all platform_device and related codes in arch/arm. For example:
dts:
        amba {
                compatible = "simple-bus";
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;

                harmony_audio: audio_card {
                       compatible = "nvidia,harmony-audio";
                       .....
                };
                ....
                ....
       }
then in arch/arm board file, let the platform device registerred automatically:

static struct of_device_id cell_bus_ids[] __initdata = {
        { .compatible = "simple-bus", },
        {},
};

static int __init cell_publish_devices(void)
{
        of_platform_bus_probe(NULL, cell_bus_ids, NULL);

        return 0;
}


>
> g.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Thu, Jun 02, 2011 at 05:07:02PM +0800, Barry Song wrote:
> 2011/6/2 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> 2011/5/28 John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> >>> This patch makes it so the top level audio card is initialized from the device
> >>> tree. This is just the first step getting the audio complex of devices
> >>> iniialized from device tree nodes.
> >>>
> >>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> >>> ---
> >>>
> >>>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
> >>>  arch/arm/mach-tegra/board-dt.c      |    3 ++
> >>>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
> >>>  3 files changed, 48 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> >>> index 05521a5..217a7f0 100644
> >>> --- a/arch/arm/boot/dts/tegra-harmony.dts
> >>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> >>> @@ -53,6 +53,10 @@
> >>>                clock-frequency = <400000>;
> >>>        };
> >>>
> >>> +       harmony_audio: audio_card {
> >>> +               compatible = "nvidia,harmony-audio";
> >>> +       };
> >>> +
> >>>        serial@70006300 {
> >>>                status = "ok";
> >>>                clock-frequency = < 216000000 >;
> >>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> >>> index c498e84..a569ad9 100644
> >>> --- a/arch/arm/mach-tegra/board-dt.c
> >>> +++ b/arch/arm/mach-tegra/board-dt.c
> >>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
> >>>        &tegra_i2c_device2,
> >>>        &tegra_i2c_device3,
> >>>        &tegra_i2c_device4,
> >>> +       &tegra_i2s_device1,
> >>> +       &tegra_das_device,
> >>> +       &tegra_pcm_device,
> >>
> >> i am thinking whether platform_device should be created by
> >> of_platform_device_create after scanning device tree, but not by
> >> hard-code? looks like the two patches are only trying to let device
> >> tree deliver gpio to drivers?
> >
> > Can you please restate your question?  I don't think I understand what
> > you are trying to ask.
> 
> i mean we don't need to have this platform device registerred by the
> old way here. we may get it by matching information in dts and delete
> all platform_device and related codes in arch/arm. For example:

Ah, I see what you mean.  Yes, it is ultimately the goal to register
all the device from data in the device tree, but we're taking things
one step at a time.  For this series, it is expedient to register the
on-chip devices, and to move to dynamic registration later.

Right now we can't do dynamic registration for on-chip devices in a
lot of cases because we don't have the infrastructure to hook up the
associated struct clks.

g.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Thu, Jun 02, 2011 at 05:07:02PM +0800, Barry Song wrote:
> 2011/6/2 Grant Likely <grant.likely@secretlab.ca>:
> > On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao@gmail.com> wrote:
> >> 2011/5/28 John Bonesio <bones@secretlab.ca>:
> >>> This patch makes it so the top level audio card is initialized from the device
> >>> tree. This is just the first step getting the audio complex of devices
> >>> iniialized from device tree nodes.
> >>>
> >>> Signed-off-by: John Bonesio<bones@secretlab.ca>
> >>> ---
> >>>
> >>> ?arch/arm/boot/dts/tegra-harmony.dts | ? ?4 +++
> >>> ?arch/arm/mach-tegra/board-dt.c ? ? ?| ? ?3 ++
> >>> ?sound/soc/tegra/harmony.c ? ? ? ? ? | ? 45 ++++++++++++++++++++++++++++++++---
> >>> ?3 files changed, 48 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> >>> index 05521a5..217a7f0 100644
> >>> --- a/arch/arm/boot/dts/tegra-harmony.dts
> >>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> >>> @@ -53,6 +53,10 @@
> >>> ? ? ? ? ? ? ? ?clock-frequency = <400000>;
> >>> ? ? ? ?};
> >>>
> >>> + ? ? ? harmony_audio: audio_card {
> >>> + ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
> >>> + ? ? ? };
> >>> +
> >>> ? ? ? ?serial at 70006300 {
> >>> ? ? ? ? ? ? ? ?status = "ok";
> >>> ? ? ? ? ? ? ? ?clock-frequency = < 216000000 >;
> >>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> >>> index c498e84..a569ad9 100644
> >>> --- a/arch/arm/mach-tegra/board-dt.c
> >>> +++ b/arch/arm/mach-tegra/board-dt.c
> >>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
> >>> ? ? ? ?&tegra_i2c_device2,
> >>> ? ? ? ?&tegra_i2c_device3,
> >>> ? ? ? ?&tegra_i2c_device4,
> >>> + ? ? ? &tegra_i2s_device1,
> >>> + ? ? ? &tegra_das_device,
> >>> + ? ? ? &tegra_pcm_device,
> >>
> >> i am thinking whether platform_device should be created by
> >> of_platform_device_create after scanning device tree, but not by
> >> hard-code? looks like the two patches are only trying to let device
> >> tree deliver gpio to drivers?
> >
> > Can you please restate your question? ?I don't think I understand what
> > you are trying to ask.
> 
> i mean we don't need to have this platform device registerred by the
> old way here. we may get it by matching information in dts and delete
> all platform_device and related codes in arch/arm. For example:

Ah, I see what you mean.  Yes, it is ultimately the goal to register
all the device from data in the device tree, but we're taking things
one step at a time.  For this series, it is expedient to register the
on-chip devices, and to move to dynamic registration later.

Right now we can't do dynamic registration for on-chip devices in a
lot of cases because we don't have the infrastructure to hook up the
associated struct clks.

g.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
> On Thu, Jun 02, 2011 at 05:07:02PM +0800, Barry Song wrote:
> > 2011/6/2 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > > On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >> 2011/5/28 John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> > >>> This patch makes it so the top level audio card is initialized from the device
> > >>> tree. This is just the first step getting the audio complex of devices
> > >>> iniialized from device tree nodes.
> > >>>
> > >>> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > >>> ---
> > >>>
> > >>>  arch/arm/boot/dts/tegra-harmony.dts |    4 +++
> > >>>  arch/arm/mach-tegra/board-dt.c      |    3 ++
> > >>>  sound/soc/tegra/harmony.c           |   45 ++++++++++++++++++++++++++++++++---
> > >>>  3 files changed, 48 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> > >>> index 05521a5..217a7f0 100644
> > >>> --- a/arch/arm/boot/dts/tegra-harmony.dts
> > >>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> > >>> @@ -53,6 +53,10 @@
> > >>>                clock-frequency = <400000>;
> > >>>        };
> > >>>
> > >>> +       harmony_audio: audio_card {
> > >>> +               compatible = "nvidia,harmony-audio";
> > >>> +       };
> > >>> +
> > >>>        serial@70006300 {
> > >>>                status = "ok";
> > >>>                clock-frequency = < 216000000 >;
> > >>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> > >>> index c498e84..a569ad9 100644
> > >>> --- a/arch/arm/mach-tegra/board-dt.c
> > >>> +++ b/arch/arm/mach-tegra/board-dt.c
> > >>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
> > >>>        &tegra_i2c_device2,
> > >>>        &tegra_i2c_device3,
> > >>>        &tegra_i2c_device4,
> > >>> +       &tegra_i2s_device1,
> > >>> +       &tegra_das_device,
> > >>> +       &tegra_pcm_device,
> > >>
> > >> i am thinking whether platform_device should be created by
> > >> of_platform_device_create after scanning device tree, but not by
> > >> hard-code? looks like the two patches are only trying to let device
> > >> tree deliver gpio to drivers?
> > >
> > > Can you please restate your question?  I don't think I understand what
> > > you are trying to ask.
> >
> > i mean we don't need to have this platform device registerred by the
> > old way here. we may get it by matching information in dts and delete
> > all platform_device and related codes in arch/arm. For example:
>
> Ah, I see what you mean.  Yes, it is ultimately the goal to register
> all the device from data in the device tree, but we're taking things
> one step at a time.  For this series, it is expedient to register the
> on-chip devices, and to move to dynamic registration later.
>
> Right now we can't do dynamic registration for on-chip devices in a
> lot of cases because we don't have the infrastructure to hook up the
> associated struct clks.
Arnd has required me to use device tree in our new SoC for the coming
upstream. so i am trying to define a property like clock = "uart" in
dts. then in drivers,
i get this  string by:
clk = of_get_property(np, "clock", NULL);
then request this clock by clk_get().

it does work for the moment, but maybe is not what you like :-)

>
> g.
>
>

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Grant Likely <grant.likely@secretlab.ca>
>
> On Thu, Jun 02, 2011 at 05:07:02PM +0800, Barry Song wrote:
> > 2011/6/2 Grant Likely <grant.likely@secretlab.ca>:
> > > On Wed, Jun 1, 2011 at 1:07 AM, Barry Song <21cnbao@gmail.com> wrote:
> > >> 2011/5/28 John Bonesio <bones@secretlab.ca>:
> > >>> This patch makes it so the top level audio card is initialized from the device
> > >>> tree. This is just the first step getting the audio complex of devices
> > >>> iniialized from device tree nodes.
> > >>>
> > >>> Signed-off-by: John Bonesio<bones@secretlab.ca>
> > >>> ---
> > >>>
> > >>> ?arch/arm/boot/dts/tegra-harmony.dts | ? ?4 +++
> > >>> ?arch/arm/mach-tegra/board-dt.c ? ? ?| ? ?3 ++
> > >>> ?sound/soc/tegra/harmony.c ? ? ? ? ? | ? 45 ++++++++++++++++++++++++++++++++---
> > >>> ?3 files changed, 48 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> > >>> index 05521a5..217a7f0 100644
> > >>> --- a/arch/arm/boot/dts/tegra-harmony.dts
> > >>> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> > >>> @@ -53,6 +53,10 @@
> > >>> ? ? ? ? ? ? ? ?clock-frequency = <400000>;
> > >>> ? ? ? ?};
> > >>>
> > >>> + ? ? ? harmony_audio: audio_card {
> > >>> + ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
> > >>> + ? ? ? };
> > >>> +
> > >>> ? ? ? ?serial at 70006300 {
> > >>> ? ? ? ? ? ? ? ?status = "ok";
> > >>> ? ? ? ? ? ? ? ?clock-frequency = < 216000000 >;
> > >>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> > >>> index c498e84..a569ad9 100644
> > >>> --- a/arch/arm/mach-tegra/board-dt.c
> > >>> +++ b/arch/arm/mach-tegra/board-dt.c
> > >>> @@ -56,6 +56,9 @@ static struct platform_device *tegra250_devices[] __initdata = {
> > >>> ? ? ? ?&tegra_i2c_device2,
> > >>> ? ? ? ?&tegra_i2c_device3,
> > >>> ? ? ? ?&tegra_i2c_device4,
> > >>> + ? ? ? &tegra_i2s_device1,
> > >>> + ? ? ? &tegra_das_device,
> > >>> + ? ? ? &tegra_pcm_device,
> > >>
> > >> i am thinking whether platform_device should be created by
> > >> of_platform_device_create after scanning device tree, but not by
> > >> hard-code? looks like the two patches are only trying to let device
> > >> tree deliver gpio to drivers?
> > >
> > > Can you please restate your question? ?I don't think I understand what
> > > you are trying to ask.
> >
> > i mean we don't need to have this platform device registerred by the
> > old way here. we may get it by matching information in dts and delete
> > all platform_device and related codes in arch/arm. For example:
>
> Ah, I see what you mean. ?Yes, it is ultimately the goal to register
> all the device from data in the device tree, but we're taking things
> one step at a time. ?For this series, it is expedient to register the
> on-chip devices, and to move to dynamic registration later.
>
> Right now we can't do dynamic registration for on-chip devices in a
> lot of cases because we don't have the infrastructure to hook up the
> associated struct clks.
Arnd has required me to use device tree in our new SoC for the coming
upstream. so i am trying to define a property like clock = "uart" in
dts. then in drivers,
i get this  string by:
clk = of_get_property(np, "clock", NULL);
then request this clock by clk_get().

it does work for the moment, but maybe is not what you like :-)

>
> g.
>
>

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Fri, Jun 03, 2011 at 12:21:50AM +0800, Barry Song wrote:
> Arnd has required me to use device tree in our new SoC for the coming
> upstream. so i am trying to define a property like clock = "uart" in
> dts. then in drivers,
> i get this  string by:
> clk = of_get_property(np, "clock", NULL);
> then request this clock by clk_get().

This is entirely wrong.  clk_get() takes two things.  It takes a struct
device.  We should know what the struct device is (provided they're named
in a stable manner.)

The other parameter, the string, is up to the driver.  It's not a device
property.  It's not a SoC-wide clock name.  It's a connection name for
the clock on the device.  This won't change from one instance of the
device to another instance of the device - it's effectively a constant.

So there's no point in having the DT describe that name - that's out of
its realm.

One of the problems is that clk_get() hides the mapping of device+connection
internally, which it has had to as we haven't had a device tree to look
things up.

In essence, clk_get() is looking up a property (the clock connection name)
for the struct device.  When clks get converted to the device tree, the
DT stuff should hook inside clk_get() to do a property lookup to discover
which clock the driver wants.

Drivers should definitely not be looking up a property in the device tree
and using that as a connection name into clk_get().

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Fri, Jun 03, 2011 at 12:21:50AM +0800, Barry Song wrote:
> Arnd has required me to use device tree in our new SoC for the coming
> upstream. so i am trying to define a property like clock = "uart" in
> dts. then in drivers,
> i get this  string by:
> clk = of_get_property(np, "clock", NULL);
> then request this clock by clk_get().

This is entirely wrong.  clk_get() takes two things.  It takes a struct
device.  We should know what the struct device is (provided they're named
in a stable manner.)

The other parameter, the string, is up to the driver.  It's not a device
property.  It's not a SoC-wide clock name.  It's a connection name for
the clock on the device.  This won't change from one instance of the
device to another instance of the device - it's effectively a constant.

So there's no point in having the DT describe that name - that's out of
its realm.

One of the problems is that clk_get() hides the mapping of device+connection
internally, which it has had to as we haven't had a device tree to look
things up.

In essence, clk_get() is looking up a property (the clock connection name)
for the struct device.  When clks get converted to the device tree, the
DT stuff should hook inside clk_get() to do a property lookup to discover
which clock the driver wants.

Drivers should definitely not be looking up a property in the device tree
and using that as a connection name into clk_get().

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>:
> On Fri, Jun 03, 2011 at 12:21:50AM +0800, Barry Song wrote:
>> Arnd has required me to use device tree in our new SoC for the coming
>> upstream. so i am trying to define a property like clock = "uart" in
>> dts. then in drivers,
>> i get this  string by:
>> clk = of_get_property(np, "clock", NULL);
>> then request this clock by clk_get().
>
> This is entirely wrong.  clk_get() takes two things.  It takes a struct
> device.  We should know what the struct device is (provided they're named
> in a stable manner.)
>
> The other parameter, the string, is up to the driver.  It's not a device
> property.  It's not a SoC-wide clock name.  It's a connection name for
> the clock on the device.  This won't change from one instance of the
> device to another instance of the device - it's effectively a constant.
>
i see :-)

but there is really no an unified rule by now, for exmaple, samsung
just required platform device names matched with the string parameter
to get a clock.
it looks like clk_get in plat-samsung depends on the string more than
device and the clock name is in SoC level.

struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *p;
        struct clk *clk = ERR_PTR(-ENOENT);
        int idno;

        if (dev == NULL || !dev_is_platform_device(dev))
                idno = -1;
        else
                idno = to_platform_device(dev)->id;

        spin_lock(&clocks_lock);

        list_for_each_entry(p, &clocks, list) {
                if (p->id == idno &&
                    strcmp(id, p->name) == 0 &&
                    try_module_get(p->owner)) {
                        clk = p;
                        break;
                }
        }

        /* check for the case where a device was supplied, but the
         * clock that was being searched for is not device specific */

        if (IS_ERR(clk)) {
                list_for_each_entry(p, &clocks, list) {
                        if (p->id == -1 && strcmp(id, p->name) == 0 &&
                            try_module_get(p->owner)) {
                                clk = p;
                                break;
                        }
                }

}

same situation for mach-at91/clock.c:

/* clocks cannot be de-registered no refcounting necessary */
struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *clk;

        list_for_each_entry(clk, &clocks, node) {
                if (strcmp(id, clk->name) == 0)
                        return clk;
                if (clk->function && (dev == clk->dev) && strcmp(id,
clk->function) == 0)
                        return clk;
        }

        return ERR_PTR(-ENOENT);
}
EXPORT_SYMBOL(clk_get);


msm required device struct matched:
struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *clk;

        mutex_lock(&clocks_mutex);

        list_for_each_entry(clk, &clocks, list)
                if (!strcmp(id, clk->name) && clk->dev == dev)
                        goto found_it;

        list_for_each_entry(clk, &clocks, list)
                if (!strcmp(id, clk->name) && clk->dev == NULL)
                        goto found_it;

        clk = ERR_PTR(-ENOENT);
found_it:
        mutex_unlock(&clocks_mutex);
        return clk;
}
EXPORT_SYMBOL(clk_get);

> So there's no point in having the DT describe that name - that's out of
> its realm.

yes. i made a mistake. our old codes have an ugly implement of clk
APIs, which is using names with index, for example "uart0", "uart1",
"uart2",  to distinguish clocks for multi-instances of same kind of
devices. so i included this name in dts to bring up and test device
tree functions fast. i believe our clk implementation need clean-up.
but what's the best possible way to bind clocks with dynamic
registerred devices from device tree? people maybe need more
agreement.


>
> One of the problems is that clk_get() hides the mapping of device+connection
> internally, which it has had to as we haven't had a device tree to look
> things up.
>
> In essence, clk_get() is looking up a property (the clock connection name)
> for the struct device.  When clks get converted to the device tree, the
> DT stuff should hook inside clk_get() to do a property lookup to discover
> which clock the driver wants.
>
> Drivers should definitely not be looking up a property in the device tree
> and using that as a connection name into clk_get().

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Jun 03, 2011 at 12:21:50AM +0800, Barry Song wrote:
>> Arnd has required me to use device tree in our new SoC for the coming
>> upstream. so i am trying to define a property like clock = "uart" in
>> dts. then in drivers,
>> i get this ?string by:
>> clk = of_get_property(np, "clock", NULL);
>> then request this clock by clk_get().
>
> This is entirely wrong. ?clk_get() takes two things. ?It takes a struct
> device. ?We should know what the struct device is (provided they're named
> in a stable manner.)
>
> The other parameter, the string, is up to the driver. ?It's not a device
> property. ?It's not a SoC-wide clock name. ?It's a connection name for
> the clock on the device. ?This won't change from one instance of the
> device to another instance of the device - it's effectively a constant.
>
i see :-)

but there is really no an unified rule by now, for exmaple, samsung
just required platform device names matched with the string parameter
to get a clock.
it looks like clk_get in plat-samsung depends on the string more than
device and the clock name is in SoC level.

struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *p;
        struct clk *clk = ERR_PTR(-ENOENT);
        int idno;

        if (dev == NULL || !dev_is_platform_device(dev))
                idno = -1;
        else
                idno = to_platform_device(dev)->id;

        spin_lock(&clocks_lock);

        list_for_each_entry(p, &clocks, list) {
                if (p->id == idno &&
                    strcmp(id, p->name) == 0 &&
                    try_module_get(p->owner)) {
                        clk = p;
                        break;
                }
        }

        /* check for the case where a device was supplied, but the
         * clock that was being searched for is not device specific */

        if (IS_ERR(clk)) {
                list_for_each_entry(p, &clocks, list) {
                        if (p->id == -1 && strcmp(id, p->name) == 0 &&
                            try_module_get(p->owner)) {
                                clk = p;
                                break;
                        }
                }

}

same situation for mach-at91/clock.c:

/* clocks cannot be de-registered no refcounting necessary */
struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *clk;

        list_for_each_entry(clk, &clocks, node) {
                if (strcmp(id, clk->name) == 0)
                        return clk;
                if (clk->function && (dev == clk->dev) && strcmp(id,
clk->function) == 0)
                        return clk;
        }

        return ERR_PTR(-ENOENT);
}
EXPORT_SYMBOL(clk_get);


msm required device struct matched:
struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *clk;

        mutex_lock(&clocks_mutex);

        list_for_each_entry(clk, &clocks, list)
                if (!strcmp(id, clk->name) && clk->dev == dev)
                        goto found_it;

        list_for_each_entry(clk, &clocks, list)
                if (!strcmp(id, clk->name) && clk->dev == NULL)
                        goto found_it;

        clk = ERR_PTR(-ENOENT);
found_it:
        mutex_unlock(&clocks_mutex);
        return clk;
}
EXPORT_SYMBOL(clk_get);

> So there's no point in having the DT describe that name - that's out of
> its realm.

yes. i made a mistake. our old codes have an ugly implement of clk
APIs, which is using names with index, for example "uart0", "uart1",
"uart2",  to distinguish clocks for multi-instances of same kind of
devices. so i included this name in dts to bring up and test device
tree functions fast. i believe our clk implementation need clean-up.
but what's the best possible way to bind clocks with dynamic
registerred devices from device tree? people maybe need more
agreement.


>
> One of the problems is that clk_get() hides the mapping of device+connection
> internally, which it has had to as we haven't had a device tree to look
> things up.
>
> In essence, clk_get() is looking up a property (the clock connection name)
> for the struct device. ?When clks get converted to the device tree, the
> DT stuff should hook inside clk_get() to do a property lookup to discover
> which clock the driver wants.
>
> Drivers should definitely not be looking up a property in the device tree
> and using that as a connection name into clk_get().

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Fri, Jun 03, 2011 at 10:32:52AM +0800, Barry Song wrote:
> but there is really no an unified rule by now, for exmaple, samsung
> just required platform device names matched with the string parameter
> to get a clock.
> it looks like clk_get in plat-samsung depends on the string more than
> device and the clock name is in SoC level.

Samsung has been broken in respect of this for quite some time, and I've
been nagging Ben about it ever since I provided clkdev.  The problem is
that Ben doesn't have the time to fix Samsung...

> same situation for mach-at91/clock.c:
> 
> /* clocks cannot be de-registered no refcounting necessary */
> struct clk *clk_get(struct device *dev, const char *id)
> {
>         struct clk *clk;
> 
>         list_for_each_entry(clk, &clocks, node) {
>                 if (strcmp(id, clk->name) == 0)
>                         return clk;
>                 if (clk->function && (dev == clk->dev) && strcmp(id,
> clk->function) == 0)
>                         return clk;
>         }
> 
>         return ERR_PTR(-ENOENT);
> }
> EXPORT_SYMBOL(clk_get);

That's broken, and it's incompatible with DT in any case because the
only way to set 'clk->dev' is to have devices statically declared.

OMAP used to be broken until I converted it to clkdev, and when I did
their drivers became more simple because they didn't need to ifdef
clocknames and such like.

> msm required device struct matched:
> struct clk *clk_get(struct device *dev, const char *id)
> {
>         struct clk *clk;
> 
>         mutex_lock(&clocks_mutex);
> 
>         list_for_each_entry(clk, &clocks, list)
>                 if (!strcmp(id, clk->name) && clk->dev == dev)
>                         goto found_it;
> 
>         list_for_each_entry(clk, &clocks, list)
>                 if (!strcmp(id, clk->name) && clk->dev == NULL)
>                         goto found_it;
> 
>         clk = ERR_PTR(-ENOENT);
> found_it:
>         mutex_unlock(&clocks_mutex);
>         return clk;
> }
> EXPORT_SYMBOL(clk_get);

Again, that's incompatible with DT in any case, as we don't know what
'clk->dev' would be if the devices aren't statically declared.  So this
is broken too.

Each need to be converted to clkdev _before_ they even start thinking
about device trees.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Fri, Jun 03, 2011 at 10:32:52AM +0800, Barry Song wrote:
> but there is really no an unified rule by now, for exmaple, samsung
> just required platform device names matched with the string parameter
> to get a clock.
> it looks like clk_get in plat-samsung depends on the string more than
> device and the clock name is in SoC level.

Samsung has been broken in respect of this for quite some time, and I've
been nagging Ben about it ever since I provided clkdev.  The problem is
that Ben doesn't have the time to fix Samsung...

> same situation for mach-at91/clock.c:
> 
> /* clocks cannot be de-registered no refcounting necessary */
> struct clk *clk_get(struct device *dev, const char *id)
> {
>         struct clk *clk;
> 
>         list_for_each_entry(clk, &clocks, node) {
>                 if (strcmp(id, clk->name) == 0)
>                         return clk;
>                 if (clk->function && (dev == clk->dev) && strcmp(id,
> clk->function) == 0)
>                         return clk;
>         }
> 
>         return ERR_PTR(-ENOENT);
> }
> EXPORT_SYMBOL(clk_get);

That's broken, and it's incompatible with DT in any case because the
only way to set 'clk->dev' is to have devices statically declared.

OMAP used to be broken until I converted it to clkdev, and when I did
their drivers became more simple because they didn't need to ifdef
clocknames and such like.

> msm required device struct matched:
> struct clk *clk_get(struct device *dev, const char *id)
> {
>         struct clk *clk;
> 
>         mutex_lock(&clocks_mutex);
> 
>         list_for_each_entry(clk, &clocks, list)
>                 if (!strcmp(id, clk->name) && clk->dev == dev)
>                         goto found_it;
> 
>         list_for_each_entry(clk, &clocks, list)
>                 if (!strcmp(id, clk->name) && clk->dev == NULL)
>                         goto found_it;
> 
>         clk = ERR_PTR(-ENOENT);
> found_it:
>         mutex_unlock(&clocks_mutex);
>         return clk;
> }
> EXPORT_SYMBOL(clk_get);

Again, that's incompatible with DT in any case, as we don't know what
'clk->dev' would be if the devices aren't statically declared.  So this
is broken too.

Each need to be converted to clkdev _before_ they even start thinking
about device trees.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
> Right now we can't do dynamic registration for on-chip devices in a
> lot of cases because we don't have the infrastructure to hook up the
> associated struct clks.

I've been wondering about this, and I don't see it as a blocking problem
as you seem to be.

I assume platform devices have stable names when they're created from
the device tree?  If yes, there's no problem having the DT start to
describe the SoC specific devices _today_ - all that the clk API using
clkdev requires is a stable device name.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Russell King - ARM Linux]

On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
> Right now we can't do dynamic registration for on-chip devices in a
> lot of cases because we don't have the infrastructure to hook up the
> associated struct clks.

I've been wondering about this, and I don't see it as a blocking problem
as you seem to be.

I assume platform devices have stable names when they're created from
the device tree?  If yes, there's no problem having the DT start to
describe the SoC specific devices _today_ - all that the clk API using
clkdev requires is a stable device name.

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>:
> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
>> Right now we can't do dynamic registration for on-chip devices in a
>> lot of cases because we don't have the infrastructure to hook up the
>> associated struct clks.
>
> I've been wondering about this, and I don't see it as a blocking problem
> as you seem to be.
>
> I assume platform devices have stable names when they're created from
> the device tree?  If yes, there's no problem having the DT start to
> describe the SoC specific devices _today_ - all that the clk API using
> clkdev requires is a stable device name.

when i write the following nodes in dts,

uart0: uart@0xb0060000 {
	compatible = "sirf,uart";
	...
}

uart1: uart@0xb0050000 {
	compatible = "sirf,uart";
	...
}

uart2: uart@0xb0070000 {
	compatible = "sirf,uart";
	...
}

then create these platform devices by of_platform_xxx things, i get
some platform devices like the below.

b0060000.uart
b0050000.uart
b0070000.uart

so these are the "stable names" you are talking about? or something else?

>

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
>> Right now we can't do dynamic registration for on-chip devices in a
>> lot of cases because we don't have the infrastructure to hook up the
>> associated struct clks.
>
> I've been wondering about this, and I don't see it as a blocking problem
> as you seem to be.
>
> I assume platform devices have stable names when they're created from
> the device tree? ?If yes, there's no problem having the DT start to
> describe the SoC specific devices _today_ - all that the clk API using
> clkdev requires is a stable device name.

when i write the following nodes in dts,

uart0: uart at 0xb0060000 {
	compatible = "sirf,uart";
	...
}

uart1: uart at 0xb0050000 {
	compatible = "sirf,uart";
	...
}

uart2: uart at 0xb0070000 {
	compatible = "sirf,uart";
	...
}

then create these platform devices by of_platform_xxx things, i get
some platform devices like the below.

b0060000.uart
b0050000.uart
b0070000.uart

so these are the "stable names" you are talking about? or something else?

>

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Barry Song <21cnbao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 2011/6/3 Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>:
>> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
>>> Right now we can't do dynamic registration for on-chip devices in a
>>> lot of cases because we don't have the infrastructure to hook up the
>>> associated struct clks.
>>
>> I've been wondering about this, and I don't see it as a blocking problem
>> as you seem to be.
>>
>> I assume platform devices have stable names when they're created from
>> the device tree?  If yes, there's no problem having the DT start to
>> describe the SoC specific devices _today_ - all that the clk API using
>> clkdev requires is a stable device name.
>
> when i write the following nodes in dts,
>
> uart0: uart@0xb0060000 {
>        compatible = "sirf,uart";
>        ...
> }
>
> uart1: uart@0xb0050000 {
>        compatible = "sirf,uart";
>        ...
> }
>
> uart2: uart@0xb0070000 {
>        compatible = "sirf,uart";
>        ...
> }
>
> then create these platform devices by of_platform_xxx things, i get
> some platform devices like the below.
>
> b0060000.uart
> b0050000.uart
> b0070000.uart
>
> so these are the "stable names" you are talking about? or something else?

if so, we will need some clk_lookup table like:

#define CLK(_clk, _devname, _conname)                   \
        {                                               \
                .clk    = &clk_##_clk,                  \
                .dev_id = _devname,                     \
                .con_id = _conname,                     \
        }

CLK(uart0,      "b0060000.uart",        NULL),
CLK(uart1,      "b0050000.uart",        NULL),
CLK(uart2,      "b0070000.uart",        NULL),

it looks not like what we want too.

>
>>
>

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Barry Song]

2011/6/3 Barry Song <21cnbao@gmail.com>:
> 2011/6/3 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
>>> Right now we can't do dynamic registration for on-chip devices in a
>>> lot of cases because we don't have the infrastructure to hook up the
>>> associated struct clks.
>>
>> I've been wondering about this, and I don't see it as a blocking problem
>> as you seem to be.
>>
>> I assume platform devices have stable names when they're created from
>> the device tree? ?If yes, there's no problem having the DT start to
>> describe the SoC specific devices _today_ - all that the clk API using
>> clkdev requires is a stable device name.
>
> when i write the following nodes in dts,
>
> uart0: uart at 0xb0060000 {
> ? ? ? ?compatible = "sirf,uart";
> ? ? ? ?...
> }
>
> uart1: uart at 0xb0050000 {
> ? ? ? ?compatible = "sirf,uart";
> ? ? ? ?...
> }
>
> uart2: uart at 0xb0070000 {
> ? ? ? ?compatible = "sirf,uart";
> ? ? ? ?...
> }
>
> then create these platform devices by of_platform_xxx things, i get
> some platform devices like the below.
>
> b0060000.uart
> b0050000.uart
> b0070000.uart
>
> so these are the "stable names" you are talking about? or something else?

if so, we will need some clk_lookup table like:

#define CLK(_clk, _devname, _conname)                   \
        {                                               \
                .clk    = &clk_##_clk,                  \
                .dev_id = _devname,                     \
                .con_id = _conname,                     \
        }

CLK(uart0,      "b0060000.uart",        NULL),
CLK(uart1,      "b0050000.uart",        NULL),
CLK(uart2,      "b0070000.uart",        NULL),

it looks not like what we want too.

>
>>
>

Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Thu, Jun 02, 2011 at 10:36:56PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
> > Right now we can't do dynamic registration for on-chip devices in a
> > lot of cases because we don't have the infrastructure to hook up the
> > associated struct clks.
> 
> I've been wondering about this, and I don't see it as a blocking problem
> as you seem to be.
> 
> I assume platform devices have stable names when they're created from
> the device tree?  If yes, there's no problem having the DT start to
> describe the SoC specific devices _today_ - all that the clk API using
> clkdev requires is a stable device name.

Yes, the name is stable, but the name isn't the same as what board
support code currently uses for statically registered
platform_devices, and when I started on this work my goal has been to
have as little impact on existing platform support code.  I certainly
didn't want two sets of clock registrations, one with the names that
statically allocated platform devices use and one with the DT
generated names.

One solution would have been to add a "linux,devname" property or
something similar to each device node, but one of the things I'm very
careful about is to strongly avoid encoding Linux specific
implementation details into the DT.  History has shown that internal
details are subject to change over time, so there is less chance of
breaking a DT platform if the description is focused on the HW.  The
name Linux needs to hook up {clocks,regulators,etc} to a device very
much falls into this category.

That said, I stepped back and took another look at the problem after
receiving your email, and I've been looking at it all wrong.  I did
actually had a solution that matched up static device registrations
to DT nodes (of_platform_prepare()), but it was complex and ugly so I
wasn't very excited about it.

However, I've got a patch now that solves the problem in a much nicer
way.  If Linux needs specific devices to have specific names, then it
can pass in a lookup table that matches DT nodes to device names,
which is considerably simpler and it leaves the platform setup code in
control over how devices get instantiated.  I'll post the patches today
or tomorrow.

So, basically, all my problems on obtaining clocks are now completely
gone.  :-)

Ultimately, this is a short-term solution that allows devices to get
described in the DT without having to implement clock mappings at the
same time.  When clock mappings also get moved to the device tree, the
need for the lookup table simply goes away.

g.

[RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree.

[Grant Likely]

On Thu, Jun 02, 2011 at 10:36:56PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 02, 2011 at 10:04:45AM -0600, Grant Likely wrote:
> > Right now we can't do dynamic registration for on-chip devices in a
> > lot of cases because we don't have the infrastructure to hook up the
> > associated struct clks.
> 
> I've been wondering about this, and I don't see it as a blocking problem
> as you seem to be.
> 
> I assume platform devices have stable names when they're created from
> the device tree?  If yes, there's no problem having the DT start to
> describe the SoC specific devices _today_ - all that the clk API using
> clkdev requires is a stable device name.

Yes, the name is stable, but the name isn't the same as what board
support code currently uses for statically registered
platform_devices, and when I started on this work my goal has been to
have as little impact on existing platform support code.  I certainly
didn't want two sets of clock registrations, one with the names that
statically allocated platform devices use and one with the DT
generated names.

One solution would have been to add a "linux,devname" property or
something similar to each device node, but one of the things I'm very
careful about is to strongly avoid encoding Linux specific
implementation details into the DT.  History has shown that internal
details are subject to change over time, so there is less chance of
breaking a DT platform if the description is focused on the HW.  The
name Linux needs to hook up {clocks,regulators,etc} to a device very
much falls into this category.

That said, I stepped back and took another look at the problem after
receiving your email, and I've been looking at it all wrong.  I did
actually had a solution that matched up static device registrations
to DT nodes (of_platform_prepare()), but it was complex and ugly so I
wasn't very excited about it.

However, I've got a patch now that solves the problem in a much nicer
way.  If Linux needs specific devices to have specific names, then it
can pass in a lookup table that matches DT nodes to device names,
which is considerably simpler and it leaves the platform setup code in
control over how devices get instantiated.  I'll post the patches today
or tomorrow.

So, basically, all my problems on obtaining clocks are now completely
gone.  :-)

Ultimately, this is a short-term solution that allows devices to get
described in the DT without having to implement clock mappings at the
same time.  When clock mappings also get moved to the device tree, the
need for the lookup table simply goes away.

g.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[John Bonesio]

This patch makes it so the gpio's used by the Tegra audio driver are
initialized from device tree nodes.

Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/boot/dts/tegra-harmony.dts |    6 ++++++
 sound/soc/codecs/wm8903.c           |    5 +++++
 sound/soc/tegra/harmony.c           |   37 ++++++++++++++++++-----------------
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
index 217a7f0..fc61e02 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -55,6 +55,12 @@
 
 	harmony_audio: audio_card {
 		compatible = "nvidia,harmony-audio";
+
+		gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
+			<&gpio 178 0>, /* hp_det, gpio PW2 */
+			<&gpio 184 0>, /* int_mic_en, gpio PX0 */
+			<&gpio 185 0>; /* ext_mic_en, gpio PX1 */
+
 	};
 
 	serial@70006300 {
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index a10e3b9..9ad3532 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -1875,6 +1875,11 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
 			wm8903->gpio_chip.base = be32_to_cpup(prop);
 	}
 
+#if defined(CONFIG_OF_GPIO)
+	wm8903->gpio_chip.of_node = of_find_compatible_node(NULL, NULL,
+						"wlf,wm8903");
+#endif /* CONFIG_OF_GPIO */
+
 	ret = gpiochip_add(&wm8903->gpio_chip);
 	if (ret != 0)
 		dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret);
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
index faeec14..40c19f0 100644
--- a/sound/soc/tegra/harmony.c
+++ b/sound/soc/tegra/harmony.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 
 #include <mach/harmony_audio.h>
 
@@ -52,18 +53,6 @@
 
 #define DRV_NAME "tegra-snd-harmony"
 
-/* temporary - info will go into device tree */
-#define TEGRA_GPIO_PW2          178
-#define TEGRA_GPIO_PX0          184
-#define TEGRA_GPIO_PX1          185
-
-#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
-#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
-#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
-#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
-#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
-/* end temporary */
-
 #define GPIO_SPKR_EN    BIT(0)
 #define GPIO_INT_MIC_EN BIT(1)
 #define GPIO_EXT_MIC_EN BIT(2)
@@ -213,6 +202,18 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
 	struct harmony_audio_platform_data *pdata = harmony->pdata;
 	int ret;
 
+	if (pdata->gpio_spkr_en == -1)
+		pdata->gpio_spkr_en = of_get_gpio(card->dev->of_node, 0);
+
+	if (pdata->gpio_hp_det == -1)
+		pdata->gpio_hp_det = of_get_gpio(card->dev->of_node, 1);
+
+	if (pdata->gpio_int_mic_en == -1)
+		pdata->gpio_int_mic_en = of_get_gpio(card->dev->of_node, 2);
+
+	if (pdata->gpio_ext_mic_en == -1)
+		pdata->gpio_ext_mic_en = of_get_gpio(card->dev->of_node, 3);
+
 	ret = gpio_request(pdata->gpio_spkr_en, "spkr_en");
 	if (ret) {
 		dev_err(card->dev, "cannot get spkr_en gpio\n");
@@ -299,12 +300,12 @@ static struct snd_soc_card snd_soc_harmony = {
 	.num_links = 1,
 };
 
-/* temporary - put this into the device tree */
+/* empty pdata struct - filled in with device tree properties */
 static struct harmony_audio_platform_data harmony_audio_pdata = {
-	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
-	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
-	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
-	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
+	.gpio_spkr_en		= -1,
+	.gpio_hp_det		= -1,
+	.gpio_int_mic_en	= -1,
+	.gpio_ext_mic_en	= -1,
 };
 
 static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
@@ -328,7 +329,7 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
 #endif
 
 	if (pdev->dev.of_node) {
-		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
+		pdev->dev.platform_data = &harmony_audio_pdata;
 		pdata = pdev->dev.platform_data;
 	} else {
 		pdata = pdev->dev.platform_data;

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:
> This patch makes it so the gpio's used by the Tegra audio driver are
> initialized from device tree nodes.
> 
> Signed-off-by: John Bonesio<bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |    6 ++++++
>  sound/soc/codecs/wm8903.c           |    5 +++++
>  sound/soc/tegra/harmony.c           |   37 ++++++++++++++++++-----------------
>  3 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 217a7f0..fc61e02 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -55,6 +55,12 @@
>  
>  	harmony_audio: audio_card {
>  		compatible = "nvidia,harmony-audio";
> +
> +		gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
> +			<&gpio 178 0>, /* hp_det, gpio PW2 */
> +			<&gpio 184 0>, /* int_mic_en, gpio PX0 */
> +			<&gpio 185 0>; /* ext_mic_en, gpio PX1 */
> +
>  	};
>  
>  	serial@70006300 {
> diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
> index a10e3b9..9ad3532 100644
> --- a/sound/soc/codecs/wm8903.c
> +++ b/sound/soc/codecs/wm8903.c
> @@ -1875,6 +1875,11 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
>  			wm8903->gpio_chip.base = be32_to_cpup(prop);
>  	}
>  
> +#if defined(CONFIG_OF_GPIO)
> +	wm8903->gpio_chip.of_node = of_find_compatible_node(NULL, NULL,
> +						"wlf,wm8903");
> +#endif /* CONFIG_OF_GPIO */
> +

You shouldn't need to search for the node pointer here.  The of_node
should already be populated in the i2c device, and you can just use
that reference directly.

>  	ret = gpiochip_add(&wm8903->gpio_chip);
>  	if (ret != 0)
>  		dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret);
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index faeec14..40c19f0 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -34,6 +34,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/gpio.h>
> +#include <linux/of_gpio.h>
>  
>  #include <mach/harmony_audio.h>
>  
> @@ -52,18 +53,6 @@
>  
>  #define DRV_NAME "tegra-snd-harmony"
>  
> -/* temporary - info will go into device tree */
> -#define TEGRA_GPIO_PW2          178
> -#define TEGRA_GPIO_PX0          184
> -#define TEGRA_GPIO_PX1          185
> -
> -#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
> -#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
> -#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
> -#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
> -#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
> -/* end temporary */
> -
>  #define GPIO_SPKR_EN    BIT(0)
>  #define GPIO_INT_MIC_EN BIT(1)
>  #define GPIO_EXT_MIC_EN BIT(2)
> @@ -213,6 +202,18 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
>  	struct harmony_audio_platform_data *pdata = harmony->pdata;
>  	int ret;
>  
> +	if (pdata->gpio_spkr_en == -1)
> +		pdata->gpio_spkr_en = of_get_gpio(card->dev->of_node, 0);
> +
> +	if (pdata->gpio_hp_det == -1)
> +		pdata->gpio_hp_det = of_get_gpio(card->dev->of_node, 1);
> +
> +	if (pdata->gpio_int_mic_en == -1)
> +		pdata->gpio_int_mic_en = of_get_gpio(card->dev->of_node, 2);
> +
> +	if (pdata->gpio_ext_mic_en == -1)
> +		pdata->gpio_ext_mic_en = of_get_gpio(card->dev->of_node, 3);
> +
>  	ret = gpio_request(pdata->gpio_spkr_en, "spkr_en");
>  	if (ret) {
>  		dev_err(card->dev, "cannot get spkr_en gpio\n");
> @@ -299,12 +300,12 @@ static struct snd_soc_card snd_soc_harmony = {
>  	.num_links = 1,
>  };
>  
> -/* temporary - put this into the device tree */
> +/* empty pdata struct - filled in with device tree properties */

As already mentioned, don't use an empty static structure.  kzalloc()
one instead.

>  static struct harmony_audio_platform_data harmony_audio_pdata = {
> -	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
> -	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
> -	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
> -	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
> +	.gpio_spkr_en		= -1,
> +	.gpio_hp_det		= -1,
> +	.gpio_int_mic_en	= -1,
> +	.gpio_ext_mic_en	= -1,
>  };
>  
>  static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
> @@ -328,7 +329,7 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  #endif
>  
>  	if (pdev->dev.of_node) {
> -		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> +		pdev->dev.platform_data = &harmony_audio_pdata;
>  		pdata = pdev->dev.platform_data;
>  	} else {
>  		pdata = pdev->dev.platform_data;
> 

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:
> This patch makes it so the gpio's used by the Tegra audio driver are
> initialized from device tree nodes.
> 
> Signed-off-by: John Bonesio<bones@secretlab.ca>
> ---
> 
>  arch/arm/boot/dts/tegra-harmony.dts |    6 ++++++
>  sound/soc/codecs/wm8903.c           |    5 +++++
>  sound/soc/tegra/harmony.c           |   37 ++++++++++++++++++-----------------
>  3 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
> index 217a7f0..fc61e02 100644
> --- a/arch/arm/boot/dts/tegra-harmony.dts
> +++ b/arch/arm/boot/dts/tegra-harmony.dts
> @@ -55,6 +55,12 @@
>  
>  	harmony_audio: audio_card {
>  		compatible = "nvidia,harmony-audio";
> +
> +		gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
> +			<&gpio 178 0>, /* hp_det, gpio PW2 */
> +			<&gpio 184 0>, /* int_mic_en, gpio PX0 */
> +			<&gpio 185 0>; /* ext_mic_en, gpio PX1 */
> +
>  	};
>  
>  	serial at 70006300 {
> diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
> index a10e3b9..9ad3532 100644
> --- a/sound/soc/codecs/wm8903.c
> +++ b/sound/soc/codecs/wm8903.c
> @@ -1875,6 +1875,11 @@ static void wm8903_init_gpio(struct snd_soc_codec *codec)
>  			wm8903->gpio_chip.base = be32_to_cpup(prop);
>  	}
>  
> +#if defined(CONFIG_OF_GPIO)
> +	wm8903->gpio_chip.of_node = of_find_compatible_node(NULL, NULL,
> +						"wlf,wm8903");
> +#endif /* CONFIG_OF_GPIO */
> +

You shouldn't need to search for the node pointer here.  The of_node
should already be populated in the i2c device, and you can just use
that reference directly.

>  	ret = gpiochip_add(&wm8903->gpio_chip);
>  	if (ret != 0)
>  		dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret);
> diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c
> index faeec14..40c19f0 100644
> --- a/sound/soc/tegra/harmony.c
> +++ b/sound/soc/tegra/harmony.c
> @@ -34,6 +34,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/gpio.h>
> +#include <linux/of_gpio.h>
>  
>  #include <mach/harmony_audio.h>
>  
> @@ -52,18 +53,6 @@
>  
>  #define DRV_NAME "tegra-snd-harmony"
>  
> -/* temporary - info will go into device tree */
> -#define TEGRA_GPIO_PW2          178
> -#define TEGRA_GPIO_PX0          184
> -#define TEGRA_GPIO_PX1          185
> -
> -#define HARMONY_GPIO_WM8903(_x_)	(TEGRA_NR_GPIOS + (_x_))
> -#define TEGRA_GPIO_SPKR_EN		HARMONY_GPIO_WM8903(2)
> -#define TEGRA_GPIO_HP_DET		TEGRA_GPIO_PW2
> -#define TEGRA_GPIO_INT_MIC_EN		TEGRA_GPIO_PX0
> -#define TEGRA_GPIO_EXT_MIC_EN		TEGRA_GPIO_PX1
> -/* end temporary */
> -
>  #define GPIO_SPKR_EN    BIT(0)
>  #define GPIO_INT_MIC_EN BIT(1)
>  #define GPIO_EXT_MIC_EN BIT(2)
> @@ -213,6 +202,18 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
>  	struct harmony_audio_platform_data *pdata = harmony->pdata;
>  	int ret;
>  
> +	if (pdata->gpio_spkr_en == -1)
> +		pdata->gpio_spkr_en = of_get_gpio(card->dev->of_node, 0);
> +
> +	if (pdata->gpio_hp_det == -1)
> +		pdata->gpio_hp_det = of_get_gpio(card->dev->of_node, 1);
> +
> +	if (pdata->gpio_int_mic_en == -1)
> +		pdata->gpio_int_mic_en = of_get_gpio(card->dev->of_node, 2);
> +
> +	if (pdata->gpio_ext_mic_en == -1)
> +		pdata->gpio_ext_mic_en = of_get_gpio(card->dev->of_node, 3);
> +
>  	ret = gpio_request(pdata->gpio_spkr_en, "spkr_en");
>  	if (ret) {
>  		dev_err(card->dev, "cannot get spkr_en gpio\n");
> @@ -299,12 +300,12 @@ static struct snd_soc_card snd_soc_harmony = {
>  	.num_links = 1,
>  };
>  
> -/* temporary - put this into the device tree */
> +/* empty pdata struct - filled in with device tree properties */

As already mentioned, don't use an empty static structure.  kzalloc()
one instead.

>  static struct harmony_audio_platform_data harmony_audio_pdata = {
> -	.gpio_spkr_en		= TEGRA_GPIO_SPKR_EN,
> -	.gpio_hp_det		= TEGRA_GPIO_HP_DET,
> -	.gpio_int_mic_en	= TEGRA_GPIO_INT_MIC_EN,
> -	.gpio_ext_mic_en	= TEGRA_GPIO_EXT_MIC_EN,
> +	.gpio_spkr_en		= -1,
> +	.gpio_hp_det		= -1,
> +	.gpio_int_mic_en	= -1,
> +	.gpio_ext_mic_en	= -1,
>  };
>  
>  static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
> @@ -328,7 +329,7 @@ static __devinit int tegra_snd_harmony_probe(struct platform_device *pdev)
>  #endif
>  
>  	if (pdev->dev.of_node) {
> -		pdev->dev.platform_data = &harmony_audio_pdata; /* temporary */
> +		pdev->dev.platform_data = &harmony_audio_pdata;
>  		pdata = pdev->dev.platform_data;
>  	} else {
>  		pdata = pdev->dev.platform_data;
> 

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:

>  	harmony_audio: audio_card {
>  		compatible = "nvidia,harmony-audio";
> +
> +		gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
> +			<&gpio 178 0>, /* hp_det, gpio PW2 */
> +			<&gpio 184 0>, /* int_mic_en, gpio PX0 */
> +			<&gpio 185 0>; /* ext_mic_en, gpio PX1 */
> +

This is a step back from the usability of the existing platform data -
the platform data uses a series of individually named GPIOs while this
uses an array of GPIO numbers with magic indexes.  The fact that you
need comments explaining what the functions of the array elements are
is a bit of a red flag here.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:

>  	harmony_audio: audio_card {
>  		compatible = "nvidia,harmony-audio";
> +
> +		gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
> +			<&gpio 178 0>, /* hp_det, gpio PW2 */
> +			<&gpio 184 0>, /* int_mic_en, gpio PX0 */
> +			<&gpio 185 0>; /* ext_mic_en, gpio PX1 */
> +

This is a step back from the usability of the existing platform data -
the platform data uses a series of individually named GPIOs while this
uses an array of GPIO numbers with magic indexes.  The fact that you
need comments explaining what the functions of the array elements are
is a bit of a red flag here.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Olof Johansson]

On Fri, May 27, 2011 at 6:24 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:
>
>>       harmony_audio: audio_card {
>>               compatible = "nvidia,harmony-audio";
>> +
>> +             gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
>> +                     <&gpio 178 0>, /* hp_det, gpio PW2 */
>> +                     <&gpio 184 0>, /* int_mic_en, gpio PX0 */
>> +                     <&gpio 185 0>; /* ext_mic_en, gpio PX1 */
>> +
>
> This is a step back from the usability of the existing platform data -
> the platform data uses a series of individually named GPIOs while this
> uses an array of GPIO numbers with magic indexes.  The fact that you
> need comments explaining what the functions of the array elements are
> is a bit of a red flag here.

Agreed, I had similar concerns with the sdhci bindings where it used a
3-element array of gpios instead of the previous named ones. I was
told it's common practice to do it that way though? Seems like a step
backwards to me. :(


-Olof

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Olof Johansson]

On Fri, May 27, 2011 at 6:24 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, May 27, 2011 at 01:57:26PM -0700, John Bonesio wrote:
>
>> ? ? ? harmony_audio: audio_card {
>> ? ? ? ? ? ? ? compatible = "nvidia,harmony-audio";
>> +
>> + ? ? ? ? ? ? gpios = <&codec 226 0>, /* spkr_en, gpio wm8903 #2 */
>> + ? ? ? ? ? ? ? ? ? ? <&gpio 178 0>, /* hp_det, gpio PW2 */
>> + ? ? ? ? ? ? ? ? ? ? <&gpio 184 0>, /* int_mic_en, gpio PX0 */
>> + ? ? ? ? ? ? ? ? ? ? <&gpio 185 0>; /* ext_mic_en, gpio PX1 */
>> +
>
> This is a step back from the usability of the existing platform data -
> the platform data uses a series of individually named GPIOs while this
> uses an array of GPIO numbers with magic indexes. ?The fact that you
> need comments explaining what the functions of the array elements are
> is a bit of a red flag here.

Agreed, I had similar concerns with the sdhci bindings where it used a
3-element array of gpios instead of the previous named ones. I was
told it's common practice to do it that way though? Seems like a step
backwards to me. :(


-Olof

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
> On Fri, May 27, 2011 at 6:24 PM, Mark Brown

> > This is a step back from the usability of the existing platform data -
> > the platform data uses a series of individually named GPIOs while this
> > uses an array of GPIO numbers with magic indexes.  The fact that you
> > need comments explaining what the functions of the array elements are
> > is a bit of a red flag here.

> Agreed, I had similar concerns with the sdhci bindings where it used a
> 3-element array of gpios instead of the previous named ones. I was
> told it's common practice to do it that way though? Seems like a step
> backwards to me. :(

Interesting...  what was the reasoning behind this?  It's a definite
step backwards but it does explain my major concern with the new batch
of device tree patches.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
> On Fri, May 27, 2011 at 6:24 PM, Mark Brown

> > This is a step back from the usability of the existing platform data -
> > the platform data uses a series of individually named GPIOs while this
> > uses an array of GPIO numbers with magic indexes. ?The fact that you
> > need comments explaining what the functions of the array elements are
> > is a bit of a red flag here.

> Agreed, I had similar concerns with the sdhci bindings where it used a
> 3-element array of gpios instead of the previous named ones. I was
> told it's common practice to do it that way though? Seems like a step
> backwards to me. :(

Interesting...  what was the reasoning behind this?  It's a definite
step backwards but it does explain my major concern with the new batch
of device tree patches.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
> > On Fri, May 27, 2011 at 6:24 PM, Mark Brown
> 
> > > This is a step back from the usability of the existing platform data -
> > > the platform data uses a series of individually named GPIOs while this
> > > uses an array of GPIO numbers with magic indexes.  The fact that you
> > > need comments explaining what the functions of the array elements are
> > > is a bit of a red flag here.
> 
> > Agreed, I had similar concerns with the sdhci bindings where it used a
> > 3-element array of gpios instead of the previous named ones. I was
> > told it's common practice to do it that way though? Seems like a step
> > backwards to me. :(
> 
> Interesting...  what was the reasoning behind this?  It's a definite
> step backwards but it does explain my major concern with the new batch
> of device tree patches.

The binding for gpios was defined a few years ago and it is in fairly
wide use within the powerpc sphere.  The design followed the pattern
established for specifying irqs, and in that regard satisfied the
principle of least surprise.

That said, it isn't a very large leap to go from a single 'gpios'
property to allowing multiple named gpios properties with meaningful
names, particularly if they are fully specified by the device
binding, and they follow exactly the same binding semantics as the
existing 'gpios' proprety (phandle + gpio specifier).

Personally, I'm /cautious/ about saying okay to extending the binding,
simply because once the extension is in use it is really hard to go
back on it, but I cannot think of any reason why this particular case
wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

g.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
> > On Fri, May 27, 2011 at 6:24 PM, Mark Brown
> 
> > > This is a step back from the usability of the existing platform data -
> > > the platform data uses a series of individually named GPIOs while this
> > > uses an array of GPIO numbers with magic indexes. ?The fact that you
> > > need comments explaining what the functions of the array elements are
> > > is a bit of a red flag here.
> 
> > Agreed, I had similar concerns with the sdhci bindings where it used a
> > 3-element array of gpios instead of the previous named ones. I was
> > told it's common practice to do it that way though? Seems like a step
> > backwards to me. :(
> 
> Interesting...  what was the reasoning behind this?  It's a definite
> step backwards but it does explain my major concern with the new batch
> of device tree patches.

The binding for gpios was defined a few years ago and it is in fairly
wide use within the powerpc sphere.  The design followed the pattern
established for specifying irqs, and in that regard satisfied the
principle of least surprise.

That said, it isn't a very large leap to go from a single 'gpios'
property to allowing multiple named gpios properties with meaningful
names, particularly if they are fully specified by the device
binding, and they follow exactly the same binding semantics as the
existing 'gpios' proprety (phandle + gpio specifier).

Personally, I'm /cautious/ about saying okay to extending the binding,
simply because once the extension is in use it is really hard to go
back on it, but I cannot think of any reason why this particular case
wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

g.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 5/29/2011 8:11 PM, Grant Likely wrote:
> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
>> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
>>> On Fri, May 27, 2011 at 6:24 PM, Mark Brown
>>
>>>> This is a step back from the usability of the existing platform data -
>>>> the platform data uses a series of individually named GPIOs while this
>>>> uses an array of GPIO numbers with magic indexes.  The fact that you
>>>> need comments explaining what the functions of the array elements are
>>>> is a bit of a red flag here.
>>
>>> Agreed, I had similar concerns with the sdhci bindings where it used a
>>> 3-element array of gpios instead of the previous named ones. I was
>>> told it's common practice to do it that way though? Seems like a step
>>> backwards to me. :(
>>
>> Interesting...  what was the reasoning behind this?  It's a definite
>> step backwards but it does explain my major concern with the new batch
>> of device tree patches.
>
> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.
>
> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).
>
> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?


I'm currently dealing with an SoC that has over a hundred GPIOs. 
Whatever we choose, I think it should be able to handle an insane number 
of GPIOs without getting any more cumbersome that is necessary.


>
> g.
>

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 5/29/2011 8:11 PM, Grant Likely wrote:
> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
>> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
>>> On Fri, May 27, 2011 at 6:24 PM, Mark Brown
>>
>>>> This is a step back from the usability of the existing platform data -
>>>> the platform data uses a series of individually named GPIOs while this
>>>> uses an array of GPIO numbers with magic indexes.  The fact that you
>>>> need comments explaining what the functions of the array elements are
>>>> is a bit of a red flag here.
>>
>>> Agreed, I had similar concerns with the sdhci bindings where it used a
>>> 3-element array of gpios instead of the previous named ones. I was
>>> told it's common practice to do it that way though? Seems like a step
>>> backwards to me. :(
>>
>> Interesting...  what was the reasoning behind this?  It's a definite
>> step backwards but it does explain my major concern with the new batch
>> of device tree patches.
>
> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.
>
> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).
>
> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?


I'm currently dealing with an SoC that has over a hundred GPIOs. 
Whatever we choose, I think it should be able to handle an insane number 
of GPIOs without getting any more cumbersome that is necessary.


>
> g.
>

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 12:18 AM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> On 5/29/2011 8:11 PM, Grant Likely wrote:
>>
>> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
>>>
>>> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
>>>>
>>>> On Fri, May 27, 2011 at 6:24 PM, Mark Brown
>>>
>>>>> This is a step back from the usability of the existing platform data -
>>>>> the platform data uses a series of individually named GPIOs while this
>>>>> uses an array of GPIO numbers with magic indexes.  The fact that you
>>>>> need comments explaining what the functions of the array elements are
>>>>> is a bit of a red flag here.
>>>
>>>> Agreed, I had similar concerns with the sdhci bindings where it used a
>>>> 3-element array of gpios instead of the previous named ones. I was
>>>> told it's common practice to do it that way though? Seems like a step
>>>> backwards to me. :(
>>>
>>> Interesting...  what was the reasoning behind this?  It's a definite
>>> step backwards but it does explain my major concern with the new batch
>>> of device tree patches.
>>
>> The binding for gpios was defined a few years ago and it is in fairly
>> wide use within the powerpc sphere.  The design followed the pattern
>> established for specifying irqs, and in that regard satisfied the
>> principle of least surprise.
>>
>> That said, it isn't a very large leap to go from a single 'gpios'
>> property to allowing multiple named gpios properties with meaningful
>> names, particularly if they are fully specified by the device
>> binding, and they follow exactly the same binding semantics as the
>> existing 'gpios' proprety (phandle + gpio specifier).
>>
>> Personally, I'm /cautious/ about saying okay to extending the binding,
>> simply because once the extension is in use it is really hard to go
>> back on it, but I cannot think of any reason why this particular case
>> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?
>
>
> I'm currently dealing with an SoC that has over a hundred GPIOs. Whatever we
> choose, I think it should be able to handle an insane number of GPIOs
> without getting any more cumbersome that is necessary.

That's pretty common, and I don't think it will be a problem; either
with the current binding, or the proposed extension.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 12:18 AM, Mitch Bradley <wmb@firmworks.com> wrote:
> On 5/29/2011 8:11 PM, Grant Likely wrote:
>>
>> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:
>>>
>>> On Sun, May 29, 2011 at 08:11:34PM -0700, Olof Johansson wrote:
>>>>
>>>> On Fri, May 27, 2011 at 6:24 PM, Mark Brown
>>>
>>>>> This is a step back from the usability of the existing platform data -
>>>>> the platform data uses a series of individually named GPIOs while this
>>>>> uses an array of GPIO numbers with magic indexes. ?The fact that you
>>>>> need comments explaining what the functions of the array elements are
>>>>> is a bit of a red flag here.
>>>
>>>> Agreed, I had similar concerns with the sdhci bindings where it used a
>>>> 3-element array of gpios instead of the previous named ones. I was
>>>> told it's common practice to do it that way though? Seems like a step
>>>> backwards to me. :(
>>>
>>> Interesting... ?what was the reasoning behind this? ?It's a definite
>>> step backwards but it does explain my major concern with the new batch
>>> of device tree patches.
>>
>> The binding for gpios was defined a few years ago and it is in fairly
>> wide use within the powerpc sphere. ?The design followed the pattern
>> established for specifying irqs, and in that regard satisfied the
>> principle of least surprise.
>>
>> That said, it isn't a very large leap to go from a single 'gpios'
>> property to allowing multiple named gpios properties with meaningful
>> names, particularly if they are fully specified by the device
>> binding, and they follow exactly the same binding semantics as the
>> existing 'gpios' proprety (phandle + gpio specifier).
>>
>> Personally, I'm /cautious/ about saying okay to extending the binding,
>> simply because once the extension is in use it is really hard to go
>> back on it, but I cannot think of any reason why this particular case
>> wouldn't be a good idea. ?Anyone have thoughts on this? ?Ben? ?Mitch?
>
>
> I'm currently dealing with an SoC that has over a hundred GPIOs. Whatever we
> choose, I think it should be able to handle an insane number of GPIOs
> without getting any more cumbersome that is necessary.

That's pretty common, and I don't think it will be a problem; either
with the current binding, or the proposed extension.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:

> I'm currently dealing with an SoC that has over a hundred GPIOs.
> Whatever we choose, I think it should be able to handle an insane
> number of GPIOs without getting any more cumbersome that is
> necessary.

This is *consumer* side GPIOs, not bindings for the device providing the
GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
many of them will be block functions so you'd have a binding with an
array for things like "databus" and "addrbus".

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:

> I'm currently dealing with an SoC that has over a hundred GPIOs.
> Whatever we choose, I think it should be able to handle an insane
> number of GPIOs without getting any more cumbersome that is
> necessary.

This is *consumer* side GPIOs, not bindings for the device providing the
GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
many of them will be block functions so you'd have a binding with an
array for things like "databus" and "addrbus".

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 1:01 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
>
>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>> Whatever we choose, I think it should be able to handle an insane
>> number of GPIOs without getting any more cumbersome that is
>> necessary.
>
> This is *consumer* side GPIOs, not bindings for the device providing the
> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

yes.

g.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 1:01 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
>
>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>> Whatever we choose, I think it should be able to handle an insane
>> number of GPIOs without getting any more cumbersome that is
>> necessary.
>
> This is *consumer* side GPIOs, not bindings for the device providing the
> GPIOs. ?If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

yes.

g.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Segher Boessenkool]

>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>> Whatever we choose, I think it should be able to handle an insane
>> number of GPIOs without getting any more cumbersome that is
>> necessary.
>
> This is *consumer* side GPIOs, not bindings for the device providing 
> the
> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

But please name them like "databus-gpio", so that it is obvious what it
is.  Also have to think about how this will work with multiple GPIO
controllers: do you require the GPIO controller node to be part of every
GPIO description, or do you do some "gpio-parent" scheme as well, how
does that interact with not having a single array of GPIOs?

Better write this down as a binding, before committing to it :-)


Segher

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Segher Boessenkool]

>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>> Whatever we choose, I think it should be able to handle an insane
>> number of GPIOs without getting any more cumbersome that is
>> necessary.
>
> This is *consumer* side GPIOs, not bindings for the device providing 
> the
> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

But please name them like "databus-gpio", so that it is obvious what it
is.  Also have to think about how this will work with multiple GPIO
controllers: do you require the GPIO controller node to be part of every
GPIO description, or do you do some "gpio-parent" scheme as well, how
does that interact with not having a single array of GPIOs?

Better write this down as a binding, before committing to it :-)


Segher

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 12:54 PM, Segher Boessenkool
<segher-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
>>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>>> Whatever we choose, I think it should be able to handle an insane
>>> number of GPIOs without getting any more cumbersome that is
>>> necessary.
>>
>> This is *consumer* side GPIOs, not bindings for the device providing the
>> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
>> many of them will be block functions so you'd have a binding with an
>> array for things like "databus" and "addrbus".
>
> But please name them like "databus-gpio", so that it is obvious what it
> is.  Also have to think about how this will work with multiple GPIO
> controllers: do you require the GPIO controller node to be part of every
> GPIO description, or do you do some "gpio-parent" scheme as well, how
> does that interact with not having a single array of GPIOs?
>
> Better write this down as a binding, before committing to it :-)

Here's my thinking: Exactly the same binding for "gpios" as is now,
except can use different property names, like "chipsel-gpios" or
"wp-gpio".  Here is a draft patch to the binding:

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
b/Documentation/devicetree/bindings/gpio/gpio.txt
index edaa84d..21dd4f9 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -4,17 +4,45 @@ Specifying GPIO information for devices
 1) gpios property
 -----------------

-Nodes that makes use of GPIOs should define them using `gpios' property,
-format of which is: <&gpio-controller1-phandle gpio1-specifier
-		     &gpio-controller2-phandle gpio2-specifier
-		     0 /* holes are permitted, means no GPIO 3 */
-		     &gpio-controller4-phandle gpio4-specifier
-		     ...>;
+Nodes that makes use of GPIOs should specify them using one or more
+properties, each containing a 'gpio-list':

-Note that gpio-specifier length is controller dependent.
+	gpio-list ::= <single-gpio> [gpio-list]
+	single-gpio ::= <gpio-phandle> <gpio-specifier>
+	gpio-phandle : phandle to gpio controller node
+	gpio-specifier : Array of #gpio-cells specifying specific gpio
+			 (controller specific)
+
+GPIO properties should be named either "gpios" or "*-gpio".  Exact
+meaning of each gpio property must be documented in the device tree
+binding for each device.
+
+For example, the following could be used to describe gpios pins to use
+as chip select lines; with chip selects 0, 1 and 3 populated, and chip
+select 2 left empty:
+
+	gpio1: gpio1 {
+		gpio-controller
+		 #gpio-cells = <2>;
+	};
+	gpio2: gpio2 {
+		gpio-controller
+		 #gpio-cells = <1>;
+	};
+	[...]
+	 chipsel-gpio = <&gpio1 12 0>,
+			<&gpio1 13 0>,
+			<0>, /* holes are permitted, means no GPIO 2 */
+			<&gpio2 2>;
+
+Note that gpio-specifier length is controller dependent.  In the
+above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
+only uses one.

 gpio-specifier may encode: bank, pin position inside the bank,
 whether pin is open-drain and whether pin is logically inverted.
+Exact meaning of each specifier cell is controller specific, and must
+be documented in the device tree binding for the device.

 Example of the node using GPIOs:

@@ -28,8 +56,8 @@ and empty GPIO flags as accepted by the "qe_pio_e"
gpio-controller.
 2) gpio-controller nodes
 ------------------------

-Every GPIO controller node must have #gpio-cells property defined,
-this information will be used to translate gpio-specifiers.
+Every GPIO controller node must both an empty "gpio-controller"
+property, and have #gpio-cells contain the size of the gpio-specifier.

 Example of two SOC GPIO banks defined as gpio-controller nodes:

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 12:54 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>>> Whatever we choose, I think it should be able to handle an insane
>>> number of GPIOs without getting any more cumbersome that is
>>> necessary.
>>
>> This is *consumer* side GPIOs, not bindings for the device providing the
>> GPIOs. ?If a single device needs to use hundreds of GPIOs I'd expect
>> many of them will be block functions so you'd have a binding with an
>> array for things like "databus" and "addrbus".
>
> But please name them like "databus-gpio", so that it is obvious what it
> is. ?Also have to think about how this will work with multiple GPIO
> controllers: do you require the GPIO controller node to be part of every
> GPIO description, or do you do some "gpio-parent" scheme as well, how
> does that interact with not having a single array of GPIOs?
>
> Better write this down as a binding, before committing to it :-)

Here's my thinking: Exactly the same binding for "gpios" as is now,
except can use different property names, like "chipsel-gpios" or
"wp-gpio".  Here is a draft patch to the binding:

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
b/Documentation/devicetree/bindings/gpio/gpio.txt
index edaa84d..21dd4f9 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -4,17 +4,45 @@ Specifying GPIO information for devices
 1) gpios property
 -----------------

-Nodes that makes use of GPIOs should define them using `gpios' property,
-format of which is: <&gpio-controller1-phandle gpio1-specifier
-		     &gpio-controller2-phandle gpio2-specifier
-		     0 /* holes are permitted, means no GPIO 3 */
-		     &gpio-controller4-phandle gpio4-specifier
-		     ...>;
+Nodes that makes use of GPIOs should specify them using one or more
+properties, each containing a 'gpio-list':

-Note that gpio-specifier length is controller dependent.
+	gpio-list ::= <single-gpio> [gpio-list]
+	single-gpio ::= <gpio-phandle> <gpio-specifier>
+	gpio-phandle : phandle to gpio controller node
+	gpio-specifier : Array of #gpio-cells specifying specific gpio
+			 (controller specific)
+
+GPIO properties should be named either "gpios" or "*-gpio".  Exact
+meaning of each gpio property must be documented in the device tree
+binding for each device.
+
+For example, the following could be used to describe gpios pins to use
+as chip select lines; with chip selects 0, 1 and 3 populated, and chip
+select 2 left empty:
+
+	gpio1: gpio1 {
+		gpio-controller
+		 #gpio-cells = <2>;
+	};
+	gpio2: gpio2 {
+		gpio-controller
+		 #gpio-cells = <1>;
+	};
+	[...]
+	 chipsel-gpio = <&gpio1 12 0>,
+			<&gpio1 13 0>,
+			<0>, /* holes are permitted, means no GPIO 2 */
+			<&gpio2 2>;
+
+Note that gpio-specifier length is controller dependent.  In the
+above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
+only uses one.

 gpio-specifier may encode: bank, pin position inside the bank,
 whether pin is open-drain and whether pin is logically inverted.
+Exact meaning of each specifier cell is controller specific, and must
+be documented in the device tree binding for the device.

 Example of the node using GPIOs:

@@ -28,8 +56,8 @@ and empty GPIO flags as accepted by the "qe_pio_e"
gpio-controller.
 2) gpio-controller nodes
 ------------------------

-Every GPIO controller node must have #gpio-cells property defined,
-this information will be used to translate gpio-specifiers.
+Every GPIO controller node must both an empty "gpio-controller"
+property, and have #gpio-cells contain the size of the gpio-specifier.

 Example of two SOC GPIO banks defined as gpio-controller nodes:

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

Perhaps the interrupt-mapping binding is not the best model.  Interrupt 
hardware in general is hierarchical but is not isomorphic to the 
physical addressing hierarchy of the device tree.

GPIOs share the need to "point across the tree to different nodes", but 
it is unclear that there is a need for a entirely different hierarchy.

That suggests the possibility of using the device tree addressing 
mechanism as much as possible.  Normal device tree addresses could be 
used to specify GPIO numbers.

Suppose we have:

	gpio-controller1: gpio-controller {
                 #address-cells = <2>;
		#mode-cells = <2>;
                 gpio1: gpio@12,0 {
                     reg = <12 0>;
                     mode = <55 66>;
                     usage = "Audio Codec chip select";  /* Optional */
                 }
	};
	gpio-controller2: gpio-controller {
                  #address-cells = <1>;
		 #mode-cells = <1>;
                  gpio2: gpio@4 {
                      reg = <4>;
                      #mode-cells = <1>;
                  }
	};
	[...]
	chipsel-gpio =  <&gpio1>,
			<&gpio-controller1 13 0 55 77>,
			<0>, /* holes are permitted, means no GPIO 2 */
			<&gpio2 88>,
                         <&gpio-controller2 5 1>;


A GPIO spec consist of:

1) A reference to either a gpio-controller or a gpio device node.

2) 0 or more address cells, according to the value of #address-cells in 
the referenced node.  If the node has no #address-cells property, the 
value is assumed to be 0.

3) 0 or more mode cells, according to the value of #mode-cells in the 
referenced node.

In the example, the chipsel-gpio specs are interpreted as:

<&gpio1>  -  refers to a pre-bound gpio device node, in which the 
address (12 0) and mode (55 66) are specified within that node.

<&gpio-controller1 13 0 55 77>  -  refers to a GPIO controller node, 
specifing the address (13 0) and the mode (55 77) in the client's GPIO spec.

<gpio2>  -  another reference to a gpio node on a different controller. 
  In this case the address (4) is bound in the gpio node, but the mode 
(88) is passed in from the client's GPIO spec.

<&gpio-controller2 5 1>  -  another reference to a controller node, with 
a one-cell address (5) and a one-cell mode (1), according to the values 
of #address-cells and #mode-cells in that controller node.

I expect that the "pre-bound gpio node" case would get a lot of use in 
practice, as it lets you isolate the client from the details of the 
interrupt controller addressing and modes.  In my experience, GPIOs 
often get reassigned between revisions of the same board, especially 
early in the development cycle.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

Perhaps the interrupt-mapping binding is not the best model.  Interrupt 
hardware in general is hierarchical but is not isomorphic to the 
physical addressing hierarchy of the device tree.

GPIOs share the need to "point across the tree to different nodes", but 
it is unclear that there is a need for a entirely different hierarchy.

That suggests the possibility of using the device tree addressing 
mechanism as much as possible.  Normal device tree addresses could be 
used to specify GPIO numbers.

Suppose we have:

	gpio-controller1: gpio-controller {
                 #address-cells = <2>;
		#mode-cells = <2>;
                 gpio1: gpio at 12,0 {
                     reg = <12 0>;
                     mode = <55 66>;
                     usage = "Audio Codec chip select";  /* Optional */
                 }
	};
	gpio-controller2: gpio-controller {
                  #address-cells = <1>;
		 #mode-cells = <1>;
                  gpio2: gpio at 4 {
                      reg = <4>;
                      #mode-cells = <1>;
                  }
	};
	[...]
	chipsel-gpio =  <&gpio1>,
			<&gpio-controller1 13 0 55 77>,
			<0>, /* holes are permitted, means no GPIO 2 */
			<&gpio2 88>,
                         <&gpio-controller2 5 1>;


A GPIO spec consist of:

1) A reference to either a gpio-controller or a gpio device node.

2) 0 or more address cells, according to the value of #address-cells in 
the referenced node.  If the node has no #address-cells property, the 
value is assumed to be 0.

3) 0 or more mode cells, according to the value of #mode-cells in the 
referenced node.

In the example, the chipsel-gpio specs are interpreted as:

<&gpio1>  -  refers to a pre-bound gpio device node, in which the 
address (12 0) and mode (55 66) are specified within that node.

<&gpio-controller1 13 0 55 77>  -  refers to a GPIO controller node, 
specifing the address (13 0) and the mode (55 77) in the client's GPIO spec.

<gpio2>  -  another reference to a gpio node on a different controller. 
  In this case the address (4) is bound in the gpio node, but the mode 
(88) is passed in from the client's GPIO spec.

<&gpio-controller2 5 1>  -  another reference to a controller node, with 
a one-cell address (5) and a one-cell mode (1), according to the values 
of #address-cells and #mode-cells in that controller node.

I expect that the "pre-bound gpio node" case would get a lot of use in 
practice, as it lets you isolate the client from the details of the 
interrupt controller addressing and modes.  In my experience, GPIOs 
often get reassigned between revisions of the same board, especially 
early in the development cycle.

RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
> ...
> GPIOs share the need to "point across the tree to different nodes", but
> it is unclear that there is a need for a entirely different hierarchy.
> 
> That suggests the possibility of using the device tree addressing
> mechanism as much as possible.  Normal device tree addresses could be
> used to specify GPIO numbers.
> 
> Suppose we have:
> 
>     gpio-controller1: gpio-controller {
>         #address-cells = <2>;
>         #mode-cells = <2>;
>         gpio1: gpio@12,0 {
>             reg = <12 0>;
>             mode = <55 66>;
>             usage = "Audio Codec chip select";  /* Optional */
>         }
>     };
>     gpio-controller2: gpio-controller {
>         #address-cells = <1>;
>         #mode-cells = <1>;
>         gpio2: gpio@4 {
>             reg = <4>;
>             #mode-cells = <1>;
>         }
>     };

A quick note on the way that Tegra's devicetree files are broken up in
Grant's devicetree/test branch:

* There's "tegra250.dtsi" which defines everything on the Tegra SoC
  itself.
* There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
  And additionally:
  ** Defines all devices on the board
  ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
     board.

I like this model, because it shares the complete definition of the
Tegra SoC in a single file, rather than duplicating it with cut/paste
into every board file.

As such, the code quoted above would be in tegra250.dtsi.

This has two relevant implications here:

1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
naming of those GPIO pins, and not any board-specific naming, e.g.
"tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
be at the client/reference site, not the GPIO definition site.

2) The GPIO mode should be defined by the client/user of the GPIO, not
the SoC's definition of that GPIO.

Without those two conditions, we couldn't share anything through
tegra250.dtsi.

Re-iteration of these implications below.

>     [...]
>     chipsel-gpio =  <&gpio1>,
>         <&gpio-controller1 13 0 55 77>,
>         <0>, /* holes are permitted, means no GPIO 2 */
>         <&gpio2 88>,
>         <&gpio-controller2 5 1>;

> A GPIO spec consist of:
> 
> 1) A reference to either a gpio-controller or a gpio device node.
> 
> 2) 0 or more address cells, according to the value of #address-cells in
> the referenced node.  If the node has no #address-cells property, the
> value is assumed to be 0.

I'd expect address cells only if referencing a gpio-controller; if
referencing a gpio device node, the address would be implicit.

> 3) 0 or more mode cells, according to the value of #mode-cells in the
> referenced node.

Yes, I agree. Although, I think your (3) is inconsistent with the gpio
controller definitions you wrote above, which include the mode
definitions in the controller instead of the user.

> In the example, the chipsel-gpio specs are interpreted as:
> 
> <&gpio1>  -  refers to a pre-bound gpio device node, in which the
> address (12 0) and mode (55 66) are specified within that node.

Just re-iterating: I'd prefer mode to be solely in the reference, and
not in the gpio controller.

Does this SoC/board segregation make sense to everyone? Obviously it
does to me:-)

-- 
nvpublic

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
> ...
> GPIOs share the need to "point across the tree to different nodes", but
> it is unclear that there is a need for a entirely different hierarchy.
> 
> That suggests the possibility of using the device tree addressing
> mechanism as much as possible.  Normal device tree addresses could be
> used to specify GPIO numbers.
> 
> Suppose we have:
> 
>     gpio-controller1: gpio-controller {
>         #address-cells = <2>;
>         #mode-cells = <2>;
>         gpio1: gpio at 12,0 {
>             reg = <12 0>;
>             mode = <55 66>;
>             usage = "Audio Codec chip select";  /* Optional */
>         }
>     };
>     gpio-controller2: gpio-controller {
>         #address-cells = <1>;
>         #mode-cells = <1>;
>         gpio2: gpio at 4 {
>             reg = <4>;
>             #mode-cells = <1>;
>         }
>     };

A quick note on the way that Tegra's devicetree files are broken up in
Grant's devicetree/test branch:

* There's "tegra250.dtsi" which defines everything on the Tegra SoC
  itself.
* There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
  And additionally:
  ** Defines all devices on the board
  ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
     board.

I like this model, because it shares the complete definition of the
Tegra SoC in a single file, rather than duplicating it with cut/paste
into every board file.

As such, the code quoted above would be in tegra250.dtsi.

This has two relevant implications here:

1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
naming of those GPIO pins, and not any board-specific naming, e.g.
"tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
be at the client/reference site, not the GPIO definition site.

2) The GPIO mode should be defined by the client/user of the GPIO, not
the SoC's definition of that GPIO.

Without those two conditions, we couldn't share anything through
tegra250.dtsi.

Re-iteration of these implications below.

>     [...]
>     chipsel-gpio =  <&gpio1>,
>         <&gpio-controller1 13 0 55 77>,
>         <0>, /* holes are permitted, means no GPIO 2 */
>         <&gpio2 88>,
>         <&gpio-controller2 5 1>;

> A GPIO spec consist of:
> 
> 1) A reference to either a gpio-controller or a gpio device node.
> 
> 2) 0 or more address cells, according to the value of #address-cells in
> the referenced node.  If the node has no #address-cells property, the
> value is assumed to be 0.

I'd expect address cells only if referencing a gpio-controller; if
referencing a gpio device node, the address would be implicit.

> 3) 0 or more mode cells, according to the value of #mode-cells in the
> referenced node.

Yes, I agree. Although, I think your (3) is inconsistent with the gpio
controller definitions you wrote above, which include the mode
definitions in the controller instead of the user.

> In the example, the chipsel-gpio specs are interpreted as:
> 
> <&gpio1>  -  refers to a pre-bound gpio device node, in which the
> address (12 0) and mode (55 66) are specified within that node.

Just re-iterating: I'd prefer mode to be solely in the reference, and
not in the gpio controller.

Does this SoC/board segregation make sense to everyone? Obviously it
does to me:-)

-- 
nvpublic

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 5/31/2011 7:55 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>> ...
>> GPIOs share the need to "point across the tree to different nodes", but
>> it is unclear that there is a need for a entirely different hierarchy.
>>
>> That suggests the possibility of using the device tree addressing
>> mechanism as much as possible.  Normal device tree addresses could be
>> used to specify GPIO numbers.
>>
>> Suppose we have:
>>
>>      gpio-controller1: gpio-controller {
>>          #address-cells =<2>;
>>          #mode-cells =<2>;
>>          gpio1: gpio@12,0 {
>>              reg =<12 0>;
>>              mode =<55 66>;
>>              usage = "Audio Codec chip select";  /* Optional */
>>          }
>>      };
>>      gpio-controller2: gpio-controller {
>>          #address-cells =<1>;
>>          #mode-cells =<1>;
>>          gpio2: gpio@4 {
>>              reg =<4>;
>>              #mode-cells =<1>;
>>          }
>>      };
>
> A quick note on the way that Tegra's devicetree files are broken up in
> Grant's devicetree/test branch:
>
> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>    itself.
> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>    And additionally:
>    ** Defines all devices on the board
>    ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>       board.
>
> I like this model, because it shares the complete definition of the
> Tegra SoC in a single file, rather than duplicating it with cut/paste
> into every board file.
>
> As such, the code quoted above would be in tegra250.dtsi.
>
> This has two relevant implications here:
>
> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> naming of those GPIO pins, and not any board-specific naming, e.g.
> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> be at the client/reference site, not the GPIO definition site.
>
> 2) The GPIO mode should be defined by the client/user of the GPIO, not
> the SoC's definition of that GPIO.
>
> Without those two conditions, we couldn't share anything through
> tegra250.dtsi.
>
> Re-iteration of these implications below.
>
>>      [...]
>>      chipsel-gpio =<&gpio1>,
>>          <&gpio-controller1 13 0 55 77>,
>>          <0>, /* holes are permitted, means no GPIO 2 */
>>          <&gpio2 88>,
>>          <&gpio-controller2 5 1>;
>
>> A GPIO spec consist of:
>>
>> 1) A reference to either a gpio-controller or a gpio device node.
>>
>> 2) 0 or more address cells, according to the value of #address-cells in
>> the referenced node.  If the node has no #address-cells property, the
>> value is assumed to be 0.
>
> I'd expect address cells only if referencing a gpio-controller; if
> referencing a gpio device node, the address would be implicit.


Yes.  But I think it's better if there is a single rule for interpreting 
the GPIO spec, namely look at the #address-cells property, rather than 
deciding implicitly based on the type of the referent node.

>
>> 3) 0 or more mode cells, according to the value of #mode-cells in the
>> referenced node.
>
> Yes, I agree. Although, I think your (3) is inconsistent with the gpio
> controller definitions you wrote above, which include the mode
> definitions in the controller instead of the user.

Hmmm.  I think I got the example right.

Both of the gpio controller definitions have explicit "#address-cells" 
and "#mode-cells" properties, and neither has "mode".  Both references 
to controller nodes have mode values in the user spec.

gpio1 has "mode" but not "#mode-cells", and the reference to it has no 
mode value.

gpio2 has "#mode-cells=1" but not "mode", and the reference to it has 
one mode value.

Am I missing something?

>
>> In the example, the chipsel-gpio specs are interpreted as:
>>
>> <&gpio1>   -  refers to a pre-bound gpio device node, in which the
>> address (12 0) and mode (55 66) are specified within that node.
>
> Just re-iterating: I'd prefer mode to be solely in the reference, and
> not in the gpio controller.

I agree that it doesn't make much sense for a controller node to specify 
the mode.  My example doesn't do that.  The only mode value is in an 
individual gpio node, not a controller node.

 From the standpoint of a SoC manufacturer, specifying modes in the 
reference is probably a good idea.  But it's not necessarily best in all 
cases.  If the focus of attention is a board design with numerous 
variants and revisions, "binding" the modes of specific gpio pins 
according to the board wiring may be the right thing.

The way I specified it lets you choose.


>
> Does this SoC/board segregation make sense to everyone? Obviously it
> does to me:-)

It makes perfect sense from one viewpoint, but I think that board 
vendors may have a different focus.  The flexibility to specify a mode 
in either place costs little, as the parsing rule is definite and 
straightforward.  SoC vendors are free to defer mode decisions until 
later, by omitting "mode" and supplying "#mode-cells" in their device 
tree templates.  Board vendors could choose either to use the SoC 
vendor's template verbatim, or to amend it with additional 
board-specific information.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 5/31/2011 7:55 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>> ...
>> GPIOs share the need to "point across the tree to different nodes", but
>> it is unclear that there is a need for a entirely different hierarchy.
>>
>> That suggests the possibility of using the device tree addressing
>> mechanism as much as possible.  Normal device tree addresses could be
>> used to specify GPIO numbers.
>>
>> Suppose we have:
>>
>>      gpio-controller1: gpio-controller {
>>          #address-cells =<2>;
>>          #mode-cells =<2>;
>>          gpio1: gpio at 12,0 {
>>              reg =<12 0>;
>>              mode =<55 66>;
>>              usage = "Audio Codec chip select";  /* Optional */
>>          }
>>      };
>>      gpio-controller2: gpio-controller {
>>          #address-cells =<1>;
>>          #mode-cells =<1>;
>>          gpio2: gpio at 4 {
>>              reg =<4>;
>>              #mode-cells =<1>;
>>          }
>>      };
>
> A quick note on the way that Tegra's devicetree files are broken up in
> Grant's devicetree/test branch:
>
> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>    itself.
> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>    And additionally:
>    ** Defines all devices on the board
>    ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>       board.
>
> I like this model, because it shares the complete definition of the
> Tegra SoC in a single file, rather than duplicating it with cut/paste
> into every board file.
>
> As such, the code quoted above would be in tegra250.dtsi.
>
> This has two relevant implications here:
>
> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> naming of those GPIO pins, and not any board-specific naming, e.g.
> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> be at the client/reference site, not the GPIO definition site.
>
> 2) The GPIO mode should be defined by the client/user of the GPIO, not
> the SoC's definition of that GPIO.
>
> Without those two conditions, we couldn't share anything through
> tegra250.dtsi.
>
> Re-iteration of these implications below.
>
>>      [...]
>>      chipsel-gpio =<&gpio1>,
>>          <&gpio-controller1 13 0 55 77>,
>>          <0>, /* holes are permitted, means no GPIO 2 */
>>          <&gpio2 88>,
>>          <&gpio-controller2 5 1>;
>
>> A GPIO spec consist of:
>>
>> 1) A reference to either a gpio-controller or a gpio device node.
>>
>> 2) 0 or more address cells, according to the value of #address-cells in
>> the referenced node.  If the node has no #address-cells property, the
>> value is assumed to be 0.
>
> I'd expect address cells only if referencing a gpio-controller; if
> referencing a gpio device node, the address would be implicit.


Yes.  But I think it's better if there is a single rule for interpreting 
the GPIO spec, namely look at the #address-cells property, rather than 
deciding implicitly based on the type of the referent node.

>
>> 3) 0 or more mode cells, according to the value of #mode-cells in the
>> referenced node.
>
> Yes, I agree. Although, I think your (3) is inconsistent with the gpio
> controller definitions you wrote above, which include the mode
> definitions in the controller instead of the user.

Hmmm.  I think I got the example right.

Both of the gpio controller definitions have explicit "#address-cells" 
and "#mode-cells" properties, and neither has "mode".  Both references 
to controller nodes have mode values in the user spec.

gpio1 has "mode" but not "#mode-cells", and the reference to it has no 
mode value.

gpio2 has "#mode-cells=1" but not "mode", and the reference to it has 
one mode value.

Am I missing something?

>
>> In the example, the chipsel-gpio specs are interpreted as:
>>
>> <&gpio1>   -  refers to a pre-bound gpio device node, in which the
>> address (12 0) and mode (55 66) are specified within that node.
>
> Just re-iterating: I'd prefer mode to be solely in the reference, and
> not in the gpio controller.

I agree that it doesn't make much sense for a controller node to specify 
the mode.  My example doesn't do that.  The only mode value is in an 
individual gpio node, not a controller node.

 From the standpoint of a SoC manufacturer, specifying modes in the 
reference is probably a good idea.  But it's not necessarily best in all 
cases.  If the focus of attention is a board design with numerous 
variants and revisions, "binding" the modes of specific gpio pins 
according to the board wiring may be the right thing.

The way I specified it lets you choose.


>
> Does this SoC/board segregation make sense to everyone? Obviously it
> does to me:-)

It makes perfect sense from one viewpoint, but I think that board 
vendors may have a different focus.  The flexibility to specify a mode 
in either place costs little, as the parsing rule is definite and 
straightforward.  SoC vendors are free to defer mode decisions until 
later, by omitting "mode" and supplying "#mode-cells" in their device 
tree templates.  Board vendors could choose either to use the SoC 
vendor's template verbatim, or to amend it with additional 
board-specific information.

RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Tuesday, May 31, 2011 12:43 PM:
> On 5/31/2011 7:55 AM, Stephen Warren wrote:
> > Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
> >> ...
> >> GPIOs share the need to "point across the tree to different nodes", but
> >> it is unclear that there is a need for a entirely different hierarchy.
> >>
> >> That suggests the possibility of using the device tree addressing
> >> mechanism as much as possible.  Normal device tree addresses could be
> >> used to specify GPIO numbers.
> >>
> >> Suppose we have:
> >>
> >>      gpio-controller1: gpio-controller {
> >>          #address-cells =<2>;
> >>          #mode-cells =<2>;
> >>          gpio1: gpio@12,0 {
> >>              reg =<12 0>;
> >>              mode =<55 66>;
> >>              usage = "Audio Codec chip select";  /* Optional */
> >>          }
> >>      };
> >>      gpio-controller2: gpio-controller {
> >>          #address-cells =<1>;
> >>          #mode-cells =<1>;
> >>          gpio2: gpio@4 {
> >>              reg =<4>;
> >>              #mode-cells =<1>;
> >>          }
> >>      };
> >
> > A quick note on the way that Tegra's devicetree files are broken up in
> > Grant's devicetree/test branch:
> >
> > * There's "tegra250.dtsi" which defines everything on the Tegra SoC
> >    itself.
> > * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
> >    And additionally:
> >    ** Defines all devices on the board
> >    ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
> >       board.
> >
> > I like this model, because it shares the complete definition of the
> > Tegra SoC in a single file, rather than duplicating it with cut/paste
> > into every board file.
> >
> > As such, the code quoted above would be in tegra250.dtsi.
> >
> > This has two relevant implications here:
> >
> > 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> > naming of those GPIO pins, and not any board-specific naming, e.g.
> > "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> > be at the client/reference site, not the GPIO definition site.
> >
> > 2) The GPIO mode should be defined by the client/user of the GPIO, not
> > the SoC's definition of that GPIO.
> >
> > Without those two conditions, we couldn't share anything through
> > tegra250.dtsi.
> >
> > Re-iteration of these implications below.
> >
> >>      [...]
> >>      chipsel-gpio =<&gpio1>,
> >>          <&gpio-controller1 13 0 55 77>,
> >>          <0>, /* holes are permitted, means no GPIO 2 */
> >>          <&gpio2 88>,
> >>          <&gpio-controller2 5 1>;
> >
> >> A GPIO spec consist of:
> >>
> >> 1) A reference to either a gpio-controller or a gpio device node.
> >>
> >> 2) 0 or more address cells, according to the value of #address-cells in
> >> the referenced node.  If the node has no #address-cells property, the
> >> value is assumed to be 0.
> >
> > I'd expect address cells only if referencing a gpio-controller; if
> > referencing a gpio device node, the address would be implicit.
> 
> Yes.  But I think it's better if there is a single rule for interpreting
> the GPIO spec, namely look at the #address-cells property, rather than
> deciding implicitly based on the type of the referent node.
> 
> >> 3) 0 or more mode cells, according to the value of #mode-cells in the
> >> referenced node.
> >
> > Yes, I agree. Although, I think your (3) is inconsistent with the gpio
> > controller definitions you wrote above, which include the mode
> > definitions in the controller instead of the user.
> 
> Hmmm.  I think I got the example right.

You're right. The examples are consistent. I think what threw me was that
the example code itself contained "<&gpio2 88>" whereas the description
later referred to just "<gpio2>".

Also, I hadn't noticed that the gpio2 definition repeated
"#mode-cells = <1>;" whereas the gpio1 definition didn't.

I have to say, I don't like that aspect; i.e. having to repeat
#mode-cells in every gpio definition that's inside/underneath the
controller definition; in my mind, "passing on" the requirement to
define the mode would be the default state, so forcing the namer of
GPIOs (i.e. whoever writes the "gpio1: gpio@12,0 {" definitions) to
do this seems almost like busy work. Is there a way in *.dts to mark
the #mode-cells field as inherited by children unless overridden?

> >> In the example, the chipsel-gpio specs are interpreted as:
> >>
> >> <&gpio1>   -  refers to a pre-bound gpio device node, in which the
> >> address (12 0) and mode (55 66) are specified within that node.
> >
> > Just re-iterating: I'd prefer mode to be solely in the reference, and
> > not in the gpio controller.
> 
> I agree that it doesn't make much sense for a controller node to specify
> the mode.  My example doesn't do that.  The only mode value is in an
> individual gpio node, not a controller node.

Yes, I suppose that's true.

But, in my mind at least, the controller definition would be part of the
SoC definition, and provided by the SoC vendor in a separate and
basically immutable file. As such, any gpio node inside the controller
node really is part of the controller's/SoC's definition.

> From the standpoint of a SoC manufacturer, specifying modes in the
> reference is probably a good idea.  But it's not necessarily best in all
> cases.  If the focus of attention is a board design with numerous
> variants and revisions, "binding" the modes of specific gpio pins
> according to the board wiring may be the right thing.
> 
> The way I specified it lets you choose.

Granted.

However, I'm still not convinced that's a great idea; just because a
board vendor might want to cut/paste the entire SoC definition into their
DTS file and hack it, rather than just including it, doesn't mean it's
a good idea. If we agreed on that (which I'll grant we probably don't)
it seems like we shouldn't aim to add features that are probably only
needed to make the life of people who do that easier.

My perspective is that cut/pasting the entire SoC definition into a board
definition is the devicetree equivalent of having more than one driver
per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
stuff that caused Linus to complain about the state of ARM Linux.

Equally, a separation of SoC vs. board should make incorporating bugfixes
to the SoC definition into board definitions easier; simply replace the
file and recompile. And, it'll make it more obvious to board vendors which
changes need to be upstreamed to the SoC vendor, i.e. if a board vendor
finds a bug in the SoC definition file.

I suppose the one area this flexibility might make sense is if a board
vendor has N similar boards, they can setup a set of include files:

board-a.dts:
include board-common.dtsi
Do board-a customizations

board-b-dts:
include board-common.dtsi
Do board-b customizations

board-common.dtsi: include soc.dtsi
Could add the gpio definitions to the controller definition from
soc.dtsi

soc.dtsi:
base SoC definition; gpio controller, etc.

But I still don't entirely see the advantage of board-common.dtsi
defining GPIOs and having board-*.dts use those GPIOs as parameters to
HW modules, e.g. as a GPIO list for an SDHCI controller, rather than
simply having board-common.dtsi simply specify the SDHCI controller
directly, thus avoiding the new syntax.

> > Does this SoC/board segregation make sense to everyone? Obviously it
> > does to me:-)
> 
> It makes perfect sense from one viewpoint, but I think that board
> vendors may have a different focus.  The flexibility to specify a mode
> in either place costs little, as the parsing rule is definite and
> straightforward.  SoC vendors are free to defer mode decisions until
> later, by omitting "mode" and supplying "#mode-cells" in their device
> tree templates.  Board vendors could choose either to use the SoC
> vendor's template verbatim, or to amend it with additional
> board-specific information.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Tuesday, May 31, 2011 12:43 PM:
> On 5/31/2011 7:55 AM, Stephen Warren wrote:
> > Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
> >> ...
> >> GPIOs share the need to "point across the tree to different nodes", but
> >> it is unclear that there is a need for a entirely different hierarchy.
> >>
> >> That suggests the possibility of using the device tree addressing
> >> mechanism as much as possible.  Normal device tree addresses could be
> >> used to specify GPIO numbers.
> >>
> >> Suppose we have:
> >>
> >>      gpio-controller1: gpio-controller {
> >>          #address-cells =<2>;
> >>          #mode-cells =<2>;
> >>          gpio1: gpio at 12,0 {
> >>              reg =<12 0>;
> >>              mode =<55 66>;
> >>              usage = "Audio Codec chip select";  /* Optional */
> >>          }
> >>      };
> >>      gpio-controller2: gpio-controller {
> >>          #address-cells =<1>;
> >>          #mode-cells =<1>;
> >>          gpio2: gpio at 4 {
> >>              reg =<4>;
> >>              #mode-cells =<1>;
> >>          }
> >>      };
> >
> > A quick note on the way that Tegra's devicetree files are broken up in
> > Grant's devicetree/test branch:
> >
> > * There's "tegra250.dtsi" which defines everything on the Tegra SoC
> >    itself.
> > * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
> >    And additionally:
> >    ** Defines all devices on the board
> >    ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
> >       board.
> >
> > I like this model, because it shares the complete definition of the
> > Tegra SoC in a single file, rather than duplicating it with cut/paste
> > into every board file.
> >
> > As such, the code quoted above would be in tegra250.dtsi.
> >
> > This has two relevant implications here:
> >
> > 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> > naming of those GPIO pins, and not any board-specific naming, e.g.
> > "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> > be at the client/reference site, not the GPIO definition site.
> >
> > 2) The GPIO mode should be defined by the client/user of the GPIO, not
> > the SoC's definition of that GPIO.
> >
> > Without those two conditions, we couldn't share anything through
> > tegra250.dtsi.
> >
> > Re-iteration of these implications below.
> >
> >>      [...]
> >>      chipsel-gpio =<&gpio1>,
> >>          <&gpio-controller1 13 0 55 77>,
> >>          <0>, /* holes are permitted, means no GPIO 2 */
> >>          <&gpio2 88>,
> >>          <&gpio-controller2 5 1>;
> >
> >> A GPIO spec consist of:
> >>
> >> 1) A reference to either a gpio-controller or a gpio device node.
> >>
> >> 2) 0 or more address cells, according to the value of #address-cells in
> >> the referenced node.  If the node has no #address-cells property, the
> >> value is assumed to be 0.
> >
> > I'd expect address cells only if referencing a gpio-controller; if
> > referencing a gpio device node, the address would be implicit.
> 
> Yes.  But I think it's better if there is a single rule for interpreting
> the GPIO spec, namely look at the #address-cells property, rather than
> deciding implicitly based on the type of the referent node.
> 
> >> 3) 0 or more mode cells, according to the value of #mode-cells in the
> >> referenced node.
> >
> > Yes, I agree. Although, I think your (3) is inconsistent with the gpio
> > controller definitions you wrote above, which include the mode
> > definitions in the controller instead of the user.
> 
> Hmmm.  I think I got the example right.

You're right. The examples are consistent. I think what threw me was that
the example code itself contained "<&gpio2 88>" whereas the description
later referred to just "<gpio2>".

Also, I hadn't noticed that the gpio2 definition repeated
"#mode-cells = <1>;" whereas the gpio1 definition didn't.

I have to say, I don't like that aspect; i.e. having to repeat
#mode-cells in every gpio definition that's inside/underneath the
controller definition; in my mind, "passing on" the requirement to
define the mode would be the default state, so forcing the namer of
GPIOs (i.e. whoever writes the "gpio1: gpio at 12,0 {" definitions) to
do this seems almost like busy work. Is there a way in *.dts to mark
the #mode-cells field as inherited by children unless overridden?

> >> In the example, the chipsel-gpio specs are interpreted as:
> >>
> >> <&gpio1>   -  refers to a pre-bound gpio device node, in which the
> >> address (12 0) and mode (55 66) are specified within that node.
> >
> > Just re-iterating: I'd prefer mode to be solely in the reference, and
> > not in the gpio controller.
> 
> I agree that it doesn't make much sense for a controller node to specify
> the mode.  My example doesn't do that.  The only mode value is in an
> individual gpio node, not a controller node.

Yes, I suppose that's true.

But, in my mind at least, the controller definition would be part of the
SoC definition, and provided by the SoC vendor in a separate and
basically immutable file. As such, any gpio node inside the controller
node really is part of the controller's/SoC's definition.

> From the standpoint of a SoC manufacturer, specifying modes in the
> reference is probably a good idea.  But it's not necessarily best in all
> cases.  If the focus of attention is a board design with numerous
> variants and revisions, "binding" the modes of specific gpio pins
> according to the board wiring may be the right thing.
> 
> The way I specified it lets you choose.

Granted.

However, I'm still not convinced that's a great idea; just because a
board vendor might want to cut/paste the entire SoC definition into their
DTS file and hack it, rather than just including it, doesn't mean it's
a good idea. If we agreed on that (which I'll grant we probably don't)
it seems like we shouldn't aim to add features that are probably only
needed to make the life of people who do that easier.

My perspective is that cut/pasting the entire SoC definition into a board
definition is the devicetree equivalent of having more than one driver
per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
stuff that caused Linus to complain about the state of ARM Linux.

Equally, a separation of SoC vs. board should make incorporating bugfixes
to the SoC definition into board definitions easier; simply replace the
file and recompile. And, it'll make it more obvious to board vendors which
changes need to be upstreamed to the SoC vendor, i.e. if a board vendor
finds a bug in the SoC definition file.

I suppose the one area this flexibility might make sense is if a board
vendor has N similar boards, they can setup a set of include files:

board-a.dts:
include board-common.dtsi
Do board-a customizations

board-b-dts:
include board-common.dtsi
Do board-b customizations

board-common.dtsi: include soc.dtsi
Could add the gpio definitions to the controller definition from
soc.dtsi

soc.dtsi:
base SoC definition; gpio controller, etc.

But I still don't entirely see the advantage of board-common.dtsi
defining GPIOs and having board-*.dts use those GPIOs as parameters to
HW modules, e.g. as a GPIO list for an SDHCI controller, rather than
simply having board-common.dtsi simply specify the SDHCI controller
directly, thus avoiding the new syntax.

> > Does this SoC/board segregation make sense to everyone? Obviously it
> > does to me:-)
> 
> It makes perfect sense from one viewpoint, but I think that board
> vendors may have a different focus.  The flexibility to specify a mode
> in either place costs little, as the parsing rule is definite and
> straightforward.  SoC vendors are free to defer mode decisions until
> later, by omitting "mode" and supplying "#mode-cells" in their device
> tree templates.  Board vendors could choose either to use the SoC
> vendor's template verbatim, or to amend it with additional
> board-specific information.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Wed, Jun 01, 2011 at 08:59:55AM -0700, Stephen Warren wrote:

> My perspective is that cut/pasting the entire SoC definition into a board
> definition is the devicetree equivalent of having more than one driver
> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
> stuff that caused Linus to complain about the state of ARM Linux.

Very strongly agreed, we've had to do that in the past because of
limitations in the device tree infrastructure but if we end up having to
cut'n'paste that's not going to be great for maintainability.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Wed, Jun 01, 2011 at 08:59:55AM -0700, Stephen Warren wrote:

> My perspective is that cut/pasting the entire SoC definition into a board
> definition is the devicetree equivalent of having more than one driver
> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
> stuff that caused Linus to complain about the state of ARM Linux.

Very strongly agreed, we've had to do that in the past because of
limitations in the device tree infrastructure but if we end up having to
cut'n'paste that's not going to be great for maintainability.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Wed, Jun 1, 2011 at 10:18 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Jun 01, 2011 at 08:59:55AM -0700, Stephen Warren wrote:
>
>> My perspective is that cut/pasting the entire SoC definition into a board
>> definition is the devicetree equivalent of having more than one driver
>> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
>> stuff that caused Linus to complain about the state of ARM Linux.
>
> Very strongly agreed, we've had to do that in the past because of
> limitations in the device tree infrastructure but if we end up having to
> cut'n'paste that's not going to be great for maintainability.

+2

cut'n'paste is not an option.  period.  :-)

g.


>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Wed, Jun 1, 2011 at 10:18 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 01, 2011 at 08:59:55AM -0700, Stephen Warren wrote:
>
>> My perspective is that cut/pasting the entire SoC definition into a board
>> definition is the devicetree equivalent of having more than one driver
>> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
>> stuff that caused Linus to complain about the state of ARM Linux.
>
> Very strongly agreed, we've had to do that in the past because of
> limitations in the device tree infrastructure but if we end up having to
> cut'n'paste that's not going to be great for maintainability.

+2

cut'n'paste is not an option.  period.  :-)

g.


>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 6/1/2011 5:59 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Tuesday, May 31, 2011 12:43 PM:
>> On 5/31/2011 7:55 AM, Stephen Warren wrote:
>>> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>>>> ...
>>>> GPIOs share the need to "point across the tree to different nodes", but
>>>> it is unclear that there is a need for a entirely different hierarchy.
>>>>
>>>> That suggests the possibility of using the device tree addressing
>>>> mechanism as much as possible.  Normal device tree addresses could be
>>>> used to specify GPIO numbers.
>>>>
>>>> Suppose we have:
>>>>
>>>>       gpio-controller1: gpio-controller {
>>>>           #address-cells =<2>;
>>>>           #mode-cells =<2>;
>>>>           gpio1: gpio@12,0 {
>>>>               reg =<12 0>;
>>>>               mode =<55 66>;
>>>>               usage = "Audio Codec chip select";  /* Optional */
>>>>           }
>>>>       };
>>>>       gpio-controller2: gpio-controller {
>>>>           #address-cells =<1>;
>>>>           #mode-cells =<1>;
>>>>           gpio2: gpio@4 {
>>>>               reg =<4>;
>>>>               #mode-cells =<1>;
>>>>           }
>>>>       };
>>>
>>> A quick note on the way that Tegra's devicetree files are broken up in
>>> Grant's devicetree/test branch:
>>>
>>> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>>>     itself.
>>> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>>>     And additionally:
>>>     ** Defines all devices on the board
>>>     ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>>>        board.
>>>
>>> I like this model, because it shares the complete definition of the
>>> Tegra SoC in a single file, rather than duplicating it with cut/paste
>>> into every board file.
>>>
>>> As such, the code quoted above would be in tegra250.dtsi.
>>>
>>> This has two relevant implications here:
>>>
>>> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
>>> naming of those GPIO pins, and not any board-specific naming, e.g.
>>> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
>>> be at the client/reference site, not the GPIO definition site.
>>>
>>> 2) The GPIO mode should be defined by the client/user of the GPIO, not
>>> the SoC's definition of that GPIO.
>>>
>>> Without those two conditions, we couldn't share anything through
>>> tegra250.dtsi.
>>>
>>> Re-iteration of these implications below.
>>>
>>>>       [...]
>>>>       chipsel-gpio =<&gpio1>,
>>>>           <&gpio-controller1 13 0 55 77>,
>>>>           <0>, /* holes are permitted, means no GPIO 2 */
>>>>           <&gpio2 88>,
>>>>           <&gpio-controller2 5 1>;
>>>
>>>> A GPIO spec consist of:
>>>>
>>>> 1) A reference to either a gpio-controller or a gpio device node.
>>>>
>>>> 2) 0 or more address cells, according to the value of #address-cells in
>>>> the referenced node.  If the node has no #address-cells property, the
>>>> value is assumed to be 0.
>>>
>>> I'd expect address cells only if referencing a gpio-controller; if
>>> referencing a gpio device node, the address would be implicit.
>>
>> Yes.  But I think it's better if there is a single rule for interpreting
>> the GPIO spec, namely look at the #address-cells property, rather than
>> deciding implicitly based on the type of the referent node.
>>
>>>> 3) 0 or more mode cells, according to the value of #mode-cells in the
>>>> referenced node.
>>>
>>> Yes, I agree. Although, I think your (3) is inconsistent with the gpio
>>> controller definitions you wrote above, which include the mode
>>> definitions in the controller instead of the user.
>>
>> Hmmm.  I think I got the example right.
>
> You're right. The examples are consistent. I think what threw me was that
> the example code itself contained "<&gpio2 88>" whereas the description
> later referred to just "<gpio2>".
>
> Also, I hadn't noticed that the gpio2 definition repeated
> "#mode-cells =<1>;" whereas the gpio1 definition didn't.
>
> I have to say, I don't like that aspect; i.e. having to repeat
> #mode-cells in every gpio definition that's inside/underneath the
> controller definition; in my mind, "passing on" the requirement to
> define the mode would be the default state, so forcing the namer of
> GPIOs (i.e. whoever writes the "gpio1: gpio@12,0 {" definitions) to
> do this seems almost like busy work. Is there a way in *.dts to mark
> the #mode-cells field as inherited by children unless overridden?


That is a very good point.  Addressing it led me to a revised scheme 
that I like much better.  (Notice that in the notes below I address your 
reasonable desire for an immutable SoC core definition.)

The example:

     gpio-controller1: gpio-controller {
         #address-cells = <2>;
         #mode-cells = <2>;
         unbound_gpio1: gpio {
             /* No reg property */
             /* No mode property */
         }
         fully_bound_gpio1: audio-chipsel@12,0 {
             reg = <12 0>;
             mode = <55 66>;
             usage = "Audio Codec chip select";  /* Optional */
         }
         address_bound_gpio1: gpio@13,0 {
             reg = <13 0>;
             /* No mode property */
         }
         mode_bound_gpio1: open-drain-gpio {
             /* No reg property */
             mode = <77 88>;
         }
     };
     gpio-controller2: gpio-controller {
          #address-cells = <1>;
          #mode-cells = <1>;
          unbound_gpio2: gpio {
              /* No reg property */
              /* No mode property */
          }
          address_bound_gpio2: gpio@4 {
              reg = <4>;
              /* No mode property */
          }
     };
     [...]
     chipsel-gpio =
         <&fully_bound_gpio1>,
         <&unbound_gpio1 13 0 55 77>,
         <&mode_bound_gpio1 14 0>,
         <&address_bound_gpio2 88>,
         <&unbound_gpio2 5 1>;

Notes:

a) A reference to a GPIO always points to the child of a 
gpio-controller, i.e. to an individual gpio node.

b) If that gpio node has a "reg" property, the number of address cells 
in the reference is 0, otherwise it is #address-cells from the parent 
(gpio-controller) node.

c) If that gpio node has a "mode" property, the number of mode cells in 
the reference is 0, otherwise it is #mode-cells from the parent 
(gpio-controller) node.

d) It's unnecessary for all children of a gpio controller to be named 
just "gpio".  The important thing is that the semantics of the node - 
the set of properties (and, for Open Firmware systems, the set of 
methods) - meet the usage needs of the node's "client".

e) gpios that are mode-bound but not address-bound must have distinct
names from each other and from the unbound node name ("gpio"), because 
of the device tree rule that the combination of name+address must be 
unique among the children of a given node.  It is okay to have both 
"gpio" and "gpio@12", but you cannot have two nodes both named just "gpio".

f) SoC device tree blobs would probably use only the unbound form.  A 
given platform might choose to specialize the tree by adding additional 
bound nodes to the tree established by the unmodified SoC core blob.

g) reg-less nodes have been part of the Open Firmware spec forever; they 
are called "wildcard nodes".  Their intended use is to refer to a class 
of similar objects, potentially so numerous that full enumeration is 
undesireable.


>
>>>> In the example, the chipsel-gpio specs are interpreted as:
>>>>
>>>> <&gpio1>    -  refers to a pre-bound gpio device node, in which the
>>>> address (12 0) and mode (55 66) are specified within that node.
>>>
>>> Just re-iterating: I'd prefer mode to be solely in the reference, and
>>> not in the gpio controller.
>>
>> I agree that it doesn't make much sense for a controller node to specify
>> the mode.  My example doesn't do that.  The only mode value is in an
>> individual gpio node, not a controller node.
>
> Yes, I suppose that's true.
>
> But, in my mind at least, the controller definition would be part of the
> SoC definition, and provided by the SoC vendor in a separate and
> basically immutable file. As such, any gpio node inside the controller
> node really is part of the controller's/SoC's definition.
>
>>  From the standpoint of a SoC manufacturer, specifying modes in the
>> reference is probably a good idea.  But it's not necessarily best in all
>> cases.  If the focus of attention is a board design with numerous
>> variants and revisions, "binding" the modes of specific gpio pins
>> according to the board wiring may be the right thing.
>>
>> The way I specified it lets you choose.
>
> Granted.
>
> However, I'm still not convinced that's a great idea; just because a
> board vendor might want to cut/paste the entire SoC definition into their
> DTS file and hack it, rather than just including it, doesn't mean it's
> a good idea. If we agreed on that (which I'll grant we probably don't)
> it seems like we shouldn't aim to add features that are probably only
> needed to make the life of people who do that easier.
>
> My perspective is that cut/pasting the entire SoC definition into a board
> definition is the devicetree equivalent of having more than one driver
> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
> stuff that caused Linus to complain about the state of ARM Linux.
>
> Equally, a separation of SoC vs. board should make incorporating bugfixes
> to the SoC definition into board definitions easier; simply replace the
> file and recompile. And, it'll make it more obvious to board vendors which
> changes need to be upstreamed to the SoC vendor, i.e. if a board vendor
> finds a bug in the SoC definition file.
>
> I suppose the one area this flexibility might make sense is if a board
> vendor has N similar boards, they can setup a set of include files:
>
> board-a.dts:
> include board-common.dtsi
> Do board-a customizations
>
> board-b-dts:
> include board-common.dtsi
> Do board-b customizations
>
> board-common.dtsi: include soc.dtsi
> Could add the gpio definitions to the controller definition from
> soc.dtsi
>
> soc.dtsi:
> base SoC definition; gpio controller, etc.
>
> But I still don't entirely see the advantage of board-common.dtsi
> defining GPIOs and having board-*.dts use those GPIOs as parameters to
> HW modules, e.g. as a GPIO list for an SDHCI controller, rather than
> simply having board-common.dtsi simply specify the SDHCI controller
> directly, thus avoiding the new syntax.
>
>>> Does this SoC/board segregation make sense to everyone? Obviously it
>>> does to me:-)
>>
>> It makes perfect sense from one viewpoint, but I think that board
>> vendors may have a different focus.  The flexibility to specify a mode
>> in either place costs little, as the parsing rule is definite and
>> straightforward.  SoC vendors are free to defer mode decisions until
>> later, by omitting "mode" and supplying "#mode-cells" in their device
>> tree templates.  Board vendors could choose either to use the SoC
>> vendor's template verbatim, or to amend it with additional
>> board-specific information.
>
>

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 6/1/2011 5:59 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Tuesday, May 31, 2011 12:43 PM:
>> On 5/31/2011 7:55 AM, Stephen Warren wrote:
>>> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>>>> ...
>>>> GPIOs share the need to "point across the tree to different nodes", but
>>>> it is unclear that there is a need for a entirely different hierarchy.
>>>>
>>>> That suggests the possibility of using the device tree addressing
>>>> mechanism as much as possible.  Normal device tree addresses could be
>>>> used to specify GPIO numbers.
>>>>
>>>> Suppose we have:
>>>>
>>>>       gpio-controller1: gpio-controller {
>>>>           #address-cells =<2>;
>>>>           #mode-cells =<2>;
>>>>           gpio1: gpio at 12,0 {
>>>>               reg =<12 0>;
>>>>               mode =<55 66>;
>>>>               usage = "Audio Codec chip select";  /* Optional */
>>>>           }
>>>>       };
>>>>       gpio-controller2: gpio-controller {
>>>>           #address-cells =<1>;
>>>>           #mode-cells =<1>;
>>>>           gpio2: gpio at 4 {
>>>>               reg =<4>;
>>>>               #mode-cells =<1>;
>>>>           }
>>>>       };
>>>
>>> A quick note on the way that Tegra's devicetree files are broken up in
>>> Grant's devicetree/test branch:
>>>
>>> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>>>     itself.
>>> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>>>     And additionally:
>>>     ** Defines all devices on the board
>>>     ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>>>        board.
>>>
>>> I like this model, because it shares the complete definition of the
>>> Tegra SoC in a single file, rather than duplicating it with cut/paste
>>> into every board file.
>>>
>>> As such, the code quoted above would be in tegra250.dtsi.
>>>
>>> This has two relevant implications here:
>>>
>>> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
>>> naming of those GPIO pins, and not any board-specific naming, e.g.
>>> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
>>> be at the client/reference site, not the GPIO definition site.
>>>
>>> 2) The GPIO mode should be defined by the client/user of the GPIO, not
>>> the SoC's definition of that GPIO.
>>>
>>> Without those two conditions, we couldn't share anything through
>>> tegra250.dtsi.
>>>
>>> Re-iteration of these implications below.
>>>
>>>>       [...]
>>>>       chipsel-gpio =<&gpio1>,
>>>>           <&gpio-controller1 13 0 55 77>,
>>>>           <0>, /* holes are permitted, means no GPIO 2 */
>>>>           <&gpio2 88>,
>>>>           <&gpio-controller2 5 1>;
>>>
>>>> A GPIO spec consist of:
>>>>
>>>> 1) A reference to either a gpio-controller or a gpio device node.
>>>>
>>>> 2) 0 or more address cells, according to the value of #address-cells in
>>>> the referenced node.  If the node has no #address-cells property, the
>>>> value is assumed to be 0.
>>>
>>> I'd expect address cells only if referencing a gpio-controller; if
>>> referencing a gpio device node, the address would be implicit.
>>
>> Yes.  But I think it's better if there is a single rule for interpreting
>> the GPIO spec, namely look at the #address-cells property, rather than
>> deciding implicitly based on the type of the referent node.
>>
>>>> 3) 0 or more mode cells, according to the value of #mode-cells in the
>>>> referenced node.
>>>
>>> Yes, I agree. Although, I think your (3) is inconsistent with the gpio
>>> controller definitions you wrote above, which include the mode
>>> definitions in the controller instead of the user.
>>
>> Hmmm.  I think I got the example right.
>
> You're right. The examples are consistent. I think what threw me was that
> the example code itself contained "<&gpio2 88>" whereas the description
> later referred to just "<gpio2>".
>
> Also, I hadn't noticed that the gpio2 definition repeated
> "#mode-cells =<1>;" whereas the gpio1 definition didn't.
>
> I have to say, I don't like that aspect; i.e. having to repeat
> #mode-cells in every gpio definition that's inside/underneath the
> controller definition; in my mind, "passing on" the requirement to
> define the mode would be the default state, so forcing the namer of
> GPIOs (i.e. whoever writes the "gpio1: gpio at 12,0 {" definitions) to
> do this seems almost like busy work. Is there a way in *.dts to mark
> the #mode-cells field as inherited by children unless overridden?


That is a very good point.  Addressing it led me to a revised scheme 
that I like much better.  (Notice that in the notes below I address your 
reasonable desire for an immutable SoC core definition.)

The example:

     gpio-controller1: gpio-controller {
         #address-cells = <2>;
         #mode-cells = <2>;
         unbound_gpio1: gpio {
             /* No reg property */
             /* No mode property */
         }
         fully_bound_gpio1: audio-chipsel at 12,0 {
             reg = <12 0>;
             mode = <55 66>;
             usage = "Audio Codec chip select";  /* Optional */
         }
         address_bound_gpio1: gpio at 13,0 {
             reg = <13 0>;
             /* No mode property */
         }
         mode_bound_gpio1: open-drain-gpio {
             /* No reg property */
             mode = <77 88>;
         }
     };
     gpio-controller2: gpio-controller {
          #address-cells = <1>;
          #mode-cells = <1>;
          unbound_gpio2: gpio {
              /* No reg property */
              /* No mode property */
          }
          address_bound_gpio2: gpio at 4 {
              reg = <4>;
              /* No mode property */
          }
     };
     [...]
     chipsel-gpio =
         <&fully_bound_gpio1>,
         <&unbound_gpio1 13 0 55 77>,
         <&mode_bound_gpio1 14 0>,
         <&address_bound_gpio2 88>,
         <&unbound_gpio2 5 1>;

Notes:

a) A reference to a GPIO always points to the child of a 
gpio-controller, i.e. to an individual gpio node.

b) If that gpio node has a "reg" property, the number of address cells 
in the reference is 0, otherwise it is #address-cells from the parent 
(gpio-controller) node.

c) If that gpio node has a "mode" property, the number of mode cells in 
the reference is 0, otherwise it is #mode-cells from the parent 
(gpio-controller) node.

d) It's unnecessary for all children of a gpio controller to be named 
just "gpio".  The important thing is that the semantics of the node - 
the set of properties (and, for Open Firmware systems, the set of 
methods) - meet the usage needs of the node's "client".

e) gpios that are mode-bound but not address-bound must have distinct
names from each other and from the unbound node name ("gpio"), because 
of the device tree rule that the combination of name+address must be 
unique among the children of a given node.  It is okay to have both 
"gpio" and "gpio at 12", but you cannot have two nodes both named just "gpio".

f) SoC device tree blobs would probably use only the unbound form.  A 
given platform might choose to specialize the tree by adding additional 
bound nodes to the tree established by the unmodified SoC core blob.

g) reg-less nodes have been part of the Open Firmware spec forever; they 
are called "wildcard nodes".  Their intended use is to refer to a class 
of similar objects, potentially so numerous that full enumeration is 
undesireable.


>
>>>> In the example, the chipsel-gpio specs are interpreted as:
>>>>
>>>> <&gpio1>    -  refers to a pre-bound gpio device node, in which the
>>>> address (12 0) and mode (55 66) are specified within that node.
>>>
>>> Just re-iterating: I'd prefer mode to be solely in the reference, and
>>> not in the gpio controller.
>>
>> I agree that it doesn't make much sense for a controller node to specify
>> the mode.  My example doesn't do that.  The only mode value is in an
>> individual gpio node, not a controller node.
>
> Yes, I suppose that's true.
>
> But, in my mind at least, the controller definition would be part of the
> SoC definition, and provided by the SoC vendor in a separate and
> basically immutable file. As such, any gpio node inside the controller
> node really is part of the controller's/SoC's definition.
>
>>  From the standpoint of a SoC manufacturer, specifying modes in the
>> reference is probably a good idea.  But it's not necessarily best in all
>> cases.  If the focus of attention is a board design with numerous
>> variants and revisions, "binding" the modes of specific gpio pins
>> according to the board wiring may be the right thing.
>>
>> The way I specified it lets you choose.
>
> Granted.
>
> However, I'm still not convinced that's a great idea; just because a
> board vendor might want to cut/paste the entire SoC definition into their
> DTS file and hack it, rather than just including it, doesn't mean it's
> a good idea. If we agreed on that (which I'll grant we probably don't)
> it seems like we shouldn't aim to add features that are probably only
> needed to make the life of people who do that easier.
>
> My perspective is that cut/pasting the entire SoC definition into a board
> definition is the devicetree equivalent of having more than one driver
> per chip, not sharing gpio/pinmux/... code, etc. in the kernel; the very
> stuff that caused Linus to complain about the state of ARM Linux.
>
> Equally, a separation of SoC vs. board should make incorporating bugfixes
> to the SoC definition into board definitions easier; simply replace the
> file and recompile. And, it'll make it more obvious to board vendors which
> changes need to be upstreamed to the SoC vendor, i.e. if a board vendor
> finds a bug in the SoC definition file.
>
> I suppose the one area this flexibility might make sense is if a board
> vendor has N similar boards, they can setup a set of include files:
>
> board-a.dts:
> include board-common.dtsi
> Do board-a customizations
>
> board-b-dts:
> include board-common.dtsi
> Do board-b customizations
>
> board-common.dtsi: include soc.dtsi
> Could add the gpio definitions to the controller definition from
> soc.dtsi
>
> soc.dtsi:
> base SoC definition; gpio controller, etc.
>
> But I still don't entirely see the advantage of board-common.dtsi
> defining GPIOs and having board-*.dts use those GPIOs as parameters to
> HW modules, e.g. as a GPIO list for an SDHCI controller, rather than
> simply having board-common.dtsi simply specify the SDHCI controller
> directly, thus avoiding the new syntax.
>
>>> Does this SoC/board segregation make sense to everyone? Obviously it
>>> does to me:-)
>>
>> It makes perfect sense from one viewpoint, but I think that board
>> vendors may have a different focus.  The flexibility to specify a mode
>> in either place costs little, as the parsing rule is definite and
>> straightforward.  SoC vendors are free to defer mode decisions until
>> later, by omitting "mode" and supplying "#mode-cells" in their device
>> tree templates.  Board vendors could choose either to use the SoC
>> vendor's template verbatim, or to amend it with additional
>> board-specific information.
>
>

RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Wednesday, June 01, 2011 3:33 PM:
> On 6/1/2011 5:59 AM, Stephen Warren wrote:
> > ...
> > I have to say, I don't like that aspect; i.e. having to repeat
> > #mode-cells in every gpio definition that's inside/underneath the
> > controller definition; in my mind, "passing on" the requirement to
> > define the mode would be the default state, so forcing the namer of
> > GPIOs (i.e. whoever writes the "gpio1: gpio@12,0 {" definitions) to
> > do this seems almost like busy work. Is there a way in *.dts to mark
> > the #mode-cells field as inherited by children unless overridden?
> 
> That is a very good point.  Addressing it led me to a revised scheme
> that I like much better.  (Notice that in the notes below I address your
> reasonable desire for an immutable SoC core definition.)
> 
> The example:
> 
>      gpio-controller1: gpio-controller {
>          #address-cells = <2>;
>          #mode-cells = <2>;
>          unbound_gpio1: gpio {
>              /* No reg property */
>              /* No mode property */
>          }
>          fully_bound_gpio1: audio-chipsel@12,0 {
>              reg = <12 0>;
>              mode = <55 66>;
>              usage = "Audio Codec chip select";  /* Optional */
>          }
>          address_bound_gpio1: gpio@13,0 {
>              reg = <13 0>;
>              /* No mode property */
>          }
>          mode_bound_gpio1: open-drain-gpio {
>              /* No reg property */
>              mode = <77 88>;
>          }
>      };
>      gpio-controller2: gpio-controller {
>           #address-cells = <1>;
>           #mode-cells = <1>;
>           unbound_gpio2: gpio {
>               /* No reg property */
>               /* No mode property */
>           }
>           address_bound_gpio2: gpio@4 {
>               reg = <4>;
>               /* No mode property */
>           }
>      };
>      [...]
>      chipsel-gpio =
>          <&fully_bound_gpio1>,
>          <&unbound_gpio1 13 0 55 77>,
>          <&mode_bound_gpio1 14 0>,
>          <&address_bound_gpio2 88>,
>          <&unbound_gpio2 5 1>;

Sorry for the slow response. Some brief comments:

a) I like this better than the previous proposal; the handling of 
   #address-cells and #mode-cells is more consistent here.

b) I like that the GPIO controller (or some overlay file on top of it)
   can provide names for GPIOs and names for modes.

c) I suspect that something like the following won't work:

   chipsel-gpio =
       <&address_bound_gpio2 &mode_bound_gpio2>;

   It's an attempt to symbolically reference both a GPIO name/address
   and mode. I feel this kind of reference would be the most common
   case; a device wanting to use symbolic names for a particular GPIO
   location and mode. Only being able to specify name/address *or* mode
   symbolically, but not both, seems like a rather serious hole, and
   makes the scheme a lot less interesting to me.

d) Doing this all with DT node references seems complex and overkill. I'd
   personally far rather see the device-tree compiler grow something like
   the C pre-processor, so you could just do:

   gpioc: gpio-controller {
       #address-cells = <2>;
       #mode-cells = <2>;
   };
   #define TEGRA_GPIO_PA1 0 0
   #define TEGRA_GPIO_PX2 23 1
   #define TERGA_GPIO_PULLUP 0
   #define TERGA_GPIO_PULLDOWN 1
   #define TEGRA_GPIO_FAST 0
   #define TEGRA_GPIO_SLOW 1

   And then reference them like:

   chipsel-gpio =
       <&gpioc TEGRA_GPIO_PX2 TEGRA_GPIO_PULLUP TEGRA_GPIO_FAST>;

   Related to this, I'm having difficulty understanding why editing a
   GPIO reference in the style immediately above is any harder than
   editing a GPIO node within the GPIO controller of the style you most
   recently proposed.

-- 
nvpublic

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Stephen Warren]

Mitch Bradley wrote at Wednesday, June 01, 2011 3:33 PM:
> On 6/1/2011 5:59 AM, Stephen Warren wrote:
> > ...
> > I have to say, I don't like that aspect; i.e. having to repeat
> > #mode-cells in every gpio definition that's inside/underneath the
> > controller definition; in my mind, "passing on" the requirement to
> > define the mode would be the default state, so forcing the namer of
> > GPIOs (i.e. whoever writes the "gpio1: gpio at 12,0 {" definitions) to
> > do this seems almost like busy work. Is there a way in *.dts to mark
> > the #mode-cells field as inherited by children unless overridden?
> 
> That is a very good point.  Addressing it led me to a revised scheme
> that I like much better.  (Notice that in the notes below I address your
> reasonable desire for an immutable SoC core definition.)
> 
> The example:
> 
>      gpio-controller1: gpio-controller {
>          #address-cells = <2>;
>          #mode-cells = <2>;
>          unbound_gpio1: gpio {
>              /* No reg property */
>              /* No mode property */
>          }
>          fully_bound_gpio1: audio-chipsel at 12,0 {
>              reg = <12 0>;
>              mode = <55 66>;
>              usage = "Audio Codec chip select";  /* Optional */
>          }
>          address_bound_gpio1: gpio at 13,0 {
>              reg = <13 0>;
>              /* No mode property */
>          }
>          mode_bound_gpio1: open-drain-gpio {
>              /* No reg property */
>              mode = <77 88>;
>          }
>      };
>      gpio-controller2: gpio-controller {
>           #address-cells = <1>;
>           #mode-cells = <1>;
>           unbound_gpio2: gpio {
>               /* No reg property */
>               /* No mode property */
>           }
>           address_bound_gpio2: gpio at 4 {
>               reg = <4>;
>               /* No mode property */
>           }
>      };
>      [...]
>      chipsel-gpio =
>          <&fully_bound_gpio1>,
>          <&unbound_gpio1 13 0 55 77>,
>          <&mode_bound_gpio1 14 0>,
>          <&address_bound_gpio2 88>,
>          <&unbound_gpio2 5 1>;

Sorry for the slow response. Some brief comments:

a) I like this better than the previous proposal; the handling of 
   #address-cells and #mode-cells is more consistent here.

b) I like that the GPIO controller (or some overlay file on top of it)
   can provide names for GPIOs and names for modes.

c) I suspect that something like the following won't work:

   chipsel-gpio =
       <&address_bound_gpio2 &mode_bound_gpio2>;

   It's an attempt to symbolically reference both a GPIO name/address
   and mode. I feel this kind of reference would be the most common
   case; a device wanting to use symbolic names for a particular GPIO
   location and mode. Only being able to specify name/address *or* mode
   symbolically, but not both, seems like a rather serious hole, and
   makes the scheme a lot less interesting to me.

d) Doing this all with DT node references seems complex and overkill. I'd
   personally far rather see the device-tree compiler grow something like
   the C pre-processor, so you could just do:

   gpioc: gpio-controller {
       #address-cells = <2>;
       #mode-cells = <2>;
   };
   #define TEGRA_GPIO_PA1 0 0
   #define TEGRA_GPIO_PX2 23 1
   #define TERGA_GPIO_PULLUP 0
   #define TERGA_GPIO_PULLDOWN 1
   #define TEGRA_GPIO_FAST 0
   #define TEGRA_GPIO_SLOW 1

   And then reference them like:

   chipsel-gpio =
       <&gpioc TEGRA_GPIO_PX2 TEGRA_GPIO_PULLUP TEGRA_GPIO_FAST>;

   Related to this, I'm having difficulty understanding why editing a
   GPIO reference in the style immediately above is any harder than
   editing a GPIO node within the GPIO controller of the style you most
   recently proposed.

-- 
nvpublic

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 6/3/2011 11:24 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Wednesday, June 01, 2011 3:33 PM:
>> On 6/1/2011 5:59 AM, Stephen Warren wrote:
>>> ...
>>> I have to say, I don't like that aspect; i.e. having to repeat
>>> #mode-cells in every gpio definition that's inside/underneath the
>>> controller definition; in my mind, "passing on" the requirement to
>>> define the mode would be the default state, so forcing the namer of
>>> GPIOs (i.e. whoever writes the "gpio1: gpio@12,0 {" definitions) to
>>> do this seems almost like busy work. Is there a way in *.dts to mark
>>> the #mode-cells field as inherited by children unless overridden?
>>
>> That is a very good point.  Addressing it led me to a revised scheme
>> that I like much better.  (Notice that in the notes below I address your
>> reasonable desire for an immutable SoC core definition.)
>>
>> The example:
>>
>>       gpio-controller1: gpio-controller {
>>           #address-cells =<2>;
>>           #mode-cells =<2>;
>>           unbound_gpio1: gpio {
>>               /* No reg property */
>>               /* No mode property */
>>           }
>>           fully_bound_gpio1: audio-chipsel@12,0 {
>>               reg =<12 0>;
>>               mode =<55 66>;
>>               usage = "Audio Codec chip select";  /* Optional */
>>           }
>>           address_bound_gpio1: gpio@13,0 {
>>               reg =<13 0>;
>>               /* No mode property */
>>           }
>>           mode_bound_gpio1: open-drain-gpio {
>>               /* No reg property */
>>               mode =<77 88>;
>>           }
>>       };
>>       gpio-controller2: gpio-controller {
>>            #address-cells =<1>;
>>            #mode-cells =<1>;
>>            unbound_gpio2: gpio {
>>                /* No reg property */
>>                /* No mode property */
>>            }
>>            address_bound_gpio2: gpio@4 {
>>                reg =<4>;
>>                /* No mode property */
>>            }
>>       };
>>       [...]
>>       chipsel-gpio =
>>           <&fully_bound_gpio1>,
>>           <&unbound_gpio1 13 0 55 77>,
>>           <&mode_bound_gpio1 14 0>,
>>           <&address_bound_gpio2 88>,
>>           <&unbound_gpio2 5 1>;
>
> Sorry for the slow response. Some brief comments:
>
> a) I like this better than the previous proposal; the handling of
>     #address-cells and #mode-cells is more consistent here.
>
> b) I like that the GPIO controller (or some overlay file on top of it)
>     can provide names for GPIOs and names for modes.
>
> c) I suspect that something like the following won't work:
>
>     chipsel-gpio =
>         <&address_bound_gpio2&mode_bound_gpio2>;
>
>     It's an attempt to symbolically reference both a GPIO name/address
>     and mode. I feel this kind of reference would be the most common
>     case; a device wanting to use symbolic names for a particular GPIO
>     location and mode. Only being able to specify name/address *or* mode
>     symbolically, but not both, seems like a rather serious hole, and
>     makes the scheme a lot less interesting to me.
>
> d) Doing this all with DT node references seems complex and overkill. I'd
>     personally far rather see the device-tree compiler grow something like
>     the C pre-processor, so you could just do:
>
>     gpioc: gpio-controller {
>         #address-cells =<2>;
>         #mode-cells =<2>;
>     };
>     #define TEGRA_GPIO_PA1 0 0
>     #define TEGRA_GPIO_PX2 23 1
>     #define TERGA_GPIO_PULLUP 0
>     #define TERGA_GPIO_PULLDOWN 1
>     #define TEGRA_GPIO_FAST 0
>     #define TEGRA_GPIO_SLOW 1
>
>     And then reference them like:
>
>     chipsel-gpio =
>         <&gpioc TEGRA_GPIO_PX2 TEGRA_GPIO_PULLUP TEGRA_GPIO_FAST>;
>
>     Related to this, I'm having difficulty understanding why editing a
>     GPIO reference in the style immediately above is any harder than
>     editing a GPIO node within the GPIO controller of the style you most
>     recently proposed.
>

It seems that your focus is on the syntax of the device tree compiler. 
That's fine, but my focus is rather different - I'm concerned about the 
semantics of the "object model" represented by the tree layout.  I don't 
use the device tree compiler.  I do full-on Open Firmware systems, in 
which device node are active objects with methods.

My intention in suggesting this scheme was primarily to promote the idea 
that gpios "underneath" a gpio-controller could be addressed using the 
device tree's normal physical addressing rules.  It seems to me that a 
GPIO identification number is an "address" according to the device tree 
rules, thus the portion of the "gpio cells" list that identifies the 
specific pin should be thought of as a bona-file address, instead of 
something new and different.

That thought led me to consider what individual GPIO nodes would look 
like, and that led me to the thought that it might be useful to "bind" 
some of them, so clients could refer to the "bound package" without 
having to know the details.  This seemed good from an information-hiding 
perspective.  I liked the idea that you could change the board and fix 
the reassignment in one place, instead of having to track down all the 
usage points.

I sympathize with your desire for a human-readable DTC syntax that is 
not error-prone.  I think that's somewhat orthogonal to the semantic 
issues of the device tree structure.  Both are important.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mitch Bradley]

On 6/3/2011 11:24 AM, Stephen Warren wrote:
> Mitch Bradley wrote at Wednesday, June 01, 2011 3:33 PM:
>> On 6/1/2011 5:59 AM, Stephen Warren wrote:
>>> ...
>>> I have to say, I don't like that aspect; i.e. having to repeat
>>> #mode-cells in every gpio definition that's inside/underneath the
>>> controller definition; in my mind, "passing on" the requirement to
>>> define the mode would be the default state, so forcing the namer of
>>> GPIOs (i.e. whoever writes the "gpio1: gpio at 12,0 {" definitions) to
>>> do this seems almost like busy work. Is there a way in *.dts to mark
>>> the #mode-cells field as inherited by children unless overridden?
>>
>> That is a very good point.  Addressing it led me to a revised scheme
>> that I like much better.  (Notice that in the notes below I address your
>> reasonable desire for an immutable SoC core definition.)
>>
>> The example:
>>
>>       gpio-controller1: gpio-controller {
>>           #address-cells =<2>;
>>           #mode-cells =<2>;
>>           unbound_gpio1: gpio {
>>               /* No reg property */
>>               /* No mode property */
>>           }
>>           fully_bound_gpio1: audio-chipsel at 12,0 {
>>               reg =<12 0>;
>>               mode =<55 66>;
>>               usage = "Audio Codec chip select";  /* Optional */
>>           }
>>           address_bound_gpio1: gpio at 13,0 {
>>               reg =<13 0>;
>>               /* No mode property */
>>           }
>>           mode_bound_gpio1: open-drain-gpio {
>>               /* No reg property */
>>               mode =<77 88>;
>>           }
>>       };
>>       gpio-controller2: gpio-controller {
>>            #address-cells =<1>;
>>            #mode-cells =<1>;
>>            unbound_gpio2: gpio {
>>                /* No reg property */
>>                /* No mode property */
>>            }
>>            address_bound_gpio2: gpio at 4 {
>>                reg =<4>;
>>                /* No mode property */
>>            }
>>       };
>>       [...]
>>       chipsel-gpio =
>>           <&fully_bound_gpio1>,
>>           <&unbound_gpio1 13 0 55 77>,
>>           <&mode_bound_gpio1 14 0>,
>>           <&address_bound_gpio2 88>,
>>           <&unbound_gpio2 5 1>;
>
> Sorry for the slow response. Some brief comments:
>
> a) I like this better than the previous proposal; the handling of
>     #address-cells and #mode-cells is more consistent here.
>
> b) I like that the GPIO controller (or some overlay file on top of it)
>     can provide names for GPIOs and names for modes.
>
> c) I suspect that something like the following won't work:
>
>     chipsel-gpio =
>         <&address_bound_gpio2&mode_bound_gpio2>;
>
>     It's an attempt to symbolically reference both a GPIO name/address
>     and mode. I feel this kind of reference would be the most common
>     case; a device wanting to use symbolic names for a particular GPIO
>     location and mode. Only being able to specify name/address *or* mode
>     symbolically, but not both, seems like a rather serious hole, and
>     makes the scheme a lot less interesting to me.
>
> d) Doing this all with DT node references seems complex and overkill. I'd
>     personally far rather see the device-tree compiler grow something like
>     the C pre-processor, so you could just do:
>
>     gpioc: gpio-controller {
>         #address-cells =<2>;
>         #mode-cells =<2>;
>     };
>     #define TEGRA_GPIO_PA1 0 0
>     #define TEGRA_GPIO_PX2 23 1
>     #define TERGA_GPIO_PULLUP 0
>     #define TERGA_GPIO_PULLDOWN 1
>     #define TEGRA_GPIO_FAST 0
>     #define TEGRA_GPIO_SLOW 1
>
>     And then reference them like:
>
>     chipsel-gpio =
>         <&gpioc TEGRA_GPIO_PX2 TEGRA_GPIO_PULLUP TEGRA_GPIO_FAST>;
>
>     Related to this, I'm having difficulty understanding why editing a
>     GPIO reference in the style immediately above is any harder than
>     editing a GPIO node within the GPIO controller of the style you most
>     recently proposed.
>

It seems that your focus is on the syntax of the device tree compiler. 
That's fine, but my focus is rather different - I'm concerned about the 
semantics of the "object model" represented by the tree layout.  I don't 
use the device tree compiler.  I do full-on Open Firmware systems, in 
which device node are active objects with methods.

My intention in suggesting this scheme was primarily to promote the idea 
that gpios "underneath" a gpio-controller could be addressed using the 
device tree's normal physical addressing rules.  It seems to me that a 
GPIO identification number is an "address" according to the device tree 
rules, thus the portion of the "gpio cells" list that identifies the 
specific pin should be thought of as a bona-file address, instead of 
something new and different.

That thought led me to consider what individual GPIO nodes would look 
like, and that led me to the thought that it might be useful to "bind" 
some of them, so clients could refer to the "bound package" without 
having to know the details.  This seemed good from an information-hiding 
perspective.  I liked the idea that you could change the board and fix 
the reassignment in one place, instead of having to track down all the 
usage points.

I sympathize with your desire for a human-readable DTC syntax that is 
not error-prone.  I think that's somewhat orthogonal to the semantic 
issues of the device tree structure.  Both are important.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Tue, May 31, 2011 at 11:55 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>> ...
>> GPIOs share the need to "point across the tree to different nodes", but
>> it is unclear that there is a need for a entirely different hierarchy.
>>
>> That suggests the possibility of using the device tree addressing
>> mechanism as much as possible.  Normal device tree addresses could be
>> used to specify GPIO numbers.
>>
>> Suppose we have:
>>
>>     gpio-controller1: gpio-controller {
>>         #address-cells = <2>;
>>         #mode-cells = <2>;
>>         gpio1: gpio@12,0 {
>>             reg = <12 0>;
>>             mode = <55 66>;
>>             usage = "Audio Codec chip select";  /* Optional */
>>         }
>>     };
>>     gpio-controller2: gpio-controller {
>>         #address-cells = <1>;
>>         #mode-cells = <1>;
>>         gpio2: gpio@4 {
>>             reg = <4>;
>>             #mode-cells = <1>;
>>         }
>>     };
>
> A quick note on the way that Tegra's devicetree files are broken up in
> Grant's devicetree/test branch:
>
> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
>  itself.
> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
>  And additionally:
>  ** Defines all devices on the board
>  ** Hence, defines the usage of e.g. all the Tegra GPIOs for the
>     board.
>
> I like this model, because it shares the complete definition of the
> Tegra SoC in a single file, rather than duplicating it with cut/paste
> into every board file.
>
> As such, the code quoted above would be in tegra250.dtsi.
>
> This has two relevant implications here:
>
> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> naming of those GPIO pins, and not any board-specific naming, e.g.
> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> be at the client/reference site, not the GPIO definition site.
>
> 2) The GPIO mode should be defined by the client/user of the GPIO, not
> the SoC's definition of that GPIO.
>
> Without those two conditions, we couldn't share anything through
> tegra250.dtsi.

It's worth noting that the board file can override anything in
tegra250.dtsi, even in the SoC nodes, so if needed properties can be
added or modified in the SoC's description of the gpio controller.

>
> Just re-iterating: I'd prefer mode to be solely in the reference, and
> not in the gpio controller.
>
> Does this SoC/board segregation make sense to everyone? Obviously it
> does to me:-)

It certainly does to me.  :-)

g.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Tue, May 31, 2011 at 11:55 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Mitch Bradley wrote at Monday, May 30, 2011 2:53 PM:
>> ...
>> GPIOs share the need to "point across the tree to different nodes", but
>> it is unclear that there is a need for a entirely different hierarchy.
>>
>> That suggests the possibility of using the device tree addressing
>> mechanism as much as possible. ?Normal device tree addresses could be
>> used to specify GPIO numbers.
>>
>> Suppose we have:
>>
>> ? ? gpio-controller1: gpio-controller {
>> ? ? ? ? #address-cells = <2>;
>> ? ? ? ? #mode-cells = <2>;
>> ? ? ? ? gpio1: gpio at 12,0 {
>> ? ? ? ? ? ? reg = <12 0>;
>> ? ? ? ? ? ? mode = <55 66>;
>> ? ? ? ? ? ? usage = "Audio Codec chip select"; ?/* Optional */
>> ? ? ? ? }
>> ? ? };
>> ? ? gpio-controller2: gpio-controller {
>> ? ? ? ? #address-cells = <1>;
>> ? ? ? ? #mode-cells = <1>;
>> ? ? ? ? gpio2: gpio at 4 {
>> ? ? ? ? ? ? reg = <4>;
>> ? ? ? ? ? ? #mode-cells = <1>;
>> ? ? ? ? }
>> ? ? };
>
> A quick note on the way that Tegra's devicetree files are broken up in
> Grant's devicetree/test branch:
>
> * There's "tegra250.dtsi" which defines everything on the Tegra SoC
> ?itself.
> * There's a per-board e.g. harmony.dts which includes tegra250.dtsi,
> ?And additionally:
> ?** Defines all devices on the board
> ?** Hence, defines the usage of e.g. all the Tegra GPIOs for the
> ? ? board.
>
> I like this model, because it shares the complete definition of the
> Tegra SoC in a single file, rather than duplicating it with cut/paste
> into every board file.
>
> As such, the code quoted above would be in tegra250.dtsi.
>
> This has two relevant implications here:
>
> 1) The names "gpio1" and "gpio2" would be driven by the Tegra SoC's
> naming of those GPIO pins, and not any board-specific naming, e.g.
> "tegra_gpio_px1", "tegra_gpio_pb5". Equally, the usage comment would
> be at the client/reference site, not the GPIO definition site.
>
> 2) The GPIO mode should be defined by the client/user of the GPIO, not
> the SoC's definition of that GPIO.
>
> Without those two conditions, we couldn't share anything through
> tegra250.dtsi.

It's worth noting that the board file can override anything in
tegra250.dtsi, even in the SoC nodes, so if needed properties can be
added or modified in the SoC's description of the gpio controller.

>
> Just re-iterating: I'd prefer mode to be solely in the reference, and
> not in the gpio controller.
>
> Does this SoC/board segregation make sense to everyone? Obviously it
> does to me:-)

It certainly does to me.  :-)

g.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 2:53 PM, Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:
> Perhaps the interrupt-mapping binding is not the best model.  Interrupt
> hardware in general is hierarchical but is not isomorphic to the physical
> addressing hierarchy of the device tree.
>
> GPIOs share the need to "point across the tree to different nodes", but it
> is unclear that there is a need for a entirely different hierarchy.
>
> That suggests the possibility of using the device tree addressing mechanism
> as much as possible.  Normal device tree addresses could be used to specify
> GPIO numbers.
>
> Suppose we have:
>
>        gpio-controller1: gpio-controller {
>                #address-cells = <2>;
>                #mode-cells = <2>;
>                gpio1: gpio@12,0 {
>                    reg = <12 0>;
>                    mode = <55 66>;
>                    usage = "Audio Codec chip select";  /* Optional */
>                }
>        };
>        gpio-controller2: gpio-controller {
>                 #address-cells = <1>;
>                 #mode-cells = <1>;
>                 gpio2: gpio@4 {
>                     reg = <4>;
>                     #mode-cells = <1>;
>                 }
>        };
>        [...]
>        chipsel-gpio =  <&gpio1>,
>                        <&gpio-controller1 13 0 55 77>,
>                        <0>, /* holes are permitted, means no GPIO 2 */
>                        <&gpio2 88>,
>                        <&gpio-controller2 5 1>;
>
>
> A GPIO spec consist of:
>
> 1) A reference to either a gpio-controller or a gpio device node.
>
> 2) 0 or more address cells, according to the value of #address-cells in the
> referenced node.  If the node has no #address-cells property, the value is
> assumed to be 0.
>
> 3) 0 or more mode cells, according to the value of #mode-cells in the
> referenced node.

I can see having nodes for individual gpios being useful in
circumstances, but I really don't like having multiple methods of
specifying a gpio (handle to the gpio-controller, or a handle to the
gpio node, and a different specifier depending on the contents of the
target node).  I think it will be less confusing for users if the
reference is always to the gpio controller.  A specific gpio
controller can still use child nodes to capture extra information
about specific gpios, but doing so doesn't need to be exposed to a
gpio consumer; it can all be internal to the gpio controller and its
hardware specific binding (since any mode details are going to be
hardware specific anyway most likely).

[Amending to the above which was written before you latest post: even
with the refined proposal to link to only a child node, the gpio
specifier still changes depending on the contents of the child node]

If a gpio controller does use child nodes, then I do like the approach
of using #{address,size}-cells to line up with gpio numbering.
However, we've already got a definition of #gpio-cells in use which
specifies the total number of cells for a '*-gpio' property binding,
so I do want to take care not to conflict with the existing pattern.
I suspect the solution would simply be to state that #gpio-cells >=
#address-cells must be true.

> In the example, the chipsel-gpio specs are interpreted as:
>
> <&gpio1>  -  refers to a pre-bound gpio device node, in which the address
> (12 0) and mode (55 66) are specified within that node.
>
> <&gpio-controller1 13 0 55 77>  -  refers to a GPIO controller node,
> specifing the address (13 0) and the mode (55 77) in the client's GPIO spec.
>
> <gpio2>  -  another reference to a gpio node on a different controller.  In
> this case the address (4) is bound in the gpio node, but the mode (88) is
> passed in from the client's GPIO spec.
>
> <&gpio-controller2 5 1>  -  another reference to a controller node, with a
> one-cell address (5) and a one-cell mode (1), according to the values of
> #address-cells and #mode-cells in that controller node.
>
> I expect that the "pre-bound gpio node" case would get a lot of use in
> practice, as it lets you isolate the client from the details of the
> interrupt controller addressing and modes.  In my experience, GPIOs often
> get reassigned between revisions of the same board, especially early in the
> development cycle.

I'm not convinced that the pre-bound gpio node will be any better or
worse from a usability standpoint that direct references.  I've
certainly not had problems with keeping up with gpio moves (and
everything else moving) on the FPGA projects that I've worked with.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 2:53 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> Perhaps the interrupt-mapping binding is not the best model. ?Interrupt
> hardware in general is hierarchical but is not isomorphic to the physical
> addressing hierarchy of the device tree.
>
> GPIOs share the need to "point across the tree to different nodes", but it
> is unclear that there is a need for a entirely different hierarchy.
>
> That suggests the possibility of using the device tree addressing mechanism
> as much as possible. ?Normal device tree addresses could be used to specify
> GPIO numbers.
>
> Suppose we have:
>
> ? ? ? ?gpio-controller1: gpio-controller {
> ? ? ? ? ? ? ? ?#address-cells = <2>;
> ? ? ? ? ? ? ? ?#mode-cells = <2>;
> ? ? ? ? ? ? ? ?gpio1: gpio at 12,0 {
> ? ? ? ? ? ? ? ? ? ?reg = <12 0>;
> ? ? ? ? ? ? ? ? ? ?mode = <55 66>;
> ? ? ? ? ? ? ? ? ? ?usage = "Audio Codec chip select"; ?/* Optional */
> ? ? ? ? ? ? ? ?}
> ? ? ? ?};
> ? ? ? ?gpio-controller2: gpio-controller {
> ? ? ? ? ? ? ? ? #address-cells = <1>;
> ? ? ? ? ? ? ? ? #mode-cells = <1>;
> ? ? ? ? ? ? ? ? gpio2: gpio at 4 {
> ? ? ? ? ? ? ? ? ? ? reg = <4>;
> ? ? ? ? ? ? ? ? ? ? #mode-cells = <1>;
> ? ? ? ? ? ? ? ? }
> ? ? ? ?};
> ? ? ? ?[...]
> ? ? ? ?chipsel-gpio = ?<&gpio1>,
> ? ? ? ? ? ? ? ? ? ? ? ?<&gpio-controller1 13 0 55 77>,
> ? ? ? ? ? ? ? ? ? ? ? ?<0>, /* holes are permitted, means no GPIO 2 */
> ? ? ? ? ? ? ? ? ? ? ? ?<&gpio2 88>,
> ? ? ? ? ? ? ? ? ? ? ? ?<&gpio-controller2 5 1>;
>
>
> A GPIO spec consist of:
>
> 1) A reference to either a gpio-controller or a gpio device node.
>
> 2) 0 or more address cells, according to the value of #address-cells in the
> referenced node. ?If the node has no #address-cells property, the value is
> assumed to be 0.
>
> 3) 0 or more mode cells, according to the value of #mode-cells in the
> referenced node.

I can see having nodes for individual gpios being useful in
circumstances, but I really don't like having multiple methods of
specifying a gpio (handle to the gpio-controller, or a handle to the
gpio node, and a different specifier depending on the contents of the
target node).  I think it will be less confusing for users if the
reference is always to the gpio controller.  A specific gpio
controller can still use child nodes to capture extra information
about specific gpios, but doing so doesn't need to be exposed to a
gpio consumer; it can all be internal to the gpio controller and its
hardware specific binding (since any mode details are going to be
hardware specific anyway most likely).

[Amending to the above which was written before you latest post: even
with the refined proposal to link to only a child node, the gpio
specifier still changes depending on the contents of the child node]

If a gpio controller does use child nodes, then I do like the approach
of using #{address,size}-cells to line up with gpio numbering.
However, we've already got a definition of #gpio-cells in use which
specifies the total number of cells for a '*-gpio' property binding,
so I do want to take care not to conflict with the existing pattern.
I suspect the solution would simply be to state that #gpio-cells >=
#address-cells must be true.

> In the example, the chipsel-gpio specs are interpreted as:
>
> <&gpio1> ?- ?refers to a pre-bound gpio device node, in which the address
> (12 0) and mode (55 66) are specified within that node.
>
> <&gpio-controller1 13 0 55 77> ?- ?refers to a GPIO controller node,
> specifing the address (13 0) and the mode (55 77) in the client's GPIO spec.
>
> <gpio2> ?- ?another reference to a gpio node on a different controller. ?In
> this case the address (4) is bound in the gpio node, but the mode (88) is
> passed in from the client's GPIO spec.
>
> <&gpio-controller2 5 1> ?- ?another reference to a controller node, with a
> one-cell address (5) and a one-cell mode (1), according to the values of
> #address-cells and #mode-cells in that controller node.
>
> I expect that the "pre-bound gpio node" case would get a lot of use in
> practice, as it lets you isolate the client from the details of the
> interrupt controller addressing and modes. ?In my experience, GPIOs often
> get reassigned between revisions of the same board, especially early in the
> development cycle.

I'm not convinced that the pre-bound gpio node will be any better or
worse from a usability standpoint that direct references.  I've
certainly not had problems with keeping up with gpio moves (and
everything else moving) on the FPGA projects that I've worked with.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 1:20 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, May 30, 2011 at 12:54 PM, Segher Boessenkool
> <segher-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
>>>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>>>> Whatever we choose, I think it should be able to handle an insane
>>>> number of GPIOs without getting any more cumbersome that is
>>>> necessary.
>>>
>>> This is *consumer* side GPIOs, not bindings for the device providing the
>>> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
>>> many of them will be block functions so you'd have a binding with an
>>> array for things like "databus" and "addrbus".
>>
>> But please name them like "databus-gpio", so that it is obvious what it
>> is.  Also have to think about how this will work with multiple GPIO
>> controllers: do you require the GPIO controller node to be part of every
>> GPIO description, or do you do some "gpio-parent" scheme as well, how
>> does that interact with not having a single array of GPIOs?
>>
>> Better write this down as a binding, before committing to it :-)
>
> Here's my thinking: Exactly the same binding for "gpios" as is now,
> except can use different property names, like "chipsel-gpios" or
> "wp-gpio".  Here is a draft patch to the binding:

I know there is still some debate on this, but I'm going to go ahead
and commit this extension since it draws naturally from what we're
already using, and it does not preclude doing a node binding at some
point in the future.

g.

>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index edaa84d..21dd4f9 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -4,17 +4,45 @@ Specifying GPIO information for devices
>  1) gpios property
>  -----------------
>
> -Nodes that makes use of GPIOs should define them using `gpios' property,
> -format of which is: <&gpio-controller1-phandle gpio1-specifier
> -                    &gpio-controller2-phandle gpio2-specifier
> -                    0 /* holes are permitted, means no GPIO 3 */
> -                    &gpio-controller4-phandle gpio4-specifier
> -                    ...>;
> +Nodes that makes use of GPIOs should specify them using one or more
> +properties, each containing a 'gpio-list':
>
> -Note that gpio-specifier length is controller dependent.
> +       gpio-list ::= <single-gpio> [gpio-list]
> +       single-gpio ::= <gpio-phandle> <gpio-specifier>
> +       gpio-phandle : phandle to gpio controller node
> +       gpio-specifier : Array of #gpio-cells specifying specific gpio
> +                        (controller specific)
> +
> +GPIO properties should be named either "gpios" or "*-gpio".  Exact
> +meaning of each gpio property must be documented in the device tree
> +binding for each device.
> +
> +For example, the following could be used to describe gpios pins to use
> +as chip select lines; with chip selects 0, 1 and 3 populated, and chip
> +select 2 left empty:
> +
> +       gpio1: gpio1 {
> +               gpio-controller
> +                #gpio-cells = <2>;
> +       };
> +       gpio2: gpio2 {
> +               gpio-controller
> +                #gpio-cells = <1>;
> +       };
> +       [...]
> +        chipsel-gpio = <&gpio1 12 0>,
> +                       <&gpio1 13 0>,
> +                       <0>, /* holes are permitted, means no GPIO 2 */
> +                       <&gpio2 2>;
> +
> +Note that gpio-specifier length is controller dependent.  In the
> +above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
> +only uses one.
>
>  gpio-specifier may encode: bank, pin position inside the bank,
>  whether pin is open-drain and whether pin is logically inverted.
> +Exact meaning of each specifier cell is controller specific, and must
> +be documented in the device tree binding for the device.
>
>  Example of the node using GPIOs:
>
> @@ -28,8 +56,8 @@ and empty GPIO flags as accepted by the "qe_pio_e"
> gpio-controller.
>  2) gpio-controller nodes
>  ------------------------
>
> -Every GPIO controller node must have #gpio-cells property defined,
> -this information will be used to translate gpio-specifiers.
> +Every GPIO controller node must both an empty "gpio-controller"
> +property, and have #gpio-cells contain the size of the gpio-specifier.
>
>  Example of two SOC GPIO banks defined as gpio-controller nodes:
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Grant Likely]

On Mon, May 30, 2011 at 1:20 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, May 30, 2011 at 12:54 PM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>>>> I'm currently dealing with an SoC that has over a hundred GPIOs.
>>>> Whatever we choose, I think it should be able to handle an insane
>>>> number of GPIOs without getting any more cumbersome that is
>>>> necessary.
>>>
>>> This is *consumer* side GPIOs, not bindings for the device providing the
>>> GPIOs. ?If a single device needs to use hundreds of GPIOs I'd expect
>>> many of them will be block functions so you'd have a binding with an
>>> array for things like "databus" and "addrbus".
>>
>> But please name them like "databus-gpio", so that it is obvious what it
>> is. ?Also have to think about how this will work with multiple GPIO
>> controllers: do you require the GPIO controller node to be part of every
>> GPIO description, or do you do some "gpio-parent" scheme as well, how
>> does that interact with not having a single array of GPIOs?
>>
>> Better write this down as a binding, before committing to it :-)
>
> Here's my thinking: Exactly the same binding for "gpios" as is now,
> except can use different property names, like "chipsel-gpios" or
> "wp-gpio". ?Here is a draft patch to the binding:

I know there is still some debate on this, but I'm going to go ahead
and commit this extension since it draws naturally from what we're
already using, and it does not preclude doing a node binding at some
point in the future.

g.

>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
> b/Documentation/devicetree/bindings/gpio/gpio.txt
> index edaa84d..21dd4f9 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -4,17 +4,45 @@ Specifying GPIO information for devices
> ?1) gpios property
> ?-----------------
>
> -Nodes that makes use of GPIOs should define them using `gpios' property,
> -format of which is: <&gpio-controller1-phandle gpio1-specifier
> - ? ? ? ? ? ? ? ? ? ?&gpio-controller2-phandle gpio2-specifier
> - ? ? ? ? ? ? ? ? ? ?0 /* holes are permitted, means no GPIO 3 */
> - ? ? ? ? ? ? ? ? ? ?&gpio-controller4-phandle gpio4-specifier
> - ? ? ? ? ? ? ? ? ? ?...>;
> +Nodes that makes use of GPIOs should specify them using one or more
> +properties, each containing a 'gpio-list':
>
> -Note that gpio-specifier length is controller dependent.
> + ? ? ? gpio-list ::= <single-gpio> [gpio-list]
> + ? ? ? single-gpio ::= <gpio-phandle> <gpio-specifier>
> + ? ? ? gpio-phandle : phandle to gpio controller node
> + ? ? ? gpio-specifier : Array of #gpio-cells specifying specific gpio
> + ? ? ? ? ? ? ? ? ? ? ? ?(controller specific)
> +
> +GPIO properties should be named either "gpios" or "*-gpio". ?Exact
> +meaning of each gpio property must be documented in the device tree
> +binding for each device.
> +
> +For example, the following could be used to describe gpios pins to use
> +as chip select lines; with chip selects 0, 1 and 3 populated, and chip
> +select 2 left empty:
> +
> + ? ? ? gpio1: gpio1 {
> + ? ? ? ? ? ? ? gpio-controller
> + ? ? ? ? ? ? ? ?#gpio-cells = <2>;
> + ? ? ? };
> + ? ? ? gpio2: gpio2 {
> + ? ? ? ? ? ? ? gpio-controller
> + ? ? ? ? ? ? ? ?#gpio-cells = <1>;
> + ? ? ? };
> + ? ? ? [...]
> + ? ? ? ?chipsel-gpio = <&gpio1 12 0>,
> + ? ? ? ? ? ? ? ? ? ? ? <&gpio1 13 0>,
> + ? ? ? ? ? ? ? ? ? ? ? <0>, /* holes are permitted, means no GPIO 2 */
> + ? ? ? ? ? ? ? ? ? ? ? <&gpio2 2>;
> +
> +Note that gpio-specifier length is controller dependent. ?In the
> +above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
> +only uses one.
>
> ?gpio-specifier may encode: bank, pin position inside the bank,
> ?whether pin is open-drain and whether pin is logically inverted.
> +Exact meaning of each specifier cell is controller specific, and must
> +be documented in the device tree binding for the device.
>
> ?Example of the node using GPIOs:
>
> @@ -28,8 +56,8 @@ and empty GPIO flags as accepted by the "qe_pio_e"
> gpio-controller.
> ?2) gpio-controller nodes
> ?------------------------
>
> -Every GPIO controller node must have #gpio-cells property defined,
> -this information will be used to translate gpio-specifiers.
> +Every GPIO controller node must both an empty "gpio-controller"
> +property, and have #gpio-cells contain the size of the gpio-specifier.
>
> ?Example of two SOC GPIO banks defined as gpio-controller nodes:
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Benjamin Herrenschmidt]

On Mon, 2011-05-30 at 15:01 +0800, Mark Brown wrote:
> On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
> 
> > I'm currently dealing with an SoC that has over a hundred GPIOs.
> > Whatever we choose, I think it should be able to handle an insane
> > number of GPIOs without getting any more cumbersome that is
> > necessary.
> 
> This is *consumer* side GPIOs, not bindings for the device providing the
> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

Yes. Agreed. The producer remains identified by phandle/gpio#, so it's
just a naming thing on the consumer side.

So what are the options ?

 - gpio-xxxx = < ... > properties

 - existing binding, along with an optional gpio-names string list

 - anything else ?

Cheers,
Ben.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Benjamin Herrenschmidt]

On Mon, 2011-05-30 at 15:01 +0800, Mark Brown wrote:
> On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
> 
> > I'm currently dealing with an SoC that has over a hundred GPIOs.
> > Whatever we choose, I think it should be able to handle an insane
> > number of GPIOs without getting any more cumbersome that is
> > necessary.
> 
> This is *consumer* side GPIOs, not bindings for the device providing the
> GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> many of them will be block functions so you'd have a binding with an
> array for things like "databus" and "addrbus".

Yes. Agreed. The producer remains identified by phandle/gpio#, so it's
just a naming thing on the consumer side.

So what are the options ?

 - gpio-xxxx = < ... > properties

 - existing binding, along with an optional gpio-names string list

 - anything else ?

Cheers,
Ben.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Olof Johansson]

On Tue, May 31, 2011 at 09:27:12AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2011-05-30 at 15:01 +0800, Mark Brown wrote:
> > On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
> > 
> > > I'm currently dealing with an SoC that has over a hundred GPIOs.
> > > Whatever we choose, I think it should be able to handle an insane
> > > number of GPIOs without getting any more cumbersome that is
> > > necessary.
> > 
> > This is *consumer* side GPIOs, not bindings for the device providing the
> > GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> > many of them will be block functions so you'd have a binding with an
> > array for things like "databus" and "addrbus".
> 
> Yes. Agreed. The producer remains identified by phandle/gpio#, so it's
> just a naming thing on the consumer side.
> 
> So what are the options ?
> 
>  - gpio-xxxx = < ... > properties
> 
>  - existing binding, along with an optional gpio-names string list
> 
>  - anything else ?

The producer side works fine as-is, agreed.

What I was not sure about was the use of having an array of unnamed
gpios as part of the consumer-side binding, where there's no logical
ordering between these entries.

In the sdhci case, there are three gpios; one to supply power to the slot;
one for card detect and one for write protect sense.

In that case, it would make a whole lot more sense to have three separate
properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque array of
entries without description besides what comments are used in the dts file.

That these in turn point just to gpio number <x> at controller <y> is OK with
me. Also, I can see cases where it makes sense to have more than one gpio
references in a property (i.e. busses), but only where there's either internal
ordering to them, or where ordering doesn't matter at all.


-Olof

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Olof Johansson]

On Tue, May 31, 2011 at 09:27:12AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2011-05-30 at 15:01 +0800, Mark Brown wrote:
> > On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote:
> > 
> > > I'm currently dealing with an SoC that has over a hundred GPIOs.
> > > Whatever we choose, I think it should be able to handle an insane
> > > number of GPIOs without getting any more cumbersome that is
> > > necessary.
> > 
> > This is *consumer* side GPIOs, not bindings for the device providing the
> > GPIOs.  If a single device needs to use hundreds of GPIOs I'd expect
> > many of them will be block functions so you'd have a binding with an
> > array for things like "databus" and "addrbus".
> 
> Yes. Agreed. The producer remains identified by phandle/gpio#, so it's
> just a naming thing on the consumer side.
> 
> So what are the options ?
> 
>  - gpio-xxxx = < ... > properties
> 
>  - existing binding, along with an optional gpio-names string list
> 
>  - anything else ?

The producer side works fine as-is, agreed.

What I was not sure about was the use of having an array of unnamed
gpios as part of the consumer-side binding, where there's no logical
ordering between these entries.

In the sdhci case, there are three gpios; one to supply power to the slot;
one for card detect and one for write protect sense.

In that case, it would make a whole lot more sense to have three separate
properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque array of
entries without description besides what comments are used in the dts file.

That these in turn point just to gpio number <x> at controller <y> is OK with
me. Also, I can see cases where it makes sense to have more than one gpio
references in a property (i.e. busses), but only where there's either internal
ordering to them, or where ordering doesn't matter at all.


-Olof

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Segher Boessenkool]

> The producer side works fine as-is, agreed.

+1

> What I was not sure about was the use of having an array of unnamed
> gpios as part of the consumer-side binding, where there's no logical
> ordering between these entries.
>
> In the sdhci case, there are three gpios; one to supply power to the 
> slot;
> one for card detect and one for write protect sense.
>
> In that case, it would make a whole lot more sense to have three 
> separate
> properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque 
> array of
> entries without description besides what comments are used in the dts 
> file.

Yes.  And this way you can also show a specific board doesn't have a
specific function GPIO wired up by simply not having the property for
it, instead of that ugly 0 phandle business.

> That these in turn point just to gpio number <x> at controller <y> is 
> OK with
> me.

Controller <y> GPIO # <x> with flags <w>, yes.  A generic "GPIO 
specifier".

>  Also, I can see cases where it makes sense to have more than one gpio
> references in a property (i.e. busses), but only where there's either 
> internal
> ordering to them, or where ordering doesn't matter at all.

That's up to the device binding, let's hope the people who write those
show good judgement :-)


Segher

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Segher Boessenkool]

> The producer side works fine as-is, agreed.

+1

> What I was not sure about was the use of having an array of unnamed
> gpios as part of the consumer-side binding, where there's no logical
> ordering between these entries.
>
> In the sdhci case, there are three gpios; one to supply power to the 
> slot;
> one for card detect and one for write protect sense.
>
> In that case, it would make a whole lot more sense to have three 
> separate
> properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque 
> array of
> entries without description besides what comments are used in the dts 
> file.

Yes.  And this way you can also show a specific board doesn't have a
specific function GPIO wired up by simply not having the property for
it, instead of that ugly 0 phandle business.

> That these in turn point just to gpio number <x> at controller <y> is 
> OK with
> me.

Controller <y> GPIO # <x> with flags <w>, yes.  A generic "GPIO 
specifier".

>  Also, I can see cases where it makes sense to have more than one gpio
> references in a property (i.e. busses), but only where there's either 
> internal
> ordering to them, or where ordering doesn't matter at all.

That's up to the device binding, let's hope the people who write those
show good judgement :-)


Segher

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Mon, May 30, 2011 at 04:49:09PM -0700, Olof Johansson wrote:

> What I was not sure about was the use of having an array of unnamed
> gpios as part of the consumer-side binding, where there's no logical
> ordering between these entries.

> In the sdhci case, there are three gpios; one to supply power to the slot;
> one for card detect and one for write protect sense.

> In that case, it would make a whole lot more sense to have three separate
> properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque array of
> entries without description besides what comments are used in the dts file.

> That these in turn point just to gpio number <x> at controller <y> is OK with
> me. Also, I can see cases where it makes sense to have more than one gpio
> references in a property (i.e. busses), but only where there's either internal
> ordering to them, or where ordering doesn't matter at all.

I agree strongly with this - there's a very good reason why this is the
existing pattern for platform data.  There's other cases that are even
worse than SDHCI where you can get a large number of potential outputs
from a device that could be wired up, a vanishingly small number
(possibly even none) of which will actually be used in a given system.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Mon, May 30, 2011 at 04:49:09PM -0700, Olof Johansson wrote:

> What I was not sure about was the use of having an array of unnamed
> gpios as part of the consumer-side binding, where there's no logical
> ordering between these entries.

> In the sdhci case, there are three gpios; one to supply power to the slot;
> one for card detect and one for write protect sense.

> In that case, it would make a whole lot more sense to have three separate
> properties, say "power-gpio", "cd-gpio" and "wp-gpio", than an opaque array of
> entries without description besides what comments are used in the dts file.

> That these in turn point just to gpio number <x> at controller <y> is OK with
> me. Also, I can see cases where it makes sense to have more than one gpio
> references in a property (i.e. busses), but only where there's either internal
> ordering to them, or where ordering doesn't matter at all.

I agree strongly with this - there's a very good reason why this is the
existing pattern for platform data.  There's other cases that are even
worse than SDHCI where you can get a large number of potential outputs
from a device that could be wired up, a vanishingly small number
(possibly even none) of which will actually be used in a given system.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Mon, May 30, 2011 at 12:11:55AM -0600, Grant Likely wrote:
> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:

> > Interesting...  what was the reasoning behind this?  It's a definite
> > step backwards but it does explain my major concern with the new batch
> > of device tree patches.

> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.

GPIOs are rather different to IRQs in that one tends to have rather more
of them, the typical case is that you only have a very small number of
IRQs (usually only one).

> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).

This should definitely be specified, yes.

> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

I just can't see how it's sustainable to use an array here - for devices
with many possible functions that could be connected to a GPIO on the
CPU, most of which won't be in use on any given system (a typical part
that I work with might have 20 or so GPIO functions but only be able to
use a fraction of those at once), a flat array with magic indexes is
just going to be error prone and illegible.  Speaking as someone who'll
have to support users using affected drivers I'm entirely unenthusiastic
about the idea.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Mon, May 30, 2011 at 12:11:55AM -0600, Grant Likely wrote:
> On Mon, May 30, 2011 at 11:38:27AM +0800, Mark Brown wrote:

> > Interesting...  what was the reasoning behind this?  It's a definite
> > step backwards but it does explain my major concern with the new batch
> > of device tree patches.

> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.

GPIOs are rather different to IRQs in that one tends to have rather more
of them, the typical case is that you only have a very small number of
IRQs (usually only one).

> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).

This should definitely be specified, yes.

> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

I just can't see how it's sustainable to use an array here - for devices
with many possible functions that could be connected to a GPIO on the
CPU, most of which won't be in use on any given system (a typical part
that I work with might have 20 or so GPIO functions but only be able to
use a fraction of those at once), a flat array with magic indexes is
just going to be error prone and illegible.  Speaking as someone who'll
have to support users using affected drivers I'm entirely unenthusiastic
about the idea.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Benjamin Herrenschmidt]

On Mon, 2011-05-30 at 00:11 -0600, Grant Likely wrote:

> > Interesting...  what was the reasoning behind this?  It's a definite
> > step backwards but it does explain my major concern with the new batch
> > of device tree patches.
>
> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.
> 
> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).
> 
> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

So first, I wasn't involved in the definition of the GPIO binding much
if at all :-)

Second, I can see advantages in both the numbered and the named
approaches, it's not totally clear cut and it's definitely not a
"definite step backward" which explains "major concerns" since, Mark,
you just discovered it ;-) You -do- write like you just found the excuse
for your general hostility :-)

Now more seriously, referencing GPIOs as a list of phandle/number in an
array has the advantage of being compact. It works well for many setups,
and GPIOs in practice are -often- referenced by number within a GPIO
block. I think it's probably the right approach to "target" a given GPIO
-provider-.

The question is how do you relate the entries in that array with
basically wires on the "client" chip. This is the same problem faced by
the clock binding btw, tho there's usually a bit less clocks than GPIOs
(but still more than interrupts :-)

The approach apple uses is interesting, where they essentially use
device-specific properties that contain a GPIO binding. For example, a
reset-gpio property, that points to the GPIO doing the reset, etc...

We -could- do just that. For 'busses' such a property can easily contain
more than one entry.

Or we could keep a gpio "map" and have a parallel property to name the
entries like I was originally thinking for the clocks.

In any case, it's something that can reasonably easily be added on top
of the existing binding, and it would be much more productive Mark if
you actually proposed solutions rather than just opposing things :-) 

Cheers,
Ben.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Benjamin Herrenschmidt]

On Mon, 2011-05-30 at 00:11 -0600, Grant Likely wrote:

> > Interesting...  what was the reasoning behind this?  It's a definite
> > step backwards but it does explain my major concern with the new batch
> > of device tree patches.
>
> The binding for gpios was defined a few years ago and it is in fairly
> wide use within the powerpc sphere.  The design followed the pattern
> established for specifying irqs, and in that regard satisfied the
> principle of least surprise.
> 
> That said, it isn't a very large leap to go from a single 'gpios'
> property to allowing multiple named gpios properties with meaningful
> names, particularly if they are fully specified by the device
> binding, and they follow exactly the same binding semantics as the
> existing 'gpios' proprety (phandle + gpio specifier).
> 
> Personally, I'm /cautious/ about saying okay to extending the binding,
> simply because once the extension is in use it is really hard to go
> back on it, but I cannot think of any reason why this particular case
> wouldn't be a good idea.  Anyone have thoughts on this?  Ben?  Mitch?

So first, I wasn't involved in the definition of the GPIO binding much
if at all :-)

Second, I can see advantages in both the numbered and the named
approaches, it's not totally clear cut and it's definitely not a
"definite step backward" which explains "major concerns" since, Mark,
you just discovered it ;-) You -do- write like you just found the excuse
for your general hostility :-)

Now more seriously, referencing GPIOs as a list of phandle/number in an
array has the advantage of being compact. It works well for many setups,
and GPIOs in practice are -often- referenced by number within a GPIO
block. I think it's probably the right approach to "target" a given GPIO
-provider-.

The question is how do you relate the entries in that array with
basically wires on the "client" chip. This is the same problem faced by
the clock binding btw, tho there's usually a bit less clocks than GPIOs
(but still more than interrupts :-)

The approach apple uses is interesting, where they essentially use
device-specific properties that contain a GPIO binding. For example, a
reset-gpio property, that points to the GPIO doing the reset, etc...

We -could- do just that. For 'busses' such a property can easily contain
more than one entry.

Or we could keep a gpio "map" and have a parallel property to name the
entries like I was originally thinking for the clocks.

In any case, it's something that can reasonably easily be added on top
of the existing binding, and it would be much more productive Mark if
you actually proposed solutions rather than just opposing things :-) 

Cheers,
Ben.

Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Tue, May 31, 2011 at 09:26:00AM +1000, Benjamin Herrenschmidt wrote:

> The approach apple uses is interesting, where they essentially use
> device-specific properties that contain a GPIO binding. For example, a
> reset-gpio property, that points to the GPIO doing the reset, etc...

This is exactly what I'm suggesting; it seems like the immediately
obvious approach to take and it's certainly exactly what happens with
the existing platform data code.

> Or we could keep a gpio "map" and have a parallel property to name the
> entries like I was originally thinking for the clocks.

Honestly this would never even have occurred to me; how would that look
and what would the indirection buy us (perhaps seeing how it looks would
make that obvious)?

> In any case, it's something that can reasonably easily be added on top
> of the existing binding, and it would be much more productive Mark if
> you actually proposed solutions rather than just opposing things :-) 

I'm really sorry you feel this way.  All I'm doing here is looking at
the existing platform data, looking at the device tree that's being
proposed and saying "why can't we have something that's as usable as
the platform data?" - I'd have thought that the suggestion was fairly
obvious there.  Just looking at the code it's completely unclear why
we'd even be doing the approach in the original patch in the first
place, Olaf's post was the first I knew about there being some existing
convention here.

One of the problems with the device tree style stuff to people who
haven't used it and can't actually work on it (in my case I have no
systems which can actually boot with a device tree) is that there's this
whole bunch of rules that are already in place that we don't know about
which people just refer us to - witness Olaf's response further up the
thread, he said he queried a similar issue with a SD driver and was
apparently just told that this is the way the device tree does things.

I would anticipate that especially in the early stages of rollout where
device tree is only available on a very limited selection of platforms
you're going to get a bunch of such feedback from maintainers can and
should review the code but are doing so purely based on looking at the
code they're seeing.  I've actually seen a bit of device tree stuff
before but a lot of people won't have done so - if there's stuff people
are querying you're going to have to explain things, even if they're
well established device tree conventions.

This is going to be one of the biggest challenges with rolling out
device tree, both from the point of view of maintainers learning about
how the device tree stuff should look and from the point of view of
maintainers learning who to trust on device tree.  There's a few people
like you and Grant that I know are really familiar with this stuff but
there's also going to be a whole raft of new people submitting device
tree support and if something looks off maintainers are going to have to
work out if it's due to some peculiar device tree idiom or if it's just
an issue in the patch.

[RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.

[Mark Brown]

On Tue, May 31, 2011 at 09:26:00AM +1000, Benjamin Herrenschmidt wrote:

> The approach apple uses is interesting, where they essentially use
> device-specific properties that contain a GPIO binding. For example, a
> reset-gpio property, that points to the GPIO doing the reset, etc...

This is exactly what I'm suggesting; it seems like the immediately
obvious approach to take and it's certainly exactly what happens with
the existing platform data code.

> Or we could keep a gpio "map" and have a parallel property to name the
> entries like I was originally thinking for the clocks.

Honestly this would never even have occurred to me; how would that look
and what would the indirection buy us (perhaps seeing how it looks would
make that obvious)?

> In any case, it's something that can reasonably easily be added on top
> of the existing binding, and it would be much more productive Mark if
> you actually proposed solutions rather than just opposing things :-) 

I'm really sorry you feel this way.  All I'm doing here is looking at
the existing platform data, looking at the device tree that's being
proposed and saying "why can't we have something that's as usable as
the platform data?" - I'd have thought that the suggestion was fairly
obvious there.  Just looking at the code it's completely unclear why
we'd even be doing the approach in the original patch in the first
place, Olaf's post was the first I knew about there being some existing
convention here.

One of the problems with the device tree style stuff to people who
haven't used it and can't actually work on it (in my case I have no
systems which can actually boot with a device tree) is that there's this
whole bunch of rules that are already in place that we don't know about
which people just refer us to - witness Olaf's response further up the
thread, he said he queried a similar issue with a SD driver and was
apparently just told that this is the way the device tree does things.

I would anticipate that especially in the early stages of rollout where
device tree is only available on a very limited selection of platforms
you're going to get a bunch of such feedback from maintainers can and
should review the code but are doing so purely based on looking at the
code they're seeing.  I've actually seen a bit of device tree stuff
before but a lot of people won't have done so - if there's stuff people
are querying you're going to have to explain things, even if they're
well established device tree conventions.

This is going to be one of the biggest challenges with rolling out
device tree, both from the point of view of maintainers learning about
how the device tree stuff should look and from the point of view of
maintainers learning who to trust on device tree.  There's a few people
like you and Grant that I know are really familiar with this stuff but
there's also going to be a whole raft of new people submitting device
tree support and if something looks off maintainers are going to have to
work out if it's due to some peculiar device tree idiom or if it's just
an issue in the patch.