* [RFC 0/2] ARM: Tegra: Device Tree: Audio @ 2011-05-27 20:56 ` John Bonesio 0 siblings, 0 replies; 92+ messages in thread From: John Bonesio @ 2011-05-27 20:56 UTC (permalink / raw) To: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Cc: glikely-s3s/WqlpOiPyB63q8FvJNQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 0/2] ARM: Tegra: Device Tree: Audio @ 2011-05-27 20:56 ` John Bonesio 0 siblings, 0 replies; 92+ messages in thread From: John Bonesio @ 2011-05-27 20:56 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-05-27 20:56 ` John Bonesio (?) @ 2011-05-27 20:57 ` John Bonesio 2011-05-27 21:05 ` Grant Likely ` (2 more replies) -1 siblings, 3 replies; 92+ messages in thread From: John Bonesio @ 2011-05-27 20:57 UTC (permalink / raw) To: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Cc: glikely-s3s/WqlpOiPyB63q8FvJNQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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), ^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-05-27 20:57 ` [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree John Bonesio @ 2011-05-27 21:05 ` Grant Likely 2011-05-28 1:28 ` Mark Brown 2011-06-01 7:07 ` Barry Song 2 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-27 21:05 UTC (permalink / raw) To: John Bonesio Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-05-27 21:05 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-27 21:05 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-05-27 20:57 ` [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree John Bonesio @ 2011-05-28 1:28 ` Mark Brown 2011-05-28 1:28 ` Mark Brown 2011-06-01 7:07 ` Barry Song 2 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-28 1:28 UTC (permalink / raw) To: John Bonesio Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-05-28 1:28 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-28 1:28 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-05-27 20:57 ` [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree John Bonesio @ 2011-06-01 7:07 ` Barry Song 2011-05-28 1:28 ` Mark Brown 2011-06-01 7:07 ` Barry Song 2 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-01 7:07 UTC (permalink / raw) To: John Bonesio Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, olofj-hpIqsD4AKlfQT0dZR+AlfA, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, glikely-s3s/WqlpOiPyB63q8FvJNQ 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-01 7:07 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-01 7:07 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTi=NMjBjWVv_o3PocejhgGr8TdG+1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-01 7:07 ` Barry Song @ 2011-06-01 16:47 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-01 16:47 UTC (permalink / raw) To: Barry Song Cc: John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, olofj-hpIqsD4AKlfQT0dZR+AlfA, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-01 16:47 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-01 16:47 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTi=PtEhxxmZo2DqvmySCmnEd3LbezQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-01 16:47 ` Grant Likely @ 2011-06-02 9:07 ` Barry Song -1 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-02 9:07 UTC (permalink / raw) To: Grant Likely Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, linux-tegra-u79uwXL29TY76Z2rM5mHXA, John Bonesio, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-02 9:07 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-02 9:07 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTikWpiY_o27OF-Jxvq7iPeWzrAYOsQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 9:07 ` Barry Song @ 2011-06-02 16:04 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 16:04 UTC (permalink / raw) To: Barry Song Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, linux-tegra-u79uwXL29TY76Z2rM5mHXA, John Bonesio, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-02 16:04 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 16:04 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110602160445.GA8373-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 16:04 ` Grant Likely @ 2011-06-02 16:21 ` Barry Song -1 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-02 16:21 UTC (permalink / raw) To: Grant Likely Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, linux-tegra-u79uwXL29TY76Z2rM5mHXA, John Bonesio, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. > > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-02 16:21 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-02 16:21 UTC (permalink / raw) To: linux-arm-kernel 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. > > ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTikGp_O-Wd3nMhbV+-RLeXZoWeB6eQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 16:21 ` Barry Song @ 2011-06-02 21:43 ` Russell King - ARM Linux -1 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-02 21:43 UTC (permalink / raw) To: Barry Song Cc: Grant Likely, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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(). ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-02 21:43 ` Russell King - ARM Linux 0 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-02 21:43 UTC (permalink / raw) To: linux-arm-kernel 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(). ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110602214322.GC10532-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 21:43 ` Russell King - ARM Linux @ 2011-06-03 2:32 ` Barry Song -1 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-03 2:32 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Grant Likely, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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(). ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-03 2:32 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-03 2:32 UTC (permalink / raw) To: linux-arm-kernel 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(). ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTikiaA3CCRAXkB=x56wtt8nNF9dqxA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-03 2:32 ` Barry Song @ 2011-06-03 6:20 ` Russell King - ARM Linux -1 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-03 6:20 UTC (permalink / raw) To: Barry Song Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-03 6:20 ` Russell King - ARM Linux 0 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-03 6:20 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 16:04 ` Grant Likely @ 2011-06-02 21:36 ` Russell King - ARM Linux -1 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-02 21:36 UTC (permalink / raw) To: Grant Likely Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-02 21:36 ` Russell King - ARM Linux 0 siblings, 0 replies; 92+ messages in thread From: Russell King - ARM Linux @ 2011-06-02 21:36 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110602213656.GB10532-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 21:36 ` Russell King - ARM Linux @ 2011-06-03 1:19 ` Barry Song -1 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-03 1:19 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Grant Likely, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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? > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-03 1:19 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-03 1:19 UTC (permalink / raw) To: linux-arm-kernel 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? > ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTimavC-6oMhAHHsBJ2_Em7aXi7eyNQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-03 1:19 ` Barry Song @ 2011-06-07 3:44 ` Barry Song -1 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-07 3:44 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Grant Likely, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. > >> > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-07 3:44 ` Barry Song 0 siblings, 0 replies; 92+ messages in thread From: Barry Song @ 2011-06-07 3:44 UTC (permalink / raw) To: linux-arm-kernel 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. > >> > ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. 2011-06-02 21:36 ` Russell King - ARM Linux @ 2011-06-14 15:42 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-14 15:42 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Barry Song, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, olofj-hpIqsD4AKlfQT0dZR+AlfA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree. @ 2011-06-14 15:42 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-14 15:42 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-27 20:56 ` John Bonesio (?) (?) @ 2011-05-27 20:57 ` John Bonesio 2011-05-27 21:06 ` Grant Likely 2011-05-28 1:24 ` Mark Brown -1 siblings, 2 replies; 92+ messages in thread From: John Bonesio @ 2011-05-27 20:57 UTC (permalink / raw) To: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Cc: glikely-s3s/WqlpOiPyB63q8FvJNQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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; ^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-27 20:57 ` [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's " John Bonesio @ 2011-05-27 21:06 ` Grant Likely 2011-05-28 1:24 ` Mark Brown 1 sibling, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-27 21:06 UTC (permalink / raw) To: John Bonesio Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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; > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-27 21:06 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-27 21:06 UTC (permalink / raw) To: linux-arm-kernel 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; > ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-27 20:57 ` [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's " John Bonesio @ 2011-05-28 1:24 ` Mark Brown 2011-05-28 1:24 ` Mark Brown 1 sibling, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-28 1:24 UTC (permalink / raw) To: John Bonesio Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, olofj-hpIqsD4AKlfQT0dZR+AlfA, swarren-DDmLM1+adcrQT0dZR+AlfA 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-28 1:24 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-28 1:24 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110528012427.GB5971-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-28 1:24 ` Mark Brown @ 2011-05-30 3:11 ` Olof Johansson -1 siblings, 0 replies; 92+ messages in thread From: Olof Johansson @ 2011-05-30 3:11 UTC (permalink / raw) To: Mark Brown Cc: John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 3:11 ` Olof Johansson 0 siblings, 0 replies; 92+ messages in thread From: Olof Johansson @ 2011-05-30 3:11 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <BANLkTinKLcYmStvBEGDcN84BapJXe5Y5bg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 3:11 ` Olof Johansson @ 2011-05-30 3:38 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 3:38 UTC (permalink / raw) To: Olof Johansson Cc: John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 3:38 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 3:38 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110530033826.GE4130-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 3:38 ` Mark Brown @ 2011-05-30 6:11 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 6:11 UTC (permalink / raw) To: Mark Brown Cc: Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren, wmb-D5eQfiDGL7eakBO8gow8eQ, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 6:11 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 6:11 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110530061155.GC23517-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 6:11 ` Grant Likely @ 2011-05-30 6:18 ` Mitch Bradley -1 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-30 6:18 UTC (permalink / raw) To: Grant Likely Cc: glikely-s3s/WqlpOiPyB63q8FvJNQ, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 6:18 ` Mitch Bradley 0 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-30 6:18 UTC (permalink / raw) To: linux-arm-kernel 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. > ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <4DE336A1.5040509-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 6:18 ` Mitch Bradley @ 2011-05-30 6:22 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 6:22 UTC (permalink / raw) To: Mitch Bradley Cc: benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, John Bonesio, Stephen Warren, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 6:22 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 6:22 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 6:18 ` Mitch Bradley @ 2011-05-30 7:01 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 7:01 UTC (permalink / raw) To: Mitch Bradley Cc: Grant Likely, Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r 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". ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 7:01 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 7:01 UTC (permalink / raw) To: linux-arm-kernel 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". ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110530070138.GA5036-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 7:01 ` Mark Brown @ 2011-05-30 16:22 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 16:22 UTC (permalink / raw) To: Mark Brown Cc: Mitch Bradley, Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Stephen Warren, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 16:22 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 16:22 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 7:01 ` Mark Brown @ 2011-05-30 18:54 ` Segher Boessenkool -1 siblings, 0 replies; 92+ messages in thread From: Segher Boessenkool @ 2011-05-30 18:54 UTC (permalink / raw) To: Mark Brown Cc: Mitch Bradley, glikely-s3s/WqlpOiPyB63q8FvJNQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r >> 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 18:54 ` Segher Boessenkool 0 siblings, 0 replies; 92+ messages in thread From: Segher Boessenkool @ 2011-05-30 18:54 UTC (permalink / raw) To: linux-arm-kernel >> 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <8d2515b13c817cc956b185d043bcef00-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 18:54 ` Segher Boessenkool @ 2011-05-30 19:20 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 19:20 UTC (permalink / raw) To: Segher Boessenkool Cc: Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Mitch Bradley, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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: ^ permalink raw reply related [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 19:20 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-05-30 19:20 UTC (permalink / raw) To: linux-arm-kernel 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: ^ permalink raw reply related [flat|nested] 92+ messages in thread
[parent not found: <BANLkTi=hkScxYpt449CQCky+bLU3UvkC_A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 19:20 ` Grant Likely @ 2011-05-30 20:53 ` Mitch Bradley -1 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-30 20:53 UTC (permalink / raw) To: Grant Likely Cc: Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 20:53 ` Mitch Bradley 0 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-30 20:53 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <4DE403C5.7060003-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]
* RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 20:53 ` Mitch Bradley @ 2011-05-31 17:55 ` Stephen Warren -1 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-05-31 17:55 UTC (permalink / raw) To: Mitch Bradley, Grant Likely Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-31 17:55 ` Stephen Warren 0 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-05-31 17:55 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <74CDBE0F657A3D45AFBB94109FB122FF0498E1C22D-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-31 17:55 ` Stephen Warren @ 2011-05-31 18:42 ` Mitch Bradley -1 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-31 18:42 UTC (permalink / raw) To: Stephen Warren Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-31 18:42 ` Mitch Bradley 0 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-05-31 18:42 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <4DE536AD.5080200-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]
* RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-31 18:42 ` Mitch Bradley @ 2011-06-01 15:59 ` Stephen Warren -1 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-06-01 15:59 UTC (permalink / raw) To: Mitch Bradley Cc: Grant Likely, Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-01 15:59 ` Stephen Warren 0 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-06-01 15:59 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <74CDBE0F657A3D45AFBB94109FB122FF0498E1C3F5-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-06-01 15:59 ` Stephen Warren @ 2011-06-01 16:18 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-06-01 16:18 UTC (permalink / raw) To: Stephen Warren Cc: Mitch Bradley, Grant Likely, Segher Boessenkool, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-01 16:18 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-06-01 16:18 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110601161856.GB15583-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-06-01 16:18 ` Mark Brown @ 2011-06-02 15:40 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 15:40 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, Mitch Bradley, Segher Boessenkool, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-02 15:40 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 15:40 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-06-01 15:59 ` Stephen Warren @ 2011-06-01 21:32 ` Mitch Bradley -1 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-06-01 21:32 UTC (permalink / raw) To: Stephen Warren Cc: Grant Likely, Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. > > ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-01 21:32 ` Mitch Bradley 0 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-06-01 21:32 UTC (permalink / raw) To: linux-arm-kernel 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. > > ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <4DE6AFF7.3040905-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]
* RE: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-06-01 21:32 ` Mitch Bradley @ 2011-06-03 21:24 ` Stephen Warren -1 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-06-03 21:24 UTC (permalink / raw) To: Mitch Bradley Cc: Grant Likely, Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-03 21:24 ` Stephen Warren 0 siblings, 0 replies; 92+ messages in thread From: Stephen Warren @ 2011-06-03 21:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <74CDBE0F657A3D45AFBB94109FB122FF0498E1C870-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-06-03 21:24 ` Stephen Warren @ 2011-06-04 0:25 ` Mitch Bradley -1 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-06-04 0:25 UTC (permalink / raw) To: Stephen Warren Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-04 0:25 ` Mitch Bradley 0 siblings, 0 replies; 92+ messages in thread From: Mitch Bradley @ 2011-06-04 0:25 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-31 17:55 ` Stephen Warren @ 2011-06-02 14:59 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 14:59 UTC (permalink / raw) To: Stephen Warren Cc: Mitch Bradley, Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-02 14:59 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 14:59 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 20:53 ` Mitch Bradley @ 2011-06-02 15:40 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 15:40 UTC (permalink / raw) To: Mitch Bradley Cc: Segher Boessenkool, Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-02 15:40 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-02 15:40 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 19:20 ` Grant Likely @ 2011-06-28 21:39 ` Grant Likely -1 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-28 21:39 UTC (permalink / raw) To: Segher Boessenkool Cc: Mark Brown, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Mitch Bradley, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-06-28 21:39 ` Grant Likely 0 siblings, 0 replies; 92+ messages in thread From: Grant Likely @ 2011-06-28 21:39 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 7:01 ` Mark Brown @ 2011-05-30 23:27 ` Benjamin Herrenschmidt -1 siblings, 0 replies; 92+ messages in thread From: Benjamin Herrenschmidt @ 2011-05-30 23:27 UTC (permalink / raw) To: Mark Brown Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, glikely-s3s/WqlpOiPyB63q8FvJNQ, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 23:27 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 92+ messages in thread From: Benjamin Herrenschmidt @ 2011-05-30 23:27 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 23:27 ` Benjamin Herrenschmidt @ 2011-05-30 23:49 ` Olof Johansson -1 siblings, 0 replies; 92+ messages in thread From: Olof Johansson @ 2011-05-30 23:49 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, glikely-s3s/WqlpOiPyB63q8FvJNQ, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 23:49 ` Olof Johansson 0 siblings, 0 replies; 92+ messages in thread From: Olof Johansson @ 2011-05-30 23:49 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
[parent not found: <20110530234909.GA3411-O5ziIzlqnXUVNXGz7ipsyg@public.gmane.org>]
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 23:49 ` Olof Johansson @ 2011-05-31 0:58 ` Segher Boessenkool -1 siblings, 0 replies; 92+ messages in thread From: Segher Boessenkool @ 2011-05-31 0:58 UTC (permalink / raw) To: Olof Johansson Cc: glikely-s3s/WqlpOiPyB63q8FvJNQ, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Brown, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r > 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-31 0:58 ` Segher Boessenkool 0 siblings, 0 replies; 92+ messages in thread From: Segher Boessenkool @ 2011-05-31 0:58 UTC (permalink / raw) To: linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 23:49 ` Olof Johansson @ 2011-05-31 10:24 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-31 10:24 UTC (permalink / raw) To: Olof Johansson Cc: Benjamin Herrenschmidt, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, glikely-s3s/WqlpOiPyB63q8FvJNQ, Olof Johansson, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-31 10:24 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-31 10:24 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 6:11 ` Grant Likely @ 2011-05-30 7:10 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 7:10 UTC (permalink / raw) To: Grant Likely Cc: Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren, wmb-D5eQfiDGL7eakBO8gow8eQ, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 7:10 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-30 7:10 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 6:11 ` Grant Likely @ 2011-05-30 23:26 ` Benjamin Herrenschmidt -1 siblings, 0 replies; 92+ messages in thread From: Benjamin Herrenschmidt @ 2011-05-30 23:26 UTC (permalink / raw) To: Grant Likely Cc: Mark Brown, Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren, wmb-D5eQfiDGL7eakBO8gow8eQ 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-30 23:26 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 92+ messages in thread From: Benjamin Herrenschmidt @ 2011-05-30 23:26 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. 2011-05-30 23:26 ` Benjamin Herrenschmidt @ 2011-05-31 10:03 ` Mark Brown -1 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-31 10:03 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Grant Likely, Olof Johansson, John Bonesio, linux-tegra-u79uwXL29TY76Z2rM5mHXA, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, glikely-s3s/WqlpOiPyB63q8FvJNQ, Stephen Warren, wmb-D5eQfiDGL7eakBO8gow8eQ 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
* [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree. @ 2011-05-31 10:03 ` Mark Brown 0 siblings, 0 replies; 92+ messages in thread From: Mark Brown @ 2011-05-31 10:03 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 92+ messages in thread
end of thread, other threads:[~2011-06-28 21:39 UTC | newest] Thread overview: 92+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-27 20:56 [RFC 0/2] ARM: Tegra: Device Tree: Audio John Bonesio 2011-05-27 20:56 ` John Bonesio 2011-05-27 20:57 ` [RFC 1/2] ARM:Tegra: Device Tree Support: Initialize the audio card from the device tree John Bonesio 2011-05-27 21:05 ` Grant Likely 2011-05-27 21:05 ` Grant Likely 2011-05-28 1:28 ` Mark Brown 2011-05-28 1:28 ` Mark Brown 2011-06-01 7:07 ` Barry Song 2011-06-01 7:07 ` Barry Song [not found] ` <BANLkTi=NMjBjWVv_o3PocejhgGr8TdG+1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-01 16:47 ` Grant Likely 2011-06-01 16:47 ` Grant Likely [not found] ` <BANLkTi=PtEhxxmZo2DqvmySCmnEd3LbezQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-02 9:07 ` Barry Song 2011-06-02 9:07 ` Barry Song [not found] ` <BANLkTikWpiY_o27OF-Jxvq7iPeWzrAYOsQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-02 16:04 ` Grant Likely 2011-06-02 16:04 ` Grant Likely [not found] ` <20110602160445.GA8373-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-06-02 16:21 ` Barry Song 2011-06-02 16:21 ` Barry Song [not found] ` <BANLkTikGp_O-Wd3nMhbV+-RLeXZoWeB6eQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-02 21:43 ` Russell King - ARM Linux 2011-06-02 21:43 ` Russell King - ARM Linux [not found] ` <20110602214322.GC10532-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2011-06-03 2:32 ` Barry Song 2011-06-03 2:32 ` Barry Song [not found] ` <BANLkTikiaA3CCRAXkB=x56wtt8nNF9dqxA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-03 6:20 ` Russell King - ARM Linux 2011-06-03 6:20 ` Russell King - ARM Linux 2011-06-02 21:36 ` Russell King - ARM Linux 2011-06-02 21:36 ` Russell King - ARM Linux [not found] ` <20110602213656.GB10532-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2011-06-03 1:19 ` Barry Song 2011-06-03 1:19 ` Barry Song [not found] ` <BANLkTimavC-6oMhAHHsBJ2_Em7aXi7eyNQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-06-07 3:44 ` Barry Song 2011-06-07 3:44 ` Barry Song 2011-06-14 15:42 ` Grant Likely 2011-06-14 15:42 ` Grant Likely 2011-05-27 20:57 ` [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's " John Bonesio 2011-05-27 21:06 ` Grant Likely 2011-05-27 21:06 ` Grant Likely 2011-05-28 1:24 ` Mark Brown 2011-05-28 1:24 ` Mark Brown [not found] ` <20110528012427.GB5971-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-05-30 3:11 ` Olof Johansson 2011-05-30 3:11 ` Olof Johansson [not found] ` <BANLkTinKLcYmStvBEGDcN84BapJXe5Y5bg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-05-30 3:38 ` Mark Brown 2011-05-30 3:38 ` Mark Brown [not found] ` <20110530033826.GE4130-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-05-30 6:11 ` Grant Likely 2011-05-30 6:11 ` Grant Likely [not found] ` <20110530061155.GC23517-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-05-30 6:18 ` Mitch Bradley 2011-05-30 6:18 ` Mitch Bradley [not found] ` <4DE336A1.5040509-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> 2011-05-30 6:22 ` Grant Likely 2011-05-30 6:22 ` Grant Likely 2011-05-30 7:01 ` Mark Brown 2011-05-30 7:01 ` Mark Brown [not found] ` <20110530070138.GA5036-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-05-30 16:22 ` Grant Likely 2011-05-30 16:22 ` Grant Likely 2011-05-30 18:54 ` Segher Boessenkool 2011-05-30 18:54 ` Segher Boessenkool [not found] ` <8d2515b13c817cc956b185d043bcef00-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> 2011-05-30 19:20 ` Grant Likely 2011-05-30 19:20 ` Grant Likely [not found] ` <BANLkTi=hkScxYpt449CQCky+bLU3UvkC_A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-05-30 20:53 ` Mitch Bradley 2011-05-30 20:53 ` Mitch Bradley [not found] ` <4DE403C5.7060003-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> 2011-05-31 17:55 ` Stephen Warren 2011-05-31 17:55 ` Stephen Warren [not found] ` <74CDBE0F657A3D45AFBB94109FB122FF0498E1C22D-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org> 2011-05-31 18:42 ` Mitch Bradley 2011-05-31 18:42 ` Mitch Bradley [not found] ` <4DE536AD.5080200-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> 2011-06-01 15:59 ` Stephen Warren 2011-06-01 15:59 ` Stephen Warren [not found] ` <74CDBE0F657A3D45AFBB94109FB122FF0498E1C3F5-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org> 2011-06-01 16:18 ` Mark Brown 2011-06-01 16:18 ` Mark Brown [not found] ` <20110601161856.GB15583-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-06-02 15:40 ` Grant Likely 2011-06-02 15:40 ` Grant Likely 2011-06-01 21:32 ` Mitch Bradley 2011-06-01 21:32 ` Mitch Bradley [not found] ` <4DE6AFF7.3040905-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> 2011-06-03 21:24 ` Stephen Warren 2011-06-03 21:24 ` Stephen Warren [not found] ` <74CDBE0F657A3D45AFBB94109FB122FF0498E1C870-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org> 2011-06-04 0:25 ` Mitch Bradley 2011-06-04 0:25 ` Mitch Bradley 2011-06-02 14:59 ` Grant Likely 2011-06-02 14:59 ` Grant Likely 2011-06-02 15:40 ` Grant Likely 2011-06-02 15:40 ` Grant Likely 2011-06-28 21:39 ` Grant Likely 2011-06-28 21:39 ` Grant Likely 2011-05-30 23:27 ` Benjamin Herrenschmidt 2011-05-30 23:27 ` Benjamin Herrenschmidt 2011-05-30 23:49 ` Olof Johansson 2011-05-30 23:49 ` Olof Johansson [not found] ` <20110530234909.GA3411-O5ziIzlqnXUVNXGz7ipsyg@public.gmane.org> 2011-05-31 0:58 ` Segher Boessenkool 2011-05-31 0:58 ` Segher Boessenkool 2011-05-31 10:24 ` Mark Brown 2011-05-31 10:24 ` Mark Brown 2011-05-30 7:10 ` Mark Brown 2011-05-30 7:10 ` Mark Brown 2011-05-30 23:26 ` Benjamin Herrenschmidt 2011-05-30 23:26 ` Benjamin Herrenschmidt 2011-05-31 10:03 ` Mark Brown 2011-05-31 10:03 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.