diff mbox

[v4] ARM:dts:omap4-panda: Update the LED support for the panda DTS

Message ID 1368636386-17138-1-git-send-email-dmurphy@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Murphy May 15, 2013, 4:46 p.m. UTC
The GPIO for LED D1 on the omap4-panda a1-a3 rev and the omap4-panda-es
are different.

A1-A3 = gpio_wk7
ES = gpio_110

There is no change to LED D2

Abstract away the pinmux and the LED definitions for the two boards into
the respective DTS files.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/omap4-panda-common.dtsi |   16 +++++++++++-
 arch/arm/boot/dts/omap4-panda-es.dts      |   40 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletions(-)

Comments

Nishanth Menon May 15, 2013, 5:05 p.m. UTC | #1
On 11:46-20130515, Dan Murphy wrote:
> The GPIO for LED D1 on the omap4-panda a1-a3 rev and the omap4-panda-es
> are different.
> 
> A1-A3 = gpio_wk7
> ES = gpio_110
> 
> There is no change to LED D2
> 
> Abstract away the pinmux and the LED definitions for the two boards into
> the respective DTS files.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
nit: Giving patch history is a nice practise.
>  arch/arm/boot/dts/omap4-panda-common.dtsi |   16 +++++++++++-
>  arch/arm/boot/dts/omap4-panda-es.dts      |   40 +++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
> index 03bd60d..2b516af 100644
> --- a/arch/arm/boot/dts/omap4-panda-common.dtsi
> +++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
> @@ -16,7 +16,7 @@
>  		reg = <0x80000000 0x40000000>; /* 1 GB */
>  	};
>  
> -	leds {
> +	leds: leds {
>  		compatible = "gpio-leds";
>  		heartbeat {
>  			label = "pandaboard::status1";
> @@ -137,6 +137,20 @@
I missed noticing this previously, Apologies on the same.
Considering that drivers/leds/leds-gpio.c has ability to handle pinctrl,
One better option might be to provide pinctrl phandle with leds -
Couple of reasons why this might be good:
a) one gets the following warning at boot:
"leds-gpio leds.8: pins are not configured from the driver"
b) you donot need to setup the pins by default at boot - it is not
mandatory for the system functionality, instead we do it *if* the driver
is enabled.
Further, optionally, all you'd have to do in panda-es.dts is the following
&led_wkgpio_pins {
		pinctrl-single,pins = <
			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
		>;
}
Similarly for gpios override for panda-es.

>  	};
>  };
>  
> +&omap4_pmx_wkup {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <
> +		&led_wkgpio_pins
> +	>;
> +
> +	led_wkgpio_pins: pinmux_leds_wkpins {
> +		pinctrl-single,pins = <
> +			0x1a 0x3	/* gpio_wk7 OUTPUT | MODE 3 */
> +			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
> +		>;
> +	};
> +};
> +
>  &i2c1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&i2c1_pins>;
> diff --git a/arch/arm/boot/dts/omap4-panda-es.dts b/arch/arm/boot/dts/omap4-panda-es.dts
> index f1d8c21..e6f696d 100644
> --- a/arch/arm/boot/dts/omap4-panda-es.dts
> +++ b/arch/arm/boot/dts/omap4-panda-es.dts
> @@ -34,3 +34,43 @@
>  		0x5e 0x100	/* hdmi_sda.hdmi_sda INPUT | MODE 0 */
>  		>;
>  };
> +
> +&leds {
> +	compatible = "gpio-leds";
> +	heartbeat {
> +		label = "pandaboard::status1";
> +		gpios = <&gpio4 14 0>;
> +		linux,default-trigger = "heartbeat";
> +	};
> +	mmc {
> +		label = "pandaboard::status2";
> +		gpios = <&gpio1 8 0>;
> +		linux,default-trigger = "mmc0";
> +	};
> +};
> +
> +&omap4_pmx_core {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <
> +		&led_gpio_pins
> +	>;
> +
> +	led_gpio_pins: gpio_led_pmx {
> +		pinctrl-single,pins = <
> +			0xb6 0x3	/* gpio_110 OUTPUT | MODE 3 */
> +		>;
> +	};
> +};
> +
> +&omap4_pmx_wkup {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <
> +		&led_wkgpio_pins
> +	>;
> +
> +	led_wkgpio_pins: pinmux_leds_wkpins {
> +		pinctrl-single,pins = <
> +			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
> +		>;
> +	};
> +};
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Murphy May 16, 2013, 3:35 p.m. UTC | #2
On 05/15/2013 12:05 PM, Nishanth Menon wrote:
> On 11:46-20130515, Dan Murphy wrote:
>> The GPIO for LED D1 on the omap4-panda a1-a3 rev and the omap4-panda-es
>> are different.
>>
>> A1-A3 = gpio_wk7
>> ES = gpio_110
>>
>> There is no change to LED D2
>>
>> Abstract away the pinmux and the LED definitions for the two boards into
>> the respective DTS files.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
> nit: Giving patch history is a nice practise.
>>  arch/arm/boot/dts/omap4-panda-common.dtsi |   16 +++++++++++-
>>  arch/arm/boot/dts/omap4-panda-es.dts      |   40 +++++++++++++++++++++++++++++
>>  2 files changed, 55 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
>> index 03bd60d..2b516af 100644
>> --- a/arch/arm/boot/dts/omap4-panda-common.dtsi
>> +++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
>> @@ -16,7 +16,7 @@
>>  		reg = <0x80000000 0x40000000>; /* 1 GB */
>>  	};
>>  
>> -	leds {
>> +	leds: leds {
>>  		compatible = "gpio-leds";
>>  		heartbeat {
>>  			label = "pandaboard::status1";
>> @@ -137,6 +137,20 @@
> I missed noticing this previously, Apologies on the same.
> Considering that drivers/leds/leds-gpio.c has ability to handle pinctrl,
> One better option might be to provide pinctrl phandle with leds -
> Couple of reasons why this might be good:
> a) one gets the following warning at boot:
> "leds-gpio leds.8: pins are not configured from the driver"
> b) you donot need to setup the pins by default at boot - it is not
> mandatory for the system functionality, instead we do it *if* the driver
> is enabled.
> Further, optionally, all you'd have to do in panda-es.dts is the following
> &led_wkgpio_pins {
> 		pinctrl-single,pins = <
> 			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
> 		>;
> }
> Similarly for gpios override for panda-es.
I am not sure you really want to do this.
If I make the pinctrl part of the led structure then the only way the gpio_wk7 on a1-a3 to be configured is when
the CONFIG_LEDS_GPIO flag is set.

Do you really want that dependency?  You did say it was a key fix
At least this way the pins are configured regardless of that flag.
>>  	};
>>  };
>>  
>> +&omap4_pmx_wkup {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <
>> +		&led_wkgpio_pins
>> +	>;
>> +
>> +	led_wkgpio_pins: pinmux_leds_wkpins {
>> +		pinctrl-single,pins = <
>> +			0x1a 0x3	/* gpio_wk7 OUTPUT | MODE 3 */
>> +			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
>> +		>;
>> +	};
>> +};
>> +
>>  &i2c1 {
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&i2c1_pins>;
>> diff --git a/arch/arm/boot/dts/omap4-panda-es.dts b/arch/arm/boot/dts/omap4-panda-es.dts
>> index f1d8c21..e6f696d 100644
>> --- a/arch/arm/boot/dts/omap4-panda-es.dts
>> +++ b/arch/arm/boot/dts/omap4-panda-es.dts
>> @@ -34,3 +34,43 @@
>>  		0x5e 0x100	/* hdmi_sda.hdmi_sda INPUT | MODE 0 */
>>  		>;
>>  };
>> +
>> +&leds {
>> +	compatible = "gpio-leds";
>> +	heartbeat {
>> +		label = "pandaboard::status1";
>> +		gpios = <&gpio4 14 0>;
>> +		linux,default-trigger = "heartbeat";
>> +	};
>> +	mmc {
>> +		label = "pandaboard::status2";
>> +		gpios = <&gpio1 8 0>;
>> +		linux,default-trigger = "mmc0";
>> +	};
>> +};
>> +
>> +&omap4_pmx_core {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <
>> +		&led_gpio_pins
>> +	>;
>> +
>> +	led_gpio_pins: gpio_led_pmx {
>> +		pinctrl-single,pins = <
>> +			0xb6 0x3	/* gpio_110 OUTPUT | MODE 3 */
>> +		>;
>> +	};
>> +};
>> +
>> +&omap4_pmx_wkup {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <
>> +		&led_wkgpio_pins
>> +	>;
>> +
>> +	led_wkgpio_pins: pinmux_leds_wkpins {
>> +		pinctrl-single,pins = <
>> +			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
>> +		>;
>> +	};
>> +};
>> -- 
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon May 16, 2013, 6:18 p.m. UTC | #3
On Thu, May 16, 2013 at 10:35 AM, Dan Murphy <dmurphy@ti.com> wrote:
> I am not sure you really want to do this.
> If I make the pinctrl part of the led structure then the only way the gpio_wk7 on a1-a3 to be configured is when
> the CONFIG_LEDS_GPIO flag is set.
>
> Do you really want that dependency?  You did say it was a key fix
> At least this way the pins are configured regardless of that flag.

That is better as the system will be left in the pinmux configuration
handed over from bootloader.

The point being, muxing up pins even when not needed(config switched
off) has no real benefit - in this case albeit, the default mux was
causing a bug.
pinctrl IMHO should be considered as any other resource, if it is not
mandatory for boot, and needed only for a device functionality when
probed, it should done there only.

just my 2 cents.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Murphy May 16, 2013, 8:22 p.m. UTC | #4
On 05/16/2013 01:18 PM, Nishanth Menon wrote:
> On Thu, May 16, 2013 at 10:35 AM, Dan Murphy <dmurphy@ti.com> wrote:
>> I am not sure you really want to do this.
>> If I make the pinctrl part of the led structure then the only way the gpio_wk7 on a1-a3 to be configured is when
>> the CONFIG_LEDS_GPIO flag is set.
>>
>> Do you really want that dependency?  You did say it was a key fix
>> At least this way the pins are configured regardless of that flag.
> That is better as the system will be left in the pinmux configuration
> handed over from bootloader.
So you want to depend on a boot loader to configure pins correctly for the kernel?
Hmmm seems risky to me.
> The point being, muxing up pins even when not needed(config switched
> off) has no real benefit - in this case albeit, the default mux was
> causing a bug.
> pinctrl IMHO should be considered as any other resource, if it is not
> mandatory for boot, and needed only for a device functionality when
> probed, it should done there only.
>
> just my 2 cents.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nishanth Menon May 16, 2013, 8:42 p.m. UTC | #5
On Thu, May 16, 2013 at 3:22 PM, Dan Murphy <dmurphy@ti.com> wrote:
> On 05/16/2013 01:18 PM, Nishanth Menon wrote:
>> On Thu, May 16, 2013 at 10:35 AM, Dan Murphy <dmurphy@ti.com> wrote:
>>> I am not sure you really want to do this.
>>> If I make the pinctrl part of the led structure then the only way the gpio_wk7 on a1-a3 to be configured is when
>>> the CONFIG_LEDS_GPIO flag is set.
>>>
>>> Do you really want that dependency?  You did say it was a key fix
>>> At least this way the pins are configured regardless of that flag.
>> That is better as the system will be left in the pinmux configuration
>> handed over from bootloader.
> So you want to depend on a boot loader to configure pins correctly for the kernel?
> Hmmm seems risky to me.

Not really! if it is a critical pinmux, pinmux defaults are great,
else belongs definitely to device->pinctrl :) depending on bootloader
pinmux implies NOT having pinmux even for device - NO. that is
definitely not what I stated.
Regards,
NM
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/boot/dts/omap4-panda-common.dtsi b/arch/arm/boot/dts/omap4-panda-common.dtsi
index 03bd60d..2b516af 100644
--- a/arch/arm/boot/dts/omap4-panda-common.dtsi
+++ b/arch/arm/boot/dts/omap4-panda-common.dtsi
@@ -16,7 +16,7 @@ 
 		reg = <0x80000000 0x40000000>; /* 1 GB */
 	};
 
-	leds {
+	leds: leds {
 		compatible = "gpio-leds";
 		heartbeat {
 			label = "pandaboard::status1";
@@ -137,6 +137,20 @@ 
 	};
 };
 
+&omap4_pmx_wkup {
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&led_wkgpio_pins
+	>;
+
+	led_wkgpio_pins: pinmux_leds_wkpins {
+		pinctrl-single,pins = <
+			0x1a 0x3	/* gpio_wk7 OUTPUT | MODE 3 */
+			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
+		>;
+	};
+};
+
 &i2c1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c1_pins>;
diff --git a/arch/arm/boot/dts/omap4-panda-es.dts b/arch/arm/boot/dts/omap4-panda-es.dts
index f1d8c21..e6f696d 100644
--- a/arch/arm/boot/dts/omap4-panda-es.dts
+++ b/arch/arm/boot/dts/omap4-panda-es.dts
@@ -34,3 +34,43 @@ 
 		0x5e 0x100	/* hdmi_sda.hdmi_sda INPUT | MODE 0 */
 		>;
 };
+
+&leds {
+	compatible = "gpio-leds";
+	heartbeat {
+		label = "pandaboard::status1";
+		gpios = <&gpio4 14 0>;
+		linux,default-trigger = "heartbeat";
+	};
+	mmc {
+		label = "pandaboard::status2";
+		gpios = <&gpio1 8 0>;
+		linux,default-trigger = "mmc0";
+	};
+};
+
+&omap4_pmx_core {
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&led_gpio_pins
+	>;
+
+	led_gpio_pins: gpio_led_pmx {
+		pinctrl-single,pins = <
+			0xb6 0x3	/* gpio_110 OUTPUT | MODE 3 */
+		>;
+	};
+};
+
+&omap4_pmx_wkup {
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&led_wkgpio_pins
+	>;
+
+	led_wkgpio_pins: pinmux_leds_wkpins {
+		pinctrl-single,pins = <
+			0x1c 0x3	/* gpio_wk8 OUTPUT | MODE 3 */
+		>;
+	};
+};