Registering GPIO LEDs

Registering GPIO LEDs

[Ian Pilcher]

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!

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================

RE: Registering GPIO LEDs

[Kim, Milo]

Hi Ian,

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

>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

Re: Registering GPIO LEDs

[Ian Pilcher]

On 08/08/2013 09:08 PM, Kim, Milo wrote:
> 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!

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================

RE: Registering GPIO LEDs

[Kim, 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.

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

> 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

Re: Registering GPIO LEDs

[Ian Pilcher]

On 08/09/2013 12:26 AM, Kim, Milo wrote:
> 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.

> 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!

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
Sometimes there's nothing left to do but crash and burn...or die trying.
========================================================================