* [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2010-12-09 13:41 ` Janusz Krzysztofik
0 siblings, 0 replies; 16+ messages in thread
From: Janusz Krzysztofik @ 2010-12-09 13:41 UTC (permalink / raw)
To: Richard Purdie
Cc: linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org
This patch extends the LED backlight tirgger driver with an option that allows
for inverting the trigger output polarity.
With the invertion option provided, I (ab)use the backlight trigger for
driving a LED that indicates LCD display blank condtition on my Amstrad Delta
videophone. Since the machine has no dedicated power LED, it was not possible
to distinguish if the display was blanked, or the machine was turned off,
without touching it.
The invert sysfs control is patterned after a similiar function of the GPIO
trigger driver.
Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
Retested on linux-2.6.37-rc4.
Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
Cc: Richard Purdie <rpurdie@rpsys.net>
---
Resent because I still can't see any response received, while yet another
merge window is going to pass away soon.
Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
tried to clean up the commit message slightly - maybe my English is not good
enough to bother with, if not the code?
v1 -> v2 changes:
- improve some conditional expressions to be more readable; thanks to Ralph
Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
- refresh against linux-2.6.36-rc5.
drivers/leds/ledtrig-backlight.c | 60
++++++++++++++++++++++++++++++++++++---
1 file changed, 56 insertions(+), 4 deletions(-)
diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
--- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
+++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
@@ -26,6 +26,7 @@ struct bl_trig_notifier {
int brightness;
int old_status;
struct notifier_block notifier;
+ unsigned invert;
};
static int fb_notifier_callback(struct notifier_block *p,
@@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
struct led_classdev *led = n->led;
struct fb_event *fb_event = data;
int *blank = fb_event->data;
+ int new_status = *blank ? BLANK : UNBLANK;
switch (event) {
case FB_EVENT_BLANK :
- if (*blank && n->old_status == UNBLANK) {
+ if (new_status == n->old_status)
+ break;
+
+ if ((n->old_status == UNBLANK) ^ n->invert) {
n->brightness = led->brightness;
led_set_brightness(led, LED_OFF);
- n->old_status = BLANK;
- } else if (!*blank && n->old_status == BLANK) {
+ } else {
led_set_brightness(led, n->brightness);
- n->old_status = UNBLANK;
}
+
+ n->old_status = new_status;
+
break;
}
return 0;
}
+static ssize_t bl_trig_invert_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+
+ return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
+}
+
+static ssize_t bl_trig_invert_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t num)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+ unsigned invert;
+ int ret;
+
+ ret = sscanf(buf, "%u", &invert);
+ if (ret < 1) {
+ dev_err(dev, "invalid value\n");
+ return -EINVAL;
+ }
+
+ n->invert = !!invert;
+
+ /* After inverting, we need to update the LED. */
+ if ((n->old_status == BLANK) ^ n->invert)
+ led_set_brightness(led, LED_OFF);
+ else
+ led_set_brightness(led, n->brightness);
+
+ return num;
+}
+static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
+
static void bl_trig_activate(struct led_classdev *led)
{
int ret;
@@ -66,6 +107,10 @@ static void bl_trig_activate(struct led_
return;
}
+ ret = device_create_file(led->dev, &dev_attr_invert);
+ if (ret)
+ goto err_invert;
+
n->led = led;
n->brightness = led->brightness;
n->old_status = UNBLANK;
@@ -74,6 +119,12 @@ static void bl_trig_activate(struct led_
ret = fb_register_client(&n->notifier);
if (ret)
dev_err(led->dev, "unable to register backlight trigger\n");
+
+ return;
+
+err_invert:
+ led->trigger_data = NULL;
+ kfree(n);
}
static void bl_trig_deactivate(struct led_classdev *led)
@@ -82,6 +133,7 @@ static void bl_trig_deactivate(struct le
(struct bl_trig_notifier *) led->trigger_data;
if (n) {
+ device_remove_file(led->dev, &dev_attr_invert);
fb_unregister_client(&n->notifier);
kfree(n);
}
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
2010-12-09 13:41 ` Janusz Krzysztofik
@ 2011-01-06 7:08 ` Paul Mundt
-1 siblings, 0 replies; 16+ messages in thread
From: Paul Mundt @ 2011-01-06 7:08 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Richard Purdie, Andrew Morton
(Trying an alternate address for Richard, and adding Andrew to Cc..)
On Thu, Dec 09, 2010 at 02:41:50PM +0100, Janusz Krzysztofik wrote:
> This patch extends the LED backlight tirgger driver with an option that allows
> for inverting the trigger output polarity.
>
> With the invertion option provided, I (ab)use the backlight trigger for
> driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> videophone. Since the machine has no dedicated power LED, it was not possible
> to distinguish if the display was blanked, or the machine was turned off,
> without touching it.
>
> The invert sysfs control is patterned after a similiar function of the GPIO
> trigger driver.
>
> Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> Retested on linux-2.6.37-rc4.
>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---
>
> Resent because I still can't see any response received, while yet another
> merge window is going to pass away soon.
>
> Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> tried to clean up the commit message slightly - maybe my English is not good
> enough to bother with, if not the code?
>
> v1 -> v2 changes:
> - improve some conditional expressions to be more readable; thanks to Ralph
> Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> - refresh against linux-2.6.36-rc5.
>
> drivers/leds/ledtrig-backlight.c | 60
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 56 insertions(+), 4 deletions(-)
>
> diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> int brightness;
> int old_status;
> struct notifier_block notifier;
> + unsigned invert;
> };
>
> static int fb_notifier_callback(struct notifier_block *p,
> @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> struct led_classdev *led = n->led;
> struct fb_event *fb_event = data;
> int *blank = fb_event->data;
> + int new_status = *blank ? BLANK : UNBLANK;
>
> switch (event) {
> case FB_EVENT_BLANK :
> - if (*blank && n->old_status = UNBLANK) {
> + if (new_status = n->old_status)
> + break;
> +
> + if ((n->old_status = UNBLANK) ^ n->invert) {
> n->brightness = led->brightness;
> led_set_brightness(led, LED_OFF);
> - n->old_status = BLANK;
> - } else if (!*blank && n->old_status = BLANK) {
> + } else {
> led_set_brightness(led, n->brightness);
> - n->old_status = UNBLANK;
> }
> +
> + n->old_status = new_status;
> +
> break;
> }
>
> return 0;
> }
>
> +static ssize_t bl_trig_invert_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> +
> + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> +}
> +
> +static ssize_t bl_trig_invert_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t num)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> + unsigned invert;
> + int ret;
> +
> + ret = sscanf(buf, "%u", &invert);
> + if (ret < 1) {
> + dev_err(dev, "invalid value\n");
> + return -EINVAL;
> + }
> +
> + n->invert = !!invert;
> +
> + /* After inverting, we need to update the LED. */
> + if ((n->old_status = BLANK) ^ n->invert)
> + led_set_brightness(led, LED_OFF);
> + else
> + led_set_brightness(led, n->brightness);
> +
> + return num;
> +}
> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> +
> static void bl_trig_activate(struct led_classdev *led)
> {
> int ret;
> @@ -66,6 +107,10 @@ static void bl_trig_activate(struct led_
> return;
> }
>
> + ret = device_create_file(led->dev, &dev_attr_invert);
> + if (ret)
> + goto err_invert;
> +
> n->led = led;
> n->brightness = led->brightness;
> n->old_status = UNBLANK;
> @@ -74,6 +119,12 @@ static void bl_trig_activate(struct led_
> ret = fb_register_client(&n->notifier);
> if (ret)
> dev_err(led->dev, "unable to register backlight trigger\n");
> +
> + return;
> +
> +err_invert:
> + led->trigger_data = NULL;
> + kfree(n);
> }
>
> static void bl_trig_deactivate(struct led_classdev *led)
> @@ -82,6 +133,7 @@ static void bl_trig_deactivate(struct le
> (struct bl_trig_notifier *) led->trigger_data;
>
> if (n) {
> + device_remove_file(led->dev, &dev_attr_invert);
> fb_unregister_client(&n->notifier);
> kfree(n);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 7:08 ` Paul Mundt
0 siblings, 0 replies; 16+ messages in thread
From: Paul Mundt @ 2011-01-06 7:08 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Richard Purdie, Andrew Morton
(Trying an alternate address for Richard, and adding Andrew to Cc..)
On Thu, Dec 09, 2010 at 02:41:50PM +0100, Janusz Krzysztofik wrote:
> This patch extends the LED backlight tirgger driver with an option that allows
> for inverting the trigger output polarity.
>
> With the invertion option provided, I (ab)use the backlight trigger for
> driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> videophone. Since the machine has no dedicated power LED, it was not possible
> to distinguish if the display was blanked, or the machine was turned off,
> without touching it.
>
> The invert sysfs control is patterned after a similiar function of the GPIO
> trigger driver.
>
> Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> Retested on linux-2.6.37-rc4.
>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---
>
> Resent because I still can't see any response received, while yet another
> merge window is going to pass away soon.
>
> Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> tried to clean up the commit message slightly - maybe my English is not good
> enough to bother with, if not the code?
>
> v1 -> v2 changes:
> - improve some conditional expressions to be more readable; thanks to Ralph
> Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> - refresh against linux-2.6.36-rc5.
>
> drivers/leds/ledtrig-backlight.c | 60
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 56 insertions(+), 4 deletions(-)
>
> diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> int brightness;
> int old_status;
> struct notifier_block notifier;
> + unsigned invert;
> };
>
> static int fb_notifier_callback(struct notifier_block *p,
> @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> struct led_classdev *led = n->led;
> struct fb_event *fb_event = data;
> int *blank = fb_event->data;
> + int new_status = *blank ? BLANK : UNBLANK;
>
> switch (event) {
> case FB_EVENT_BLANK :
> - if (*blank && n->old_status == UNBLANK) {
> + if (new_status == n->old_status)
> + break;
> +
> + if ((n->old_status == UNBLANK) ^ n->invert) {
> n->brightness = led->brightness;
> led_set_brightness(led, LED_OFF);
> - n->old_status = BLANK;
> - } else if (!*blank && n->old_status == BLANK) {
> + } else {
> led_set_brightness(led, n->brightness);
> - n->old_status = UNBLANK;
> }
> +
> + n->old_status = new_status;
> +
> break;
> }
>
> return 0;
> }
>
> +static ssize_t bl_trig_invert_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> +
> + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> +}
> +
> +static ssize_t bl_trig_invert_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t num)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> + unsigned invert;
> + int ret;
> +
> + ret = sscanf(buf, "%u", &invert);
> + if (ret < 1) {
> + dev_err(dev, "invalid value\n");
> + return -EINVAL;
> + }
> +
> + n->invert = !!invert;
> +
> + /* After inverting, we need to update the LED. */
> + if ((n->old_status == BLANK) ^ n->invert)
> + led_set_brightness(led, LED_OFF);
> + else
> + led_set_brightness(led, n->brightness);
> +
> + return num;
> +}
> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> +
> static void bl_trig_activate(struct led_classdev *led)
> {
> int ret;
> @@ -66,6 +107,10 @@ static void bl_trig_activate(struct led_
> return;
> }
>
> + ret = device_create_file(led->dev, &dev_attr_invert);
> + if (ret)
> + goto err_invert;
> +
> n->led = led;
> n->brightness = led->brightness;
> n->old_status = UNBLANK;
> @@ -74,6 +119,12 @@ static void bl_trig_activate(struct led_
> ret = fb_register_client(&n->notifier);
> if (ret)
> dev_err(led->dev, "unable to register backlight trigger\n");
> +
> + return;
> +
> +err_invert:
> + led->trigger_data = NULL;
> + kfree(n);
> }
>
> static void bl_trig_deactivate(struct led_classdev *led)
> @@ -82,6 +133,7 @@ static void bl_trig_deactivate(struct le
> (struct bl_trig_notifier *) led->trigger_data;
>
> if (n) {
> + device_remove_file(led->dev, &dev_attr_invert);
> fb_unregister_client(&n->notifier);
> kfree(n);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to
2011-01-06 7:08 ` Paul Mundt
@ 2011-01-06 12:02 ` Richard Purdie
-1 siblings, 0 replies; 16+ messages in thread
From: Richard Purdie @ 2011-01-06 12:02 UTC (permalink / raw)
To: Paul Mundt, Andrew Morton
Cc: Janusz Krzysztofik, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org
On Thu, 2011-01-06 at 16:08 +0900, Paul Mundt wrote:
> (Trying an alternate address for Richard, and adding Andrew to Cc..)
>
> On Thu, Dec 09, 2010 at 02:41:50PM +0100, Janusz Krzysztofik wrote:
> > This patch extends the LED backlight tirgger driver with an option that allows
> > for inverting the trigger output polarity.
> >
> > With the invertion option provided, I (ab)use the backlight trigger for
> > driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> > videophone. Since the machine has no dedicated power LED, it was not possible
> > to distinguish if the display was blanked, or the machine was turned off,
> > without touching it.
> >
> > The invert sysfs control is patterned after a similiar function of the GPIO
> > trigger driver.
> >
> > Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> > Retested on linux-2.6.37-rc4.
> >
> > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> > Cc: Richard Purdie <rpurdie@rpsys.net>
Acked-by: Richard Purdie <richard.purdie@linuxfoundation.org>
I'll let Andrew take the patch though.
> > ---
> >
> > Resent because I still can't see any response received, while yet another
> > merge window is going to pass away soon.
> >
> > Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> > tried to clean up the commit message slightly - maybe my English is not good
> > enough to bother with, if not the code?
> >
> > v1 -> v2 changes:
> > - improve some conditional expressions to be more readable; thanks to Ralph
> > Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> > - refresh against linux-2.6.36-rc5.
> >
> > drivers/leds/ledtrig-backlight.c | 60
> > ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 56 insertions(+), 4 deletions(-)
> >
> > diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> > --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> > +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> > @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> > int brightness;
> > int old_status;
> > struct notifier_block notifier;
> > + unsigned invert;
> > };
> >
> > static int fb_notifier_callback(struct notifier_block *p,
> > @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> > struct led_classdev *led = n->led;
> > struct fb_event *fb_event = data;
> > int *blank = fb_event->data;
> > + int new_status = *blank ? BLANK : UNBLANK;
> >
> > switch (event) {
> > case FB_EVENT_BLANK :
> > - if (*blank && n->old_status = UNBLANK) {
> > + if (new_status = n->old_status)
> > + break;
> > +
> > + if ((n->old_status = UNBLANK) ^ n->invert) {
> > n->brightness = led->brightness;
> > led_set_brightness(led, LED_OFF);
> > - n->old_status = BLANK;
> > - } else if (!*blank && n->old_status = BLANK) {
> > + } else {
> > led_set_brightness(led, n->brightness);
> > - n->old_status = UNBLANK;
> > }
> > +
> > + n->old_status = new_status;
> > +
> > break;
> > }
> >
> > return 0;
> > }
> >
> > +static ssize_t bl_trig_invert_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct led_classdev *led = dev_get_drvdata(dev);
> > + struct bl_trig_notifier *n = led->trigger_data;
> > +
> > + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> > +}
> > +
> > +static ssize_t bl_trig_invert_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t num)
> > +{
> > + struct led_classdev *led = dev_get_drvdata(dev);
> > + struct bl_trig_notifier *n = led->trigger_data;
> > + unsigned invert;
> > + int ret;
> > +
> > + ret = sscanf(buf, "%u", &invert);
> > + if (ret < 1) {
> > + dev_err(dev, "invalid value\n");
> > + return -EINVAL;
> > + }
> > +
> > + n->invert = !!invert;
> > +
> > + /* After inverting, we need to update the LED. */
> > + if ((n->old_status = BLANK) ^ n->invert)
> > + led_set_brightness(led, LED_OFF);
> > + else
> > + led_set_brightness(led, n->brightness);
> > +
> > + return num;
> > +}
> > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> > +
> > static void bl_trig_activate(struct led_classdev *led)
> > {
> > int ret;
> > @@ -66,6 +107,10 @@ static void bl_trig_activate(struct led_
> > return;
> > }
> >
> > + ret = device_create_file(led->dev, &dev_attr_invert);
> > + if (ret)
> > + goto err_invert;
> > +
> > n->led = led;
> > n->brightness = led->brightness;
> > n->old_status = UNBLANK;
> > @@ -74,6 +119,12 @@ static void bl_trig_activate(struct led_
> > ret = fb_register_client(&n->notifier);
> > if (ret)
> > dev_err(led->dev, "unable to register backlight trigger\n");
> > +
> > + return;
> > +
> > +err_invert:
> > + led->trigger_data = NULL;
> > + kfree(n);
> > }
> >
> > static void bl_trig_deactivate(struct led_classdev *led)
> > @@ -82,6 +133,7 @@ static void bl_trig_deactivate(struct le
> > (struct bl_trig_notifier *) led->trigger_data;
> >
> > if (n) {
> > + device_remove_file(led->dev, &dev_attr_invert);
> > fb_unregister_client(&n->notifier);
> > kfree(n);
> > }
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 12:02 ` Richard Purdie
0 siblings, 0 replies; 16+ messages in thread
From: Richard Purdie @ 2011-01-06 12:02 UTC (permalink / raw)
To: Paul Mundt, Andrew Morton
Cc: Janusz Krzysztofik, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org
On Thu, 2011-01-06 at 16:08 +0900, Paul Mundt wrote:
> (Trying an alternate address for Richard, and adding Andrew to Cc..)
>
> On Thu, Dec 09, 2010 at 02:41:50PM +0100, Janusz Krzysztofik wrote:
> > This patch extends the LED backlight tirgger driver with an option that allows
> > for inverting the trigger output polarity.
> >
> > With the invertion option provided, I (ab)use the backlight trigger for
> > driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> > videophone. Since the machine has no dedicated power LED, it was not possible
> > to distinguish if the display was blanked, or the machine was turned off,
> > without touching it.
> >
> > The invert sysfs control is patterned after a similiar function of the GPIO
> > trigger driver.
> >
> > Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> > Retested on linux-2.6.37-rc4.
> >
> > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> > Cc: Richard Purdie <rpurdie@rpsys.net>
Acked-by: Richard Purdie <richard.purdie@linuxfoundation.org>
I'll let Andrew take the patch though.
> > ---
> >
> > Resent because I still can't see any response received, while yet another
> > merge window is going to pass away soon.
> >
> > Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> > tried to clean up the commit message slightly - maybe my English is not good
> > enough to bother with, if not the code?
> >
> > v1 -> v2 changes:
> > - improve some conditional expressions to be more readable; thanks to Ralph
> > Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> > - refresh against linux-2.6.36-rc5.
> >
> > drivers/leds/ledtrig-backlight.c | 60
> > ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 56 insertions(+), 4 deletions(-)
> >
> > diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> > --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> > +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> > @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> > int brightness;
> > int old_status;
> > struct notifier_block notifier;
> > + unsigned invert;
> > };
> >
> > static int fb_notifier_callback(struct notifier_block *p,
> > @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> > struct led_classdev *led = n->led;
> > struct fb_event *fb_event = data;
> > int *blank = fb_event->data;
> > + int new_status = *blank ? BLANK : UNBLANK;
> >
> > switch (event) {
> > case FB_EVENT_BLANK :
> > - if (*blank && n->old_status == UNBLANK) {
> > + if (new_status == n->old_status)
> > + break;
> > +
> > + if ((n->old_status == UNBLANK) ^ n->invert) {
> > n->brightness = led->brightness;
> > led_set_brightness(led, LED_OFF);
> > - n->old_status = BLANK;
> > - } else if (!*blank && n->old_status == BLANK) {
> > + } else {
> > led_set_brightness(led, n->brightness);
> > - n->old_status = UNBLANK;
> > }
> > +
> > + n->old_status = new_status;
> > +
> > break;
> > }
> >
> > return 0;
> > }
> >
> > +static ssize_t bl_trig_invert_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct led_classdev *led = dev_get_drvdata(dev);
> > + struct bl_trig_notifier *n = led->trigger_data;
> > +
> > + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> > +}
> > +
> > +static ssize_t bl_trig_invert_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t num)
> > +{
> > + struct led_classdev *led = dev_get_drvdata(dev);
> > + struct bl_trig_notifier *n = led->trigger_data;
> > + unsigned invert;
> > + int ret;
> > +
> > + ret = sscanf(buf, "%u", &invert);
> > + if (ret < 1) {
> > + dev_err(dev, "invalid value\n");
> > + return -EINVAL;
> > + }
> > +
> > + n->invert = !!invert;
> > +
> > + /* After inverting, we need to update the LED. */
> > + if ((n->old_status == BLANK) ^ n->invert)
> > + led_set_brightness(led, LED_OFF);
> > + else
> > + led_set_brightness(led, n->brightness);
> > +
> > + return num;
> > +}
> > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> > +
> > static void bl_trig_activate(struct led_classdev *led)
> > {
> > int ret;
> > @@ -66,6 +107,10 @@ static void bl_trig_activate(struct led_
> > return;
> > }
> >
> > + ret = device_create_file(led->dev, &dev_attr_invert);
> > + if (ret)
> > + goto err_invert;
> > +
> > n->led = led;
> > n->brightness = led->brightness;
> > n->old_status = UNBLANK;
> > @@ -74,6 +119,12 @@ static void bl_trig_activate(struct led_
> > ret = fb_register_client(&n->notifier);
> > if (ret)
> > dev_err(led->dev, "unable to register backlight trigger\n");
> > +
> > + return;
> > +
> > +err_invert:
> > + led->trigger_data = NULL;
> > + kfree(n);
> > }
> >
> > static void bl_trig_deactivate(struct led_classdev *led)
> > @@ -82,6 +133,7 @@ static void bl_trig_deactivate(struct le
> > (struct bl_trig_notifier *) led->trigger_data;
> >
> > if (n) {
> > + device_remove_file(led->dev, &dev_attr_invert);
> > fb_unregister_client(&n->notifier);
> > kfree(n);
> > }
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to
2010-12-09 13:41 ` Janusz Krzysztofik
@ 2011-01-06 21:04 ` Andrew Morton
-1 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2011-01-06 21:04 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Paul Mundt
On Thu, 9 Dec 2010 14:41:50 +0100
Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
> This patch extends the LED backlight tirgger driver with an option that allows
> for inverting the trigger output polarity.
>
> With the invertion option provided, I (ab)use the backlight trigger for
> driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> videophone. Since the machine has no dedicated power LED, it was not possible
> to distinguish if the display was blanked, or the machine was turned off,
> without touching it.
>
> The invert sysfs control is patterned after a similiar function of the GPIO
> trigger driver.
>
> Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> Retested on linux-2.6.37-rc4.
>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---
>
> Resent because I still can't see any response received, while yet another
> merge window is going to pass away soon.
>
> Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> tried to clean up the commit message slightly - maybe my English is not good
> enough to bother with, if not the code?
>
> v1 -> v2 changes:
> - improve some conditional expressions to be more readable; thanks to Ralph
> Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> - refresh against linux-2.6.36-rc5.
>
> drivers/leds/ledtrig-backlight.c | 60
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 56 insertions(+), 4 deletions(-)
>
> diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> int brightness;
> int old_status;
> struct notifier_block notifier;
> + unsigned invert;
> };
>
> static int fb_notifier_callback(struct notifier_block *p,
> @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> struct led_classdev *led = n->led;
> struct fb_event *fb_event = data;
> int *blank = fb_event->data;
> + int new_status = *blank ? BLANK : UNBLANK;
>
> switch (event) {
> case FB_EVENT_BLANK :
> - if (*blank && n->old_status = UNBLANK) {
> + if (new_status = n->old_status)
> + break;
> +
> + if ((n->old_status = UNBLANK) ^ n->invert) {
> n->brightness = led->brightness;
> led_set_brightness(led, LED_OFF);
> - n->old_status = BLANK;
> - } else if (!*blank && n->old_status = BLANK) {
> + } else {
> led_set_brightness(led, n->brightness);
> - n->old_status = UNBLANK;
> }
> +
> + n->old_status = new_status;
> +
> break;
> }
>
> return 0;
> }
>
> +static ssize_t bl_trig_invert_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> +
> + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> +}
I think this should show "0" or "1", to match the thing which the user
wrote here.
> +static ssize_t bl_trig_invert_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t num)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> + unsigned invert;
> + int ret;
> +
> + ret = sscanf(buf, "%u", &invert);
Here we should use strict_strtoul() so the kernel correctly rejects
input of the form "42foo".
> + if (ret < 1) {
> + dev_err(dev, "invalid value\n");
> + return -EINVAL;
> + }
And here it would be better to disallow any input other than 0 or 1.
Because "2" makes no sense and who knows, some time in the future we
might *want* to permit 2.
So...
--- a/drivers/leds/ledtrig-backlight.c~leds-add-output-inversion-option-to-backlight-trigger-fix
+++ a/drivers/leds/ledtrig-backlight.c
@@ -65,7 +65,7 @@ static ssize_t bl_trig_invert_show(struc
struct led_classdev *led = dev_get_drvdata(dev);
struct bl_trig_notifier *n = led->trigger_data;
- return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
+ return sprintf(buf, "%u\n", n->invert);
}
static ssize_t bl_trig_invert_store(struct device *dev,
@@ -73,16 +73,17 @@ static ssize_t bl_trig_invert_store(stru
{
struct led_classdev *led = dev_get_drvdata(dev);
struct bl_trig_notifier *n = led->trigger_data;
- unsigned invert;
+ unsigned long invert;
int ret;
- ret = sscanf(buf, "%u", &invert);
- if (ret < 1) {
- dev_err(dev, "invalid value\n");
+ ret = strict_strtoul(buf, 10, &invert);
+ if (ret < 0)
+ return ret;
+
+ if (invert > 1)
return -EINVAL;
- }
- n->invert = !!invert;
+ n->invert = invert;
/* After inverting, we need to update the LED. */
if ((n->old_status = BLANK) ^ n->invert)
_
Could you help test it please?
> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
This new sysfs file should be documented. Where would be an
appropriate place for that? Documentation/leds-class.txt doesn't
mention a sysfs API at all.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 21:04 ` Andrew Morton
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2011-01-06 21:04 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Paul Mundt
On Thu, 9 Dec 2010 14:41:50 +0100
Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
> This patch extends the LED backlight tirgger driver with an option that allows
> for inverting the trigger output polarity.
>
> With the invertion option provided, I (ab)use the backlight trigger for
> driving a LED that indicates LCD display blank condtition on my Amstrad Delta
> videophone. Since the machine has no dedicated power LED, it was not possible
> to distinguish if the display was blanked, or the machine was turned off,
> without touching it.
>
> The invert sysfs control is patterned after a similiar function of the GPIO
> trigger driver.
>
> Created and tested against linux-2.6.36-rc5 on Amstrad Delta.
> Retested on linux-2.6.37-rc4.
>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> ---
>
> Resent because I still can't see any response received, while yet another
> merge window is going to pass away soon.
>
> Applies cleanly on top of 2.6.37-rc4, so no need for yet another refresh. Only
> tried to clean up the commit message slightly - maybe my English is not good
> enough to bother with, if not the code?
>
> v1 -> v2 changes:
> - improve some conditional expressions to be more readable; thanks to Ralph
> Corderoy (from e3-hacking) and Lars-Peter Clausen for their suggestions,
> - refresh against linux-2.6.36-rc5.
>
> drivers/leds/ledtrig-backlight.c | 60
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 56 insertions(+), 4 deletions(-)
>
> diff -upr linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c
> --- linux-2.6.36-rc5.orig/drivers/leds/ledtrig-backlight.c 2010-09-24 15:35:13.000000000 +0200
> +++ linux-2.6.36-rc5/drivers/leds/ledtrig-backlight.c 2010-10-03 15:59:49.000000000 +0200
> @@ -26,6 +26,7 @@ struct bl_trig_notifier {
> int brightness;
> int old_status;
> struct notifier_block notifier;
> + unsigned invert;
> };
>
> static int fb_notifier_callback(struct notifier_block *p,
> @@ -36,23 +37,63 @@ static int fb_notifier_callback(struct n
> struct led_classdev *led = n->led;
> struct fb_event *fb_event = data;
> int *blank = fb_event->data;
> + int new_status = *blank ? BLANK : UNBLANK;
>
> switch (event) {
> case FB_EVENT_BLANK :
> - if (*blank && n->old_status == UNBLANK) {
> + if (new_status == n->old_status)
> + break;
> +
> + if ((n->old_status == UNBLANK) ^ n->invert) {
> n->brightness = led->brightness;
> led_set_brightness(led, LED_OFF);
> - n->old_status = BLANK;
> - } else if (!*blank && n->old_status == BLANK) {
> + } else {
> led_set_brightness(led, n->brightness);
> - n->old_status = UNBLANK;
> }
> +
> + n->old_status = new_status;
> +
> break;
> }
>
> return 0;
> }
>
> +static ssize_t bl_trig_invert_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> +
> + return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
> +}
I think this should show "0" or "1", to match the thing which the user
wrote here.
> +static ssize_t bl_trig_invert_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t num)
> +{
> + struct led_classdev *led = dev_get_drvdata(dev);
> + struct bl_trig_notifier *n = led->trigger_data;
> + unsigned invert;
> + int ret;
> +
> + ret = sscanf(buf, "%u", &invert);
Here we should use strict_strtoul() so the kernel correctly rejects
input of the form "42foo".
> + if (ret < 1) {
> + dev_err(dev, "invalid value\n");
> + return -EINVAL;
> + }
And here it would be better to disallow any input other than 0 or 1.
Because "2" makes no sense and who knows, some time in the future we
might *want* to permit 2.
So...
--- a/drivers/leds/ledtrig-backlight.c~leds-add-output-inversion-option-to-backlight-trigger-fix
+++ a/drivers/leds/ledtrig-backlight.c
@@ -65,7 +65,7 @@ static ssize_t bl_trig_invert_show(struc
struct led_classdev *led = dev_get_drvdata(dev);
struct bl_trig_notifier *n = led->trigger_data;
- return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
+ return sprintf(buf, "%u\n", n->invert);
}
static ssize_t bl_trig_invert_store(struct device *dev,
@@ -73,16 +73,17 @@ static ssize_t bl_trig_invert_store(stru
{
struct led_classdev *led = dev_get_drvdata(dev);
struct bl_trig_notifier *n = led->trigger_data;
- unsigned invert;
+ unsigned long invert;
int ret;
- ret = sscanf(buf, "%u", &invert);
- if (ret < 1) {
- dev_err(dev, "invalid value\n");
+ ret = strict_strtoul(buf, 10, &invert);
+ if (ret < 0)
+ return ret;
+
+ if (invert > 1)
return -EINVAL;
- }
- n->invert = !!invert;
+ n->invert = invert;
/* After inverting, we need to update the LED. */
if ((n->old_status == BLANK) ^ n->invert)
_
Could you help test it please?
> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
This new sysfs file should be documented. Where would be an
appropriate place for that? Documentation/leds-class.txt doesn't
mention a sysfs API at all.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to
2011-01-06 21:04 ` [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger Andrew Morton
@ 2011-01-06 21:08 ` Randy Dunlap
-1 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2011-01-06 21:08 UTC (permalink / raw)
To: Andrew Morton
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
> On Thu, 9 Dec 2010 14:41:50 +0100
> Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
>
> > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
>
> This new sysfs file should be documented. Where would be an
> appropriate place for that? Documentation/leds-class.txt doesn't
> mention a sysfs API at all.
> --
in Documentation/ABI/, where all sysfs interface info lives.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 21:08 ` Randy Dunlap
0 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2011-01-06 21:08 UTC (permalink / raw)
To: Andrew Morton
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
> On Thu, 9 Dec 2010 14:41:50 +0100
> Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
>
> > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
>
> This new sysfs file should be documented. Where would be an
> appropriate place for that? Documentation/leds-class.txt doesn't
> mention a sysfs API at all.
> --
in Documentation/ABI/, where all sysfs interface info lives.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to
2011-01-06 21:08 ` [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger Randy Dunlap
@ 2011-01-06 21:16 ` Andrew Morton
-1 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2011-01-06 21:16 UTC (permalink / raw)
To: Randy Dunlap
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On Thu, 6 Jan 2011 13:08:56 -0800
Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
>
> > On Thu, 9 Dec 2010 14:41:50 +0100
> > Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
> >
> > > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> >
> > This new sysfs file should be documented. Where would be an
> > appropriate place for that? Documentation/leds-class.txt doesn't
> > mention a sysfs API at all.
> > --
>
> in Documentation/ABI/, where all sysfs interface info lives.
>
Spose so. Documentation/ABI/stable/sysfs-class-backlight does have
some stuff in it.
Personally I tend to regard Documentation/ABI/ as fairly useless
incomprehensible stuff, maintained to keep Greg happy ;) It'd be better
to have a nice little well-maintained document for a subsystem such as
this which actually explains its operation in a useful-to-humans way.
Rather than just mechanically filling out forms.
But a Documentation/ABI update is a heck of a lot better than nothing.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 21:16 ` Andrew Morton
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2011-01-06 21:16 UTC (permalink / raw)
To: Randy Dunlap
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On Thu, 6 Jan 2011 13:08:56 -0800
Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
>
> > On Thu, 9 Dec 2010 14:41:50 +0100
> > Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
> >
> > > +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
> >
> > This new sysfs file should be documented. Where would be an
> > appropriate place for that? Documentation/leds-class.txt doesn't
> > mention a sysfs API at all.
> > --
>
> in Documentation/ABI/, where all sysfs interface info lives.
>
Spose so. Documentation/ABI/stable/sysfs-class-backlight does have
some stuff in it.
Personally I tend to regard Documentation/ABI/ as fairly useless
incomprehensible stuff, maintained to keep Greg happy ;) It'd be better
to have a nice little well-maintained document for a subsystem such as
this which actually explains its operation in a useful-to-humans way.
Rather than just mechanically filling out forms.
But a Documentation/ABI update is a heck of a lot better than nothing.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight
2011-01-06 21:16 ` [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger Andrew Morton
@ 2011-01-06 21:19 ` Randy Dunlap
-1 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2011-01-06 21:19 UTC (permalink / raw)
To: Andrew Morton
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On 01/06/11 13:16, Andrew Morton wrote:
> On Thu, 6 Jan 2011 13:08:56 -0800
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
>> On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
>>
>>> On Thu, 9 Dec 2010 14:41:50 +0100
>>> Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
>>>
>>>> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
>>>
>>> This new sysfs file should be documented. Where would be an
>>> appropriate place for that? Documentation/leds-class.txt doesn't
>>> mention a sysfs API at all.
>>> --
>>
>> in Documentation/ABI/, where all sysfs interface info lives.
>>
>
> Spose so. Documentation/ABI/stable/sysfs-class-backlight does have
> some stuff in it.
>
> Personally I tend to regard Documentation/ABI/ as fairly useless
> incomprehensible stuff, maintained to keep Greg happy ;) It'd be better
> to have a nice little well-maintained document for a subsystem such as
> this which actually explains its operation in a useful-to-humans way.
> Rather than just mechanically filling out forms.
>
> But a Documentation/ABI update is a heck of a lot better than nothing.
Yes, I was giving you the "where it currently lives", not "what is best."
I agree with you.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger
@ 2011-01-06 21:19 ` Randy Dunlap
0 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2011-01-06 21:19 UTC (permalink / raw)
To: Andrew Morton
Cc: Janusz Krzysztofik, Richard Purdie, linux-fbdev@vger.kernel.org,
linux-kernel, linux-omap@vger.kernel.org, Paul Mundt
On 01/06/11 13:16, Andrew Morton wrote:
> On Thu, 6 Jan 2011 13:08:56 -0800
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
>> On Thu, 6 Jan 2011 13:04:40 -0800 Andrew Morton wrote:
>>
>>> On Thu, 9 Dec 2010 14:41:50 +0100
>>> Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
>>>
>>>> +static DEVICE_ATTR(invert, 0644, bl_trig_invert_show, bl_trig_invert_store);
>>>
>>> This new sysfs file should be documented. Where would be an
>>> appropriate place for that? Documentation/leds-class.txt doesn't
>>> mention a sysfs API at all.
>>> --
>>
>> in Documentation/ABI/, where all sysfs interface info lives.
>>
>
> Spose so. Documentation/ABI/stable/sysfs-class-backlight does have
> some stuff in it.
>
> Personally I tend to regard Documentation/ABI/ as fairly useless
> incomprehensible stuff, maintained to keep Greg happy ;) It'd be better
> to have a nice little well-maintained document for a subsystem such as
> this which actually explains its operation in a useful-to-humans way.
> Rather than just mechanically filling out forms.
>
> But a Documentation/ABI update is a heck of a lot better than nothing.
Yes, I was giving you the "where it currently lives", not "what is best."
I agree with you.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] LEDS: Add output invertion option to backlight trigger
2011-01-06 21:04 ` [RESEND #2] [PATCH v2] LEDS: Add output invertion option to backlight trigger Andrew Morton
@ 2011-01-10 4:54 ` Janusz Krzysztofik
-1 siblings, 0 replies; 16+ messages in thread
From: Janusz Krzysztofik @ 2011-01-10 4:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Paul Mundt, Richard Purdie
This patch extends the LED backlight tirgger driver with an option that
allows for inverting the trigger output polarity.
With the invertion option provided, I (ab)use the backlight trigger for
driving a LED that indicates LCD display blank condtition on my Amstrad
Delta videophone. Since the machine has no dedicated power LED, it was
not possible to distinguish if the display was blanked, or the machine
was turned off, without touching it.
The invert sysfs control is patterned after a similiar function of the
GPIO trigger driver.
Created and tested against linux-2.6.37 on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
Cc: Richard Purdie <rpurdie@rpsys.net>
---
v2 -> v3 changes, all provided, requested or inspired by Andrew Morton
(thanks!):
- sysfs file should show "0" or "1" to match the thing which the user
wrote there,
- use strict_strtoul() so the kernel correctly rejects non-numerical
input,
- disallow any input other than 0 or 1,
- new sysfs file should be documented,
- the new sysfs file name could be consistent with the one already used
by the gpio trigger for a similiar function.
v1 -> v2 changes:
- improve some conditional expressions to be more readable; thanks to
Ralph Corderoy (from e3-hacking) and Lars-Peter Clausen for their
suggestions,
- refresh against linux-2.6.36-rc5.
Documentation/ABI/testing/sysfs-class-led | 10 ++++
drivers/leds/ledtrig-backlight.c | 61 ++++++++++++++++++++++++++++--
2 files changed, 67 insertions(+), 4 deletions(-)
--- linux-2.6.37/drivers/leds/ledtrig-backlight.c.orig 2011-01-10 02:55:26.000000000 +0100
+++ linux-2.6.37/drivers/leds/ledtrig-backlight.c 2011-01-10 03:12:46.000000000 +0100
@@ -26,6 +26,7 @@ struct bl_trig_notifier {
int brightness;
int old_status;
struct notifier_block notifier;
+ unsigned invert;
};
static int fb_notifier_callback(struct notifier_block *p,
@@ -36,23 +37,64 @@ static int fb_notifier_callback(struct n
struct led_classdev *led = n->led;
struct fb_event *fb_event = data;
int *blank = fb_event->data;
+ int new_status = *blank ? BLANK : UNBLANK;
switch (event) {
case FB_EVENT_BLANK :
- if (*blank && n->old_status = UNBLANK) {
+ if (new_status = n->old_status)
+ break;
+
+ if ((n->old_status = UNBLANK) ^ n->invert) {
n->brightness = led->brightness;
led_set_brightness(led, LED_OFF);
- n->old_status = BLANK;
- } else if (!*blank && n->old_status = BLANK) {
+ } else {
led_set_brightness(led, n->brightness);
- n->old_status = UNBLANK;
}
+
+ n->old_status = new_status;
+
break;
}
return 0;
}
+static ssize_t bl_trig_invert_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+
+ return sprintf(buf, "%u\n", n->invert);
+}
+
+static ssize_t bl_trig_invert_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t num)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+ unsigned long invert;
+ int ret;
+
+ ret = strict_strtoul(buf, 10, &invert);
+ if (ret < 0)
+ return ret;
+
+ if (invert > 1)
+ return -EINVAL;
+
+ n->invert = invert;
+
+ /* After inverting, we need to update the LED. */
+ if ((n->old_status = BLANK) ^ n->invert)
+ led_set_brightness(led, LED_OFF);
+ else
+ led_set_brightness(led, n->brightness);
+
+ return num;
+}
+static DEVICE_ATTR(inverted, 0644, bl_trig_invert_show, bl_trig_invert_store);
+
static void bl_trig_activate(struct led_classdev *led)
{
int ret;
@@ -66,6 +108,10 @@ static void bl_trig_activate(struct led_
return;
}
+ ret = device_create_file(led->dev, &dev_attr_inverted);
+ if (ret)
+ goto err_invert;
+
n->led = led;
n->brightness = led->brightness;
n->old_status = UNBLANK;
@@ -74,6 +120,12 @@ static void bl_trig_activate(struct led_
ret = fb_register_client(&n->notifier);
if (ret)
dev_err(led->dev, "unable to register backlight trigger\n");
+
+ return;
+
+err_invert:
+ led->trigger_data = NULL;
+ kfree(n);
}
static void bl_trig_deactivate(struct led_classdev *led)
@@ -82,6 +134,7 @@ static void bl_trig_deactivate(struct le
(struct bl_trig_notifier *) led->trigger_data;
if (n) {
+ device_remove_file(led->dev, &dev_attr_inverted);
fb_unregister_client(&n->notifier);
kfree(n);
}
--- linux-2.6.37/Documentation/ABI/testing/sysfs-class-led.orig 2011-01-10 02:42:16.000000000 +0100
+++ linux-2.6.37/Documentation/ABI/testing/sysfs-class-led 2011-01-10 03:38:28.000000000 +0100
@@ -26,3 +26,13 @@ Description:
scheduler is chosen. Trigger specific parameters can appear in
/sys/class/leds/<led> once a given trigger is selected.
+What: /sys/class/leds/<led>/inverted
+Date: January 2011
+KernelVersion: 2.6.38
+Contact: Richard Purdie <rpurdie@rpsys.net>
+Description:
+ Invert the LED on/off state. This parameter is specific to
+ gpio and backlight triggers. In case of the backlight trigger,
+ it is usefull when driving a LED which is intended to indicate
+ a device in a standby like state.
+
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v3] LEDS: Add output invertion option to backlight trigger
@ 2011-01-10 4:54 ` Janusz Krzysztofik
0 siblings, 0 replies; 16+ messages in thread
From: Janusz Krzysztofik @ 2011-01-10 4:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Richard Purdie, linux-fbdev@vger.kernel.org, linux-kernel,
linux-omap@vger.kernel.org, Paul Mundt, Richard Purdie
This patch extends the LED backlight tirgger driver with an option that
allows for inverting the trigger output polarity.
With the invertion option provided, I (ab)use the backlight trigger for
driving a LED that indicates LCD display blank condtition on my Amstrad
Delta videophone. Since the machine has no dedicated power LED, it was
not possible to distinguish if the display was blanked, or the machine
was turned off, without touching it.
The invert sysfs control is patterned after a similiar function of the
GPIO trigger driver.
Created and tested against linux-2.6.37 on Amstrad Delta.
Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
Cc: Richard Purdie <rpurdie@rpsys.net>
---
v2 -> v3 changes, all provided, requested or inspired by Andrew Morton
(thanks!):
- sysfs file should show "0" or "1" to match the thing which the user
wrote there,
- use strict_strtoul() so the kernel correctly rejects non-numerical
input,
- disallow any input other than 0 or 1,
- new sysfs file should be documented,
- the new sysfs file name could be consistent with the one already used
by the gpio trigger for a similiar function.
v1 -> v2 changes:
- improve some conditional expressions to be more readable; thanks to
Ralph Corderoy (from e3-hacking) and Lars-Peter Clausen for their
suggestions,
- refresh against linux-2.6.36-rc5.
Documentation/ABI/testing/sysfs-class-led | 10 ++++
drivers/leds/ledtrig-backlight.c | 61 ++++++++++++++++++++++++++++--
2 files changed, 67 insertions(+), 4 deletions(-)
--- linux-2.6.37/drivers/leds/ledtrig-backlight.c.orig 2011-01-10 02:55:26.000000000 +0100
+++ linux-2.6.37/drivers/leds/ledtrig-backlight.c 2011-01-10 03:12:46.000000000 +0100
@@ -26,6 +26,7 @@ struct bl_trig_notifier {
int brightness;
int old_status;
struct notifier_block notifier;
+ unsigned invert;
};
static int fb_notifier_callback(struct notifier_block *p,
@@ -36,23 +37,64 @@ static int fb_notifier_callback(struct n
struct led_classdev *led = n->led;
struct fb_event *fb_event = data;
int *blank = fb_event->data;
+ int new_status = *blank ? BLANK : UNBLANK;
switch (event) {
case FB_EVENT_BLANK :
- if (*blank && n->old_status == UNBLANK) {
+ if (new_status == n->old_status)
+ break;
+
+ if ((n->old_status == UNBLANK) ^ n->invert) {
n->brightness = led->brightness;
led_set_brightness(led, LED_OFF);
- n->old_status = BLANK;
- } else if (!*blank && n->old_status == BLANK) {
+ } else {
led_set_brightness(led, n->brightness);
- n->old_status = UNBLANK;
}
+
+ n->old_status = new_status;
+
break;
}
return 0;
}
+static ssize_t bl_trig_invert_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+
+ return sprintf(buf, "%u\n", n->invert);
+}
+
+static ssize_t bl_trig_invert_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t num)
+{
+ struct led_classdev *led = dev_get_drvdata(dev);
+ struct bl_trig_notifier *n = led->trigger_data;
+ unsigned long invert;
+ int ret;
+
+ ret = strict_strtoul(buf, 10, &invert);
+ if (ret < 0)
+ return ret;
+
+ if (invert > 1)
+ return -EINVAL;
+
+ n->invert = invert;
+
+ /* After inverting, we need to update the LED. */
+ if ((n->old_status == BLANK) ^ n->invert)
+ led_set_brightness(led, LED_OFF);
+ else
+ led_set_brightness(led, n->brightness);
+
+ return num;
+}
+static DEVICE_ATTR(inverted, 0644, bl_trig_invert_show, bl_trig_invert_store);
+
static void bl_trig_activate(struct led_classdev *led)
{
int ret;
@@ -66,6 +108,10 @@ static void bl_trig_activate(struct led_
return;
}
+ ret = device_create_file(led->dev, &dev_attr_inverted);
+ if (ret)
+ goto err_invert;
+
n->led = led;
n->brightness = led->brightness;
n->old_status = UNBLANK;
@@ -74,6 +120,12 @@ static void bl_trig_activate(struct led_
ret = fb_register_client(&n->notifier);
if (ret)
dev_err(led->dev, "unable to register backlight trigger\n");
+
+ return;
+
+err_invert:
+ led->trigger_data = NULL;
+ kfree(n);
}
static void bl_trig_deactivate(struct led_classdev *led)
@@ -82,6 +134,7 @@ static void bl_trig_deactivate(struct le
(struct bl_trig_notifier *) led->trigger_data;
if (n) {
+ device_remove_file(led->dev, &dev_attr_inverted);
fb_unregister_client(&n->notifier);
kfree(n);
}
--- linux-2.6.37/Documentation/ABI/testing/sysfs-class-led.orig 2011-01-10 02:42:16.000000000 +0100
+++ linux-2.6.37/Documentation/ABI/testing/sysfs-class-led 2011-01-10 03:38:28.000000000 +0100
@@ -26,3 +26,13 @@ Description:
scheduler is chosen. Trigger specific parameters can appear in
/sys/class/leds/<led> once a given trigger is selected.
+What: /sys/class/leds/<led>/inverted
+Date: January 2011
+KernelVersion: 2.6.38
+Contact: Richard Purdie <rpurdie@rpsys.net>
+Description:
+ Invert the LED on/off state. This parameter is specific to
+ gpio and backlight triggers. In case of the backlight trigger,
+ it is usefull when driving a LED which is intended to indicate
+ a device in a standby like state.
+
^ permalink raw reply [flat|nested] 16+ messages in thread