Skip to contentSkip to conversation

Registering GPIO LEDs

DISCUSSION Ian Pilcher 2 participants 5 messages Aug 7, 2013 – Aug 13, 2013 LED

lore
Share & tools
lore
#0 lore raw

I've discovered that it's quite simple to control the drive activity LEDs on my Thecus N5550 with the standard gpio_ich module. AFAIK, this means that I should be able to use them with the leds_gpio module. I just need to register them.

I've found this page that gives an example:

http://fabiobaltieri.com/2011/09/21/linux-led-subsystem/

I'm confused, however, about the .gpio member of the struct gpio_led. I'm guessing that it's the GPIO number, but (at least in the case of gpio_ich) that number can vary depending on the base number for that chip.

Am I missing something?

Thanks!

Signature · 4 lines
========================================================================
Ian Pilcher                                         [email protected]
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================
#1 lore raw

Hi Ian,

Quoted 7 lines

I've discovered that it's quite simple to control the drive activity LEDs on my Thecus N5550 with the standard gpio_ich module. AFAIK, this means that I should be able to use them with the leds_gpio module. I just need to register them.

I've found this page that gives an example:

Quoted 8 lines

http://fabiobaltieri.com/2011/09/21/linux-led-subsystem/

I'm confused, however, about the .gpio member of the struct gpio_led. I'm guessing that it's the GPIO number, but (at least in the case of gpio_ich) that number can vary depending on the base number for that chip.

From the viewpoint of a GPIO consumer, we don't care the base GPIO number.

That is required when a GPIO controller is added.
Just check '/sys/kernel/debug/gpio' and use available GPIOs for LEDs.

Regards,
Milo

#2 lore raw
Quoted 3 lines · Milo

From the viewpoint of a GPIO consumer, we don't care the base GPIO number. That is required when a GPIO controller is added.
Just check '/sys/kernel/debug/gpio' and use available GPIOs for LEDs.

Hmm. I'm using leds-gpio, and it certainly does seem to care about GPIO numbers. As a point of reference, here's what I'm doing to set up my drive activity LEDs:

#define N5550_ICH_GPIO_BASE 195

#define N5550_DISK_ACT_0_GPIO   (N5550_ICH_GPIO_BASE + 0)
#define N5550_DISK_ACT_1_GPIO   (N5550_ICH_GPIO_BASE + 2)
#define N5550_DISK_ACT_2_GPIO   (N5550_ICH_GPIO_BASE + 3)
#define N5550_DISK_ACT_3_GPIO   (N5550_ICH_GPIO_BASE + 4)
#define N5550_DISK_ACT_4_GPIO   (N5550_ICH_GPIO_BASE + 5)

static struct gpio_led n5550_disk_act_leds[5] = {
{
.name = "n5550:green:disk-act-0",
.default_trigger = "thecus-ahci-0",
.gpio = N5550_DISK_ACT_0_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-1",
.default_trigger = "thecus-ahci-1",
.gpio = N5550_DISK_ACT_1_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-2",
.default_trigger = "thecus-ahci-2",
.gpio = N5550_DISK_ACT_2_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-3",
.default_trigger = "thecus-ahci-3",
.gpio = N5550_DISK_ACT_3_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-4",
.default_trigger = "thecus-ahci-4",
.gpio = N5550_DISK_ACT_4_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
};

As you can see, every LED has a GPIO number (.gpio), which comes from the chip's GPIO base, and that number gets hardcoded into the setup code. If the ich_gpio driver ever "dynamically" selects a base other than 195, the LEDs won't work.

I just seems very fragile, and I can't help thinking that there must be a better way. Maybe a module parameter ...

Thanks!

Signature · 4 lines
========================================================================
Ian Pilcher                                         [email protected]
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================
#3 lore raw
Quoted 54 lines
Quoted 3 lines

From the viewpoint of a GPIO consumer, we don't care the base GPIO number. That is required when a GPIO controller is added.
Just check '/sys/kernel/debug/gpio' and use available GPIOs for LEDs.

Hmm. I'm using leds-gpio, and it certainly does seem to care about GPIO numbers. As a point of reference, here's what I'm doing to set up my drive activity LEDs:

#define N5550_ICH_GPIO_BASE 195

#define N5550_DISK_ACT_0_GPIO   (N5550_ICH_GPIO_BASE + 0)
#define N5550_DISK_ACT_1_GPIO   (N5550_ICH_GPIO_BASE + 2)
#define N5550_DISK_ACT_2_GPIO   (N5550_ICH_GPIO_BASE + 3)
#define N5550_DISK_ACT_3_GPIO   (N5550_ICH_GPIO_BASE + 4)
#define N5550_DISK_ACT_4_GPIO   (N5550_ICH_GPIO_BASE + 5)

static struct gpio_led n5550_disk_act_leds[5] = {
{
.name = "n5550:green:disk-act-0",
.default_trigger = "thecus-ahci-0",
.gpio = N5550_DISK_ACT_0_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-1",
.default_trigger = "thecus-ahci-1",
.gpio = N5550_DISK_ACT_1_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-2",
.default_trigger = "thecus-ahci-2",
.gpio = N5550_DISK_ACT_2_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-3",
.default_trigger = "thecus-ahci-3",
.gpio = N5550_DISK_ACT_3_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
{
.name = "n5550:green:disk-act-4",
.default_trigger = "thecus-ahci-4",
.gpio = N5550_DISK_ACT_4_GPIO,
.active_low = 1,
.default_state = LEDS_GPIO_DEFSTATE_OFF,
},
};

Quoted 4 lines

As you can see, every LED has a GPIO number (.gpio), which comes from the chip's GPIO base, and that number gets hardcoded into the setup code. If the ich_gpio driver ever "dynamically" selects a base other than 195, the LEDs won't work.

It sounds you want to make the LED platform data flexible, right?

Quoted 2 lines

I just seems very fragile, and I can't help thinking that there must be a better way. Maybe a module parameter ...

If you can use .dtb file in your system, it may be possible to use just offset values with GPIO controller, ich_gpio. Then, no hardcoded GPIO is required.

For example, leds-gpio can be created with the DT below.

leds {
	compatible = "gpio-leds";
	hdd0 {
		label = " n5550:green:disk-act-0";
		gpios = <&ich_gpio 0 1>; /* offset 0, active low */
		linux,default-trigger = " thecus-ahci-0";
		default-state = "off";
	};

	.
	.
	.

	hdd4 {
		label = " n5550:green:disk-act-4";
		gpios = <&ich_gpio 4 1>; /* offset 4, active low */
		linux,default-trigger = " thecus-ahci-4";
		default-state = "off";
	};
};

Then, we don't care the base number of the ich_gpio controller.

However, ich_gpio driver should support the GPIO DT structure. I've not checked it yet.

Best regards,
Milo

#4 lore raw
Quoted 1 line · Milo

It sounds you want to make the LED platform data flexible, right?

Yes, but only to the extent that it can handle this particular hardware. I'm mostly concerned that something like a kernel upgrade, BIOS upgrade, or loading additional modules will cause the "magic" GPIO numbers to change.

Quoted 2 lines

If you can use .dtb file in your system, it may be possible to use just offset values with GPIO controller, ich_gpio. Then, no hardcoded GPIO is required.

I'm fairly sure that device tree is not an option in most x86_64 distros. (And the whole reason I'm doing this is to used such a "mainstream" distro on this hardware, rather than the NAS vendor's "firmware".)

For now, I think I'll go the kernel module route. If necessary, user- space can use sysfs to find the GPIO base of the ICH and pass it in. I suspect there's a way to do this entirely in the kernel, but I also suspect that it's too much trouble to be worth the effort right now.

Thanks!

Signature · 4 lines
========================================================================
Ian Pilcher                                         [email protected]
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================