Em Seg, 2008-09-15 às 13:18 +0200, Antonio Ospite escreveu:
> Hi Richard, hi openezx friends,
>
> this is a proposal for the National Semiconductor LP3944[1] leds driver
> (aka Funlight) chip, found in Motorola A910. The driver is generic,
> modeled after leds-pca9532, but I still have some doubts about how to
> handle wiring setups.
> Now the doubts I have:
> Some leds can be wired with inverted logic (leds OFF for LP3944
> means that the light is ON), like in A910 LCD backlights leds, and
> +enum lp3944_type {
> + LP3944_LED_TYPE_NONE,
> + LP3944_LED_TYPE_LED,
> + /*
> + LP3944_LED_TYPE_LED_INVERTED
> + */
> +};
Uncomment this, and go ahead with the inverted on/off logic on kernel
space.
> sometimes a led needs some pre-condition to be actually ON:
> * A910 has two LCD displays whose backlights are attached to
> LP3944; well, when the main display is ON, the secondary one
> should be turned OFF, and vice-versa.
> * A910 has two flashlight modes, the flash led can be in a low
> light mode, or in a flash mode. Well, two LP3944 leds (LED6 and
> LED7) are attached to the same physical flashlight diode,
> controlling its state according to the following scheme:
>
> LED6 OFF + LED7 OFF -> lights off
> LED6 OFF + LED7 ON -> strong light that turns off after ~1sec
> LED6 ON + LED7 OFF -> lights off (useless)
> LED6 ON + LED7 ON -> low light mode
This can be handled on userspace.
> The question is, should we entrust such wiring-dependent logic to
> user-space? To handle that in kernel space I could set also
> ledX_set_brightness() callbacks via platform data, it shouldn't be too
> difficult.
I dont think that _set_brightness via platform_data is a good idea, we
would end with a lot of duplicate code.
> And, should I use backlight-class devices for backlight driving leds?
Good idea. Add a .backlights array to the platform data. :)
> +static struct lp3944_platform_data a910_lp3944_leds = {
> + /* set two default dim modes, a fast one and a slow one.
> + * they will be used when brightness == {2,3}
> + */
> + .dims = {
> + [0] = {
> + .period = 5,
> + .dutycycle = 5,
> + },
> + [1] = {
> + .period = 10,
> + .dutycycle = 50,
> + },
> + },
> + .leds = {
> + [0] = {
> + .name = "a910:red",
> + .status = LP3944_LED_STATUS_OFF,
> + .type = LP3944_LED_TYPE_LED,
> + },
> + [1] = {
> + .name = "a910:green",
> + .status = LP3944_LED_STATUS_OFF,
> + .type = LP3944_LED_TYPE_LED,
> + },
> + [2] {
> + .name = "a910:blue",
> + .status = LP3944_LED_STATUS_OFF,
> + .type = LP3944_LED_TYPE_LED,
> + },
> + /* Leds 3 and 4 are displays backlights */
> + [3] = {
> + .name = "a910:cli_backlight",
> + .status = LP3944_LED_STATUS_ON,
> + .type = LP3944_LED_TYPE_LED
> + },
> + [4] = {
> + .name = "a910:main_backlight",
> + .status = LP3944_LED_STATUS_ON,
> + .type = LP3944_LED_TYPE_LED
> + },
> + [5] = { .type = LP3944_LED_TYPE_NONE },
> + [6] = {
> + .name = "a910:torch",
> + .status = LP3944_LED_STATUS_OFF,
> + .type = LP3944_LED_TYPE_LED,
> + },
> + [7] = {
> + .name = "a910:flash",
> + .status = LP3944_LED_STATUS_OFF,
> + .type = LP3944_LED_TYPE_LED,
> + },
> + },
> +};
Add .dims_size/.leds_size fields for these arrays...
> + for (i = 0; i < 2; i++) {
> + lp3944_dim_set_period(client, i,
> pdata->dims[i].period);
> + lp3944_dim_set_dutycycle(client, i,
> pdata->dims[i].dutycycle);
> + }
> +
> + for (i = 0; i < 8; i++) {
And use it here instead of relying on the fixed array size. Also, make
the .dims array optional and pass on the led wiring number (*_LEDx) on
the .leds fields.
> +static void lp3944_set_brightness(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct lp3944_led *led = ldev_to_led(led_cdev);
> +
> + /*
> + * Meaning of the value parameter:
> + * 0 = led OFF
> + * 1 = led ON
> + * 2 = led in DIM0 mode
> + * 3 = led in DIM1 mode
> + */
> + if (value > 3)
> + value = 3;
> + pr_debug(KERN_DEBUG "%s: %d\n", led_cdev->name, value);
> + lp3944_led_set(led, value);
> +}
You probably shouldn't abuse "brightness" to set dim modes, but if you
really want to do it, the "1 = led ON" should be the default for
(value>3).
--
Daniel Ribeiro
