Linux LED subsystem development
 help / color / mirror / Atom feed
* [PATCH] Enable backlight when trigger is activated
From: Pavel Machek @ 2019-07-18 19:08 UTC (permalink / raw)
  To: kernel list, linux-arm-kernel, linux-omap, tony, sre, nekit1000,
	mpartap, merlijn, jacek.anaszewski, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Configuring backlight trigger from dts results in backlight off during
boot. Machine looks dead upon boot, which is not good.

Fix that by enabling LED on trigger activation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
index 487577d..6e6bc78 100644
--- a/drivers/leds/trigger/ledtrig-backlight.c
+++ b/drivers/leds/trigger/ledtrig-backlight.c
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+
 	ret = fb_register_client(&n->notifier);
 	if (ret)
 		dev_err(led->dev, "unable to register backlight trigger\n");
@@ -126,6 +128,7 @@ static void bl_trig_deactivate(struct led_classdev *led)
 	struct bl_trig_notifier *n = led_get_trigger_data(led);
 
 	fb_unregister_client(&n->notifier);
+	led_set_brightness(led, LED_OFF);
 	kfree(n);
 }
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply related

* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core
From: Jacek Anaszewski @ 2019-07-18 17:49 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, devicetree
In-Reply-To: <49152281-059c-6006-4c0f-a6be96a12707@ti.com>

On 7/18/19 3:31 PM, Jean-Jacques Hiblot wrote:
> 
> On 18/07/2019 14:24, Jacek Anaszewski wrote:
>> Hi Jean,
>>
>> Thank you for the updated patch set.
>>
>> I have some more comments below.
>>
>> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
>>>   +static bool __led_need_regulator_update(struct led_classdev
>>> *led_cdev,
>>> +                    int brightness)
>>> +{
>>> +    bool new_state = (brightness != LED_OFF);
>> How about:
>>
>> bool new_state = !!brightness;
> 
> Throughout the code LED_OFF is used when the LED is turned off. I think
> it would be more consistent to use it there too.

Basically brightness is a scalar and 0 always means off.
We treat enum led_brightness as a legacy type - it is no
longer valid on the whole its span since LED_FULL = 255
was depreciated with addition of max_brightness property.

IMHO use of reverse logic here only hinders code analysis.

>>> +
>>> +    return led_cdev->regulator && led_cdev->regulator_state !=
>>> new_state;
>>> +}
>>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>>> +                int brightness)
>>> +{
>>> +    int rc;
>>> +
>>> +    if (__led_need_regulator_update(led_cdev, brightness)) {
>>> +
>>> +        if (brightness != LED_OFF)
>>> +            rc = regulator_enable(led_cdev->regulator);
>>> +        else
>>> +            rc = regulator_disable(led_cdev->regulator);
>>> +        if (rc)
>>> +            return rc;
>>> +
>>> +        led_cdev->regulator_state = (brightness != LED_OFF);
>>> +    }
>>> +    return 0;
>>> +}
>> Let's have these function names without leading underscores.
> OK.
>>
>>>   static int __led_set_brightness(struct led_classdev *led_cdev,
>>>                   enum led_brightness value)
>>>   {
>>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
>>> work_struct *ws)
>>>       if (ret == -ENOTSUPP)
>>>           ret = __led_set_brightness_blocking(led_cdev,
>>>                       led_cdev->delayed_set_value);
>>> +    __led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
>> If you called it from __led_set_brightness() and
> 
> We cannot call it from __led_set_brightness() because it is supposed not
> to block.

You're right. The problematic part is that with regulator handling
we cannot treat the whole brightness setting operation uniformly
for brightness_set op case, i.e. without mediation of a workqueue.

Now you have to fire workqueue in led_set_brightness_nopm()
even for brightness_set() op path, if regulator state needs update.
This is ugly and can be misleading. Can be also error prone and
have non-obvious implications for software blink state transitions.

I think we would first need to improve locking between the workqueue
and led_timer_function(). I proposed a patch [0] over a year
ago.

Only then we could think of adding another asynchronous dependency
to the brightness setting chain.

[0] https://lkml.org/lkml/2018/1/17/1144

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-18 13:31 UTC (permalink / raw)
  To: Jacek Anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, devicetree
In-Reply-To: <4bd3b558-ea5b-0d2e-16b2-5b2e8bb484d2@gmail.com>


On 18/07/2019 14:24, Jacek Anaszewski wrote:
> Hi Jean,
>
> Thank you for the updated patch set.
>
> I have some more comments below.
>
> On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
>>   
>> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
>> +					int brightness)
>> +{
>> +	bool new_state = (brightness != LED_OFF);
> How about:
>
> bool new_state = !!brightness;

Throughout the code LED_OFF is used when the LED is turned off. I think 
it would be more consistent to use it there too.

>
>> +
>> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
>> +}
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> +				int brightness)
>> +{
>> +	int rc;
>> +
>> +	if (__led_need_regulator_update(led_cdev, brightness)) {
>> +
>> +		if (brightness != LED_OFF)
>> +			rc = regulator_enable(led_cdev->regulator);
>> +		else
>> +			rc = regulator_disable(led_cdev->regulator);
>> +		if (rc)
>> +			return rc;
>> +
>> +		led_cdev->regulator_state = (brightness != LED_OFF);
>> +	}
>> +	return 0;
>> +}
> Let's have these function names without leading underscores.
OK.
>
>>   static int __led_set_brightness(struct led_classdev *led_cdev,
>>   				enum led_brightness value)
>>   {
>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws)
>>   	if (ret == -ENOTSUPP)
>>   		ret = __led_set_brightness_blocking(led_cdev,
>>   					led_cdev->delayed_set_value);
>> +	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value)
> If you called it from __led_set_brightness() and

We cannot call it from __led_set_brightness() because it is supposed not 
to block.

JJ


^ permalink raw reply

* Re: [PATCH v5 26/26] leds: Document standard LED functions
From: Jacek Anaszewski @ 2019-07-18 13:31 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-leds, devicetree, linux-kernel, robh, dtor, linux, dmurphy
In-Reply-To: <20190718110352.GB3859@amd>

Hi Pavel,

Thanks for the review.

On 7/18/19 1:03 PM, Pavel Machek wrote:
> Hi!
> 
>> Add a documentation for standard LED functions with regard
>> to differences in meaning between cases when devicename section
>> of LED name is present or not.
>>
>> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> ---
>>  Documentation/leds/led-functions.txt | 223 +++++++++++++++++++++++++++++++++++
>>  Documentation/leds/leds-class.txt    |   3 +
>>  2 files changed, 226 insertions(+)
>>  create mode 100644 Documentation/leds/led-functions.txt
>>
>> diff --git a/Documentation/leds/led-functions.txt b/Documentation/leds/led-functions.txt
>> new file mode 100644
>> index 000000000000..003b6b6271d1
>> --- /dev/null
>> +++ b/Documentation/leds/led-functions.txt
>> @@ -0,0 +1,223 @@
>> +This file presents standardized LED functions and their meaning.
>> +
>> +Each LED function is described using below template:
>> +
>> +- LED function name
>> +    NDEV : <function meaning when LED devicename section is absent>
>> +    DEV  : <function meaning when LED devicename section is present>
>> +    DEVICENAME : <expected LED devicename for DEV case>
>> +    TRIGGER: <matching LED trigger>
>> +
>> +/* LED functions with corresponding trigger support */
>> +
>> +- activity
>> +    NDEV : system activity
>> +    DEV  : n/a
>> +    TRIGGER : "activity"
>> +
>> +- backlight
>> +    NDEV : n/a
>> +    DEV  : backlight of a frame buffer device
>> +    DEVICENAME : associated frame buffer device, e.g. fb0
>> +    TRIGGER: "backlight"
> 
> ndev: if there's single one on the platform?

Ack.

>> +- capslock
>> +    NDEV : n/a
>> +    DEV  : keyboard capslock state related to the specified input device
>> +    DEVICENAME : associated input device, e.g. input1
>> +    TRIGGER : "kbd-capslock"
>> +
> 
>> +- disk
>> +    NDEV : rw activity on any disk in the system
>> +    DEV  : rw activity on specified disk
>> +    DEVICENAME : associated disk, e.g.: hda, sdb
>> +    TRIGGER : "disk-activity", applies only to NDEV case
> 
> I'd sort this file according to the places where these leds are
> usually are present, to make it simpler for user to find the
> labels. capslock should go near scrollock etc.
> 
> Plus I guess explanation in which systems such LED is found would be
> good.

I need more input on that. No idea what you mean.

> Global "disk" LED is very common on the PCs, and we should make sure
> such LEDs have consistent labeling everywhere.

Could you please be more specific and give some examples of the rework
you propose? Whereas it's clear what you mean regarding keyboard LEDs,
I'm not sure what is your intention in case of "disk".

>> +- disk-read
>> +    NDEV : read activity on any disk in the system
>> +    DEV  : read activity on specified disk
>> +    DEVICENAME : associted disk, e.g.: hda, sdb
>> +    TRIGGER : "disk-read", applies only to NDEV case
>> +
>> +- disk-write
>> +    NDEV : write activity on any disk in the system
>> +    DEV  : write activity on specified disk
>> +    DEVICENAME : associated disk, .e.g" hda, sdb
>> +    TRIGGER : "disk-write", applies only to NDEV case
> 
> I don't see separated read/write LEDs very often. To keep the file
> size down, I'd list is at "disk-read, disk-write".

Ack.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH v4 0/4] Add a generic driver for LED-based backlight
From: Jacek Anaszewski @ 2019-07-18 13:19 UTC (permalink / raw)
  To: lee.jones
  Cc: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland,
	daniel.thompson, jingoohan1, dmurphy, linux-leds, linux-kernel,
	dri-devel, tomi.valkeinen
In-Reply-To: <20190717141514.21171-1-jjhiblot@ti.com>

On 7/17/19 4:15 PM, Jean-Jacques Hiblot wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight,
> but using a LED class device underneath.
> 
> A few years ago (2015), Tomi Valkeinen posted a series implementing a
> backlight driver on top of a LED device:
> https://patchwork.kernel.org/patch/7293991/
> https://patchwork.kernel.org/patch/7294001/
> https://patchwork.kernel.org/patch/7293981/
> 
> The discussion stopped because Tomi lacked the time to work on it.
> 
> changes in v4:
> - fix dev_err() messages and commit logs following the advices of Pavel
> - cosmetic changes (indents, getting rid of  "? 1 : 0" in
>   led_match_led_node())
> 
> changes in v3:
> - dt binding: don't limit the brightness range to 0-255. Use the range of
>   the underlying LEDs. as a side-effect, all LEDs must now have the same
>   range
> - driver: Adapt to dt binding update.
> - driver: rework probe() for clarity and remove the remaining goto.
> 
> changes in v2:
> - handle more than one LED.
> - don't make the backlight device a child of the LED controller.
> - make brightness-levels and default-brightness-level optional
> - removed the option to use a GPIO enable.
> - removed the option to use a regulator. It should be handled by the LED
>   core
> - don't make any change to the LED core (not needed anymore)
> 
> Jean-Jacques Hiblot (2):
>   leds: Add managed API to get a LED from a device driver
>   dt-bindings: backlight: Add led-backlight binding
> 
> Tomi Valkeinen (2):
>   leds: Add of_led_get() and led_put()
>   backlight: add led-backlight driver
> 
>  .../bindings/leds/backlight/led-backlight.txt |  28 ++
>  drivers/leds/led-class.c                      |  92 ++++++
>  drivers/video/backlight/Kconfig               |   7 +
>  drivers/video/backlight/Makefile              |   1 +
>  drivers/video/backlight/led_bl.c              | 268 ++++++++++++++++++
>  include/linux/leds.h                          |   6 +
>  6 files changed, 402 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
>  create mode 100644 drivers/video/backlight/led_bl.c
> 

For the whole set:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Lee - we need to create immutable branch for this set since there will
be some interfering changes in the LED core in this cycle.

I can create the branch and send the pull request once we will
obtain the ack from Rob for DT bindings, unless you have other
preference.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH v4 3/4] dt-bindings: backlight: Add led-backlight binding
From: Jacek Anaszewski @ 2019-07-18 12:30 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	devicetree@vger.kernel.org
In-Reply-To: <20190717141514.21171-4-jjhiblot@ti.com>

Cc devicetree@vger.kernel.org list - Rob once informed us this gets
higher priority in his queue this way.

On 7/17/19 4:15 PM, Jean-Jacques Hiblot wrote:
> Add DT binding for led-backlight.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  .../bindings/leds/backlight/led-backlight.txt | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..4c7dfbe7f67a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> @@ -0,0 +1,28 @@
> +led-backlight bindings
> +
> +This binding is used to describe a basic backlight device made of LEDs.
> +It can also be used to describe a backlight device controlled by the output of
> +a LED driver.
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: a list of LEDs
> +
> +Optional properties:
> +  - brightness-levels: Array of distinct brightness levels. The levels must be
> +                       in the range accepted by the underlying LED devices.
> +                       This is used to translate a backlight brightness level
> +                       into a LED brightness level. If it is not provided, the
> +                       identity mapping is used.
> +
> +  - default-brightness-level: The default brightness level.
> +
> +Example:
> +
> +	backlight {
> +		compatible = "led-backlight";
> +
> +		leds = <&led1>, <&led2>;
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;
> +	};
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core
From: Jacek Anaszewski @ 2019-07-18 12:24 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, devicetree
In-Reply-To: <20190717135948.19340-3-jjhiblot@ti.com>

Hi Jean,

Thank you for the updated patch set.

I have some more comments below.

On 7/17/19 3:59 PM, Jean-Jacques Hiblot wrote:
> Sometimes LEDs are powered by an external voltage/current regulator. Let
> the LED core know about it. This allows the LED core to turn on or off
> managed power supplies.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Reviewed-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/leds/led-class.c | 15 +++++++++++++
>  drivers/leds/led-core.c  | 47 +++++++++++++++++++++++++++++++++++++---
>  drivers/leds/leds.h      |  1 +
>  include/linux/leds.h     |  4 ++++
>  4 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77808e2..cadd43c30d50 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>  {
>  	char name[LED_MAX_NAME_SIZE];
>  	int ret;
> +	struct regulator *regulator;
>  
>  	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>  	if (ret < 0)
> @@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
>  		dev_warn(parent, "Led %s renamed to %s due to name collision",
>  				led_cdev->name, dev_name(led_cdev->dev));
>  
> +	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
> +	if (IS_ERR(regulator)) {
> +		if (PTR_ERR(regulator) != -ENODEV) {
> +			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> +				led_cdev->name);
> +			device_unregister(led_cdev->dev);
> +			mutex_unlock(&led_cdev->led_access);
> +			return PTR_ERR(regulator);
> +		}
> +		led_cdev->regulator = NULL;
> +	} else {
> +		led_cdev->regulator = regulator;
> +	}
> +
>  	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>  		ret = led_add_brightness_hw_changed(led_cdev);
>  		if (ret) {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 7107cd7e87cf..dab32cf778f2 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
>  
> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> +					int brightness)
> +{
> +	bool new_state = (brightness != LED_OFF);

How about:

bool new_state = !!brightness;

> +
> +	return led_cdev->regulator && led_cdev->regulator_state != new_state;
> +}
> +static int __led_handle_regulator(struct led_classdev *led_cdev,
> +				int brightness)
> +{
> +	int rc;
> +
> +	if (__led_need_regulator_update(led_cdev, brightness)) {
> +
> +		if (brightness != LED_OFF)
> +			rc = regulator_enable(led_cdev->regulator);
> +		else
> +			rc = regulator_disable(led_cdev->regulator);
> +		if (rc)
> +			return rc;
> +
> +		led_cdev->regulator_state = (brightness != LED_OFF);
> +	}
> +	return 0;
> +}

Let's have these function names without leading underscores.

>  static int __led_set_brightness(struct led_classdev *led_cdev,
>  				enum led_brightness value)
>  {
> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws)
>  	if (ret == -ENOTSUPP)
>  		ret = __led_set_brightness_blocking(led_cdev,
>  					led_cdev->delayed_set_value);
> +	__led_handle_regulator(led_cdev, led_cdev->delayed_set_value)

If you called it from __led_set_brightness() and
from __led_set_brightness_blocking() you wouldn't need this change here.

> +
>  	if (ret < 0 &&
>  	    /* LED HW might have been unplugged, therefore don't warn */
>  	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
> @@ -256,8 +285,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
>  			      enum led_brightness value)
>  {
>  	/* Use brightness_set op if available, it is guaranteed not to sleep */
> -	if (!__led_set_brightness(led_cdev, value))
> -		return;
> +	if (!__led_set_brightness(led_cdev, value)) {
> +		/*
> +		 * if regulator state doesn't need to be changed, that is all/
> +		 * Otherwise delegate the change to a work queue
> +		 */
> +		if (!__led_need_regulator_update(led_cdev, value))
> +			return;
> +	}

This change should be also not needed then.

>  
>  	/* If brightness setting can sleep, delegate it to a work queue task */
>  	led_cdev->delayed_set_value = value;
> @@ -280,6 +315,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>  int led_set_brightness_sync(struct led_classdev *led_cdev,
>  			    enum led_brightness value)
>  {
> +	int ret;
> +
>  	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>  		return -EBUSY;
>  
> @@ -288,7 +325,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
>  	if (led_cdev->flags & LED_SUSPENDED)
>  		return 0;
>  
> -	return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> +	ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> +	if (ret)
> +		return ret;
> +
> +	return __led_handle_regulator(led_cdev, led_cdev->brightness);

As well as this one.

>  }
>  EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>  
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 47b229469069..5aa5c038bd38 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -11,6 +11,7 @@
>  
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
> +#include <linux/regulator/consumer.h>
>  
>  static inline int led_get_brightness(struct led_classdev *led_cdev)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 9b2bf574a17a..bee8e3f8dddd 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -123,6 +123,10 @@ struct led_classdev {
>  
>  	/* Ensures consistent access to the LED Flash Class device */
>  	struct mutex		led_access;
> +
> +	/* regulator */
> +	struct regulator	*regulator;
> +	bool			regulator_state;
>  };
>  
>  extern int of_led_classdev_register(struct device *parent,
> 

-- 
Best regards,
Jacek Anaszewski



^ permalink raw reply

* Re: [PATCH v5 26/26] leds: Document standard LED functions
From: Pavel Machek @ 2019-07-18 11:03 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-leds, devicetree, linux-kernel, robh, dtor, linux, dmurphy
In-Reply-To: <20190609190803.14815-27-jacek.anaszewski@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

Hi!

> Add a documentation for standard LED functions with regard
> to differences in meaning between cases when devicename section
> of LED name is present or not.
> 
> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> ---
>  Documentation/leds/led-functions.txt | 223 +++++++++++++++++++++++++++++++++++
>  Documentation/leds/leds-class.txt    |   3 +
>  2 files changed, 226 insertions(+)
>  create mode 100644 Documentation/leds/led-functions.txt
> 
> diff --git a/Documentation/leds/led-functions.txt b/Documentation/leds/led-functions.txt
> new file mode 100644
> index 000000000000..003b6b6271d1
> --- /dev/null
> +++ b/Documentation/leds/led-functions.txt
> @@ -0,0 +1,223 @@
> +This file presents standardized LED functions and their meaning.
> +
> +Each LED function is described using below template:
> +
> +- LED function name
> +    NDEV : <function meaning when LED devicename section is absent>
> +    DEV  : <function meaning when LED devicename section is present>
> +    DEVICENAME : <expected LED devicename for DEV case>
> +    TRIGGER: <matching LED trigger>
> +
> +/* LED functions with corresponding trigger support */
> +
> +- activity
> +    NDEV : system activity
> +    DEV  : n/a
> +    TRIGGER : "activity"
> +
> +- backlight
> +    NDEV : n/a
> +    DEV  : backlight of a frame buffer device
> +    DEVICENAME : associated frame buffer device, e.g. fb0
> +    TRIGGER: "backlight"

ndev: if there's single one on the platform?

> +- capslock
> +    NDEV : n/a
> +    DEV  : keyboard capslock state related to the specified input device
> +    DEVICENAME : associated input device, e.g. input1
> +    TRIGGER : "kbd-capslock"
> +

> +- disk
> +    NDEV : rw activity on any disk in the system
> +    DEV  : rw activity on specified disk
> +    DEVICENAME : associated disk, e.g.: hda, sdb
> +    TRIGGER : "disk-activity", applies only to NDEV case

I'd sort this file according to the places where these leds are
usually are present, to make it simpler for user to find the
labels. capslock should go near scrollock etc.

Plus I guess explanation in which systems such LED is found would be
good.

Global "disk" LED is very common on the PCs, and we should make sure
such LEDs have consistent labeling everywhere.

> +- disk-read
> +    NDEV : read activity on any disk in the system
> +    DEV  : read activity on specified disk
> +    DEVICENAME : associted disk, e.g.: hda, sdb
> +    TRIGGER : "disk-read", applies only to NDEV case
> +
> +- disk-write
> +    NDEV : write activity on any disk in the system
> +    DEV  : write activity on specified disk
> +    DEVICENAME : associated disk, .e.g" hda, sdb
> +    TRIGGER : "disk-write", applies only to NDEV case

I don't see separated read/write LEDs very often. To keep the file
size down, I'd list is at "disk-read, disk-write".

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [PATCH v5 00/26] Add generic support for composing LED class device name
From: Pavel Machek @ 2019-07-18 10:52 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-leds, dmurphy, devicetree, linux-kernel, robh, dtor, linux
In-Reply-To: <405b2806-342a-952d-67ab-47516225c54e@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]


> Hi all,
> 
> I need explicit acks for some patches from this series, that
> were either requested improvements or I modified them by myself
> after v4.
> 
> The patches I am talking about are the following:
> 
> 1/26
> 21/26
> 23/26
> 25/26

Acked-by: Pavel Machek <pavel@ucw.cz>

> 26/26 would be nice to have but I presume it needs more discussion
> and analysis.

Idea is good, but I'd sort the file in different way.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [PATCH 1/1] leds: leds-lp5562 allow firmware files up to the maximum length
From: Jacek Anaszewski @ 2019-07-17 22:19 UTC (permalink / raw)
  To: Nick Stoughton, linux-leds
In-Reply-To: <20190717215606.3053-2-nstoughton@logitech.com>

Hi Nick,

Thank you for the update. Now it applies cleanly.

On 7/17/19 11:56 PM, Nick Stoughton wrote:
> Firmware files are in ASCII, using 2 hex characters per byte. The
> maximum length of a firmware string is therefore
> 
> 16 (commands) * 2 (bytes per command) * 2 (characters per byte) = 64
> 
> Signed-off-by: Nick Stoughton <nstoughton@logitech.com>
> ---
>  drivers/leds/leds-lp5562.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
> index 37632fc63741..e00117e3b50d 100644
> --- a/drivers/leds/leds-lp5562.c
> +++ b/drivers/leds/leds-lp5562.c
> @@ -260,7 +260,11 @@ static void lp5562_firmware_loaded(struct lp55xx_chip *chip)
>  {
>  	const struct firmware *fw = chip->fw;
>  
> -	if (fw->size > LP5562_PROGRAM_LENGTH) {
> +        /*
> +         * the firmware is encoded in ascii hex character, with 2 chars
> +         * per byte
> +         */

If you used scripts/checkpatch.pl on this patch then you would
notice some problems here. I fixed them up by myself.

Added also the tag to the commit message:

Fixes: ff45262a85db ("leds: add new LP5562 LED driver")

and applied.

Thanks.

> +	if (fw->size > (LP5562_PROGRAM_LENGTH * 2)) {
>  		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
>  			fw->size);
>  		return;
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* [PATCH 1/1] leds: leds-lp5562 allow firmware files up to the maximum length
From: Nick Stoughton @ 2019-07-17 21:56 UTC (permalink / raw)
  To: linux-leds; +Cc: Nick Stoughton
In-Reply-To: <20190717215606.3053-1-nstoughton@logitech.com>

Firmware files are in ASCII, using 2 hex characters per byte. The
maximum length of a firmware string is therefore

16 (commands) * 2 (bytes per command) * 2 (characters per byte) = 64

Signed-off-by: Nick Stoughton <nstoughton@logitech.com>
---
 drivers/leds/leds-lp5562.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
index 37632fc63741..e00117e3b50d 100644
--- a/drivers/leds/leds-lp5562.c
+++ b/drivers/leds/leds-lp5562.c
@@ -260,7 +260,11 @@ static void lp5562_firmware_loaded(struct lp55xx_chip *chip)
 {
 	const struct firmware *fw = chip->fw;
 
-	if (fw->size > LP5562_PROGRAM_LENGTH) {
+        /*
+         * the firmware is encoded in ascii hex character, with 2 chars
+         * per byte
+         */
+	if (fw->size > (LP5562_PROGRAM_LENGTH * 2)) {
 		dev_err(&chip->cl->dev, "firmware data size overflow: %zu\n",
 			fw->size);
 		return;
-- 
Nick Stoughton


^ permalink raw reply related

* [PATCH 0/1] Resend leds-lp5562 allow firmware files up to the maximum length
From: Nick Stoughton @ 2019-07-17 21:56 UTC (permalink / raw)
  To: linux-leds; +Cc: Nick Stoughton


As requested, resending patch with git-send-email.

Nick Stoughton (1):
  leds: leds-lp5562 allow firmware files up to the maximum length

 drivers/leds/leds-lp5562.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
Nick Stoughton


^ permalink raw reply

* Re: [PATCH v5 00/26] Add generic support for composing LED class device name
From: Jacek Anaszewski @ 2019-07-17 21:14 UTC (permalink / raw)
  To: linux-leds, pavel, dmurphy; +Cc: devicetree, linux-kernel, robh, dtor, linux
In-Reply-To: <20190609190803.14815-1-jacek.anaszewski@gmail.com>

Hi all,

I need explicit acks for some patches from this series, that
were either requested improvements or I modified them by myself
after v4.

The patches I am talking about are the following:

1/26
21/26
23/26
25/26

26/26 would be nice to have but I presume it needs more discussion
and analysis.

Best regards,
Jacek Anaszewski

On 6/9/19 9:07 PM, Jacek Anaszewski wrote:
> Changes from v4:
> 
> - switched "charge" function name to "charging"
> - added "cpu", "mute", "micmute", "disk-activity", "panic", "mtd" LED functions
>   to cover all existing triggers and removed now redundant "nand" and "mmc"
> - added "capslock", "scrollock", "numlock" LED functions
> - removed now redundant "keyboard" and "keypad" since there is "kbd_backlight"
>   already available
> - removed "tv" LED function as depreciated
> - switched LED_COLOR_ID_COUNT to LED_COLOR_ID_MAX
> - fixed led_classdev_register_ext() to not leave struct led_classdev's
>   name pointing to no longer existing composed_name stack variable
> - fixed leds-as3645 and leds-aat1290 to no longer rely on struct led_classdev's
>   name property
> - added basic LED class device name validation to get_led_device_info.sh
> - tweaked LED naming section in leds_class.txt to allow devicename section
>   also for non hot-pluggable devices
> - always initialize all fields of struct led_init_data to zero on declaration
>   in drivers
> - fix leds-gpio to avoid overwriting the LED name coming from platform_data
> - add description of LED function names with regard to whether devicename
>   section is initialized or not
> 
> Changes from v3:
> 
> - allow for devicename section for hot-pluggable devices
> - move led_colors array to led-core.c to avoid build break
>   due to Kconfig dependency issue
> - add a patch fixing led_colors array name clash with ALSA driver
> - change led-enumerator DT property name to more meaningful function-enumerator
> - add LED_FUNCTION_KBD_BACKLIGHT
> - change naming and add new proprties to struct led_init_data
>   and struct led_properties
> 
> Changes from v2:
> 
> - removed from drivers the responsibility of calling led_compose_name()
> - added struct device* argument to led_compose_name() to allow using
>   dev_<level> logging functions for more informative logs
> - adjusted the list of LED_FUNCTION definitions according to the v2 review
>   remarks
> - renamed default_desc to default_label in the struct led_init_data
> - added led-enumerator DT property to the common LED bindings
> - removed LED_COLOR_NAME definitions from include/dt-bindings/leds/common.h
> - change DT color property type from string to integer
> - change struct initialization list to explicit property assignment in leds-sc27xx-bltc.c
> - use led->client->name for led_hw_name in leds-lm3692x.c
> - few other minor improvements to docs etc.
> 
> Changes from v1:
> 
> - improved led_parse_properties() to parse label property at first
>   and return immediately after parsing succeeds
> - added tool get_led_device_info.sh for retrieving LED class device's
>   parent device related information
> - extended LED naming section of Documentation/leds/leds-class.txt
> - adjusted the list of LED_FUNCTION definitions according to the v1 review
>   remarks
> - added standard LED_COLOR_NAME definitions
> - removed functions.h and moved both LED_FUNCTION and LED_COLOR_NAME
>   definitions to include/dt-bindings/common.h
> - rebased leds-as3645a changes on top of the patch switching to fwnode
>   property API
> - updated DT bindings to use new LED_COLOR_NAME definitions
> - improved common LED bindings to not use address unit for sub-nodes
>   without reg property
> 
> Generally I still insist on deprecating label property and devicename
> section of LED name. The tool I added for obtaining LED device name
> proves availability of the related information in other places in
> the sysfs. Other discussed use cases are covered in the updated
> Documentation/leds/leds-class.txt.
> 
> Beside that, I tried also to create macros for automatic composition
> of "-N" suffixed LED functions, so that it would not be necessary
> to pollute common.h with plenty repetitions of the same function,
> differing only with the postfix. Unfortunately, the preprocessor
> of the dtc compiler seems not to accept string concatenetation.
> I.e. it is not possible to to the following assighment:
> 
> function = "hdd""-1"
> 
> If anyone knows how to obviate this shortocoming please let me know.
> 
> Original cover letter:
> 
> LED class device naming pattern included devicename section, which had
> unpleasant effect of varying userspace interface dependent on underlaying
> hardware. Moreover, this information was redundant in the LED name, since
> the LED controller name could have been obtained from sysfs device group
> 
> This patch set introduces a led_compose_name() function in the LED core,
> which unifies and simplifies LED class device name composition. This
> change is accompanied by the improvements in the common LED DT bindings
> where two new properties are introduced: "function" and "color" . The two
> deprecate the old "label" property which was leaving too much room for
> interpretation, leading to inconsistent LED naming.
> 
> There are also changes in LED DT node naming, which are in line with
> DT maintainer's request from [0].
> 
> Since some DT LED naming unification, related to not including devicename
> section in "label" DT property, is being requested during reviews of new
> LED class drivers for almost a year now, then those drivers are the first
> candidates for optimalization and the first users of the new
> led_compose_name() API. The modifications were tested with Qemu,
> by stubbing the driver internals where hardware interaction was needed
> for proper probing.
> 
> Thanks,
> Jacek Anaszewski
> 
> Jacek Anaszewski (26):
>   leds: class: Improve LED and LED flash class registration API
>   dt-bindings: leds: Add LED_COLOR_ID definitions
>   dt-bindings: leds: Add LED_FUNCTION definitions
>   dt-bindings: leds: Add properties for LED name construction
>   leds: core: Add support for composing LED class device names
>   dt-bindings: sc27xx-blt: Add function and color properties
>   leds: sc27xx-blt: Use generic support for composing LED names
>   dt-bindings: lt3593: Add function and color properties
>   leds: lt3593: Use generic support for composing LED names
>   dt-bindings: lp8860: Add function and color properties
>   leds: lp8860: Use generic support for composing LED names
>   dt-bindings: lm3692x: Add function and color properties
>   leds: lm3692x: Use generic support for composing LED names
>   dt-bindings: lm36010: Add function and color properties
>   leds: lm3601x: Use generic support for composing LED names
>   dt-bindings: cr0014114: Add function and color properties
>   leds: cr0014114: Use generic support for composing LED names
>   dt-bindings: aat1290: Add function and color properties
>   leds: aat1290: Use generic support for composing LED names
>   dt-bindings: as3645a: Add function and color properties
>   leds: as3645a: Use generic support for composing LED names
>   dt-bindings: leds-gpio: Add function and color properties
>   leds: gpio: Use generic support for composing LED names
>   dt-bindings: an30259a: Add function and color properties
>   leds: an30259a: Use generic support for composing LED names
>   leds: Document standard LED functions
> 
>  .../devicetree/bindings/leds/ams,as3645a.txt       |  22 +-
>  Documentation/devicetree/bindings/leds/common.txt  |  62 +++++-
>  .../devicetree/bindings/leds/leds-aat1290.txt      |  12 +-
>  .../devicetree/bindings/leds/leds-an30259a.txt     |  22 +-
>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  26 ++-
>  .../devicetree/bindings/leds/leds-gpio.txt         |  23 ++-
>  .../devicetree/bindings/leds/leds-lm3601x.txt      |  10 +-
>  .../devicetree/bindings/leds/leds-lm3692x.txt      |   9 +-
>  .../devicetree/bindings/leds/leds-lp8860.txt       |   9 +-
>  .../devicetree/bindings/leds/leds-lt3593.txt       |  11 +-
>  .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |  10 +-
>  Documentation/leds/led-functions.txt               | 223 +++++++++++++++++++++
>  Documentation/leds/leds-class.txt                  |  70 ++++++-
>  drivers/leds/led-class-flash.c                     |   9 +-
>  drivers/leds/led-class.c                           |  49 +++--
>  drivers/leds/led-core.c                            | 127 ++++++++++++
>  drivers/leds/leds-aat1290.c                        |  16 +-
>  drivers/leds/leds-an30259a.c                       |  25 +--
>  drivers/leds/leds-as3645a.c                        |  74 +++----
>  drivers/leds/leds-cr0014114.c                      |  33 +--
>  drivers/leds/leds-gpio.c                           |  26 +--
>  drivers/leds/leds-lm3601x.c                        |  38 ++--
>  drivers/leds/leds-lm3692x.c                        |  22 +-
>  drivers/leds/leds-lp8860.c                         |  35 ++--
>  drivers/leds/leds-lt3593.c                         |  20 +-
>  drivers/leds/leds-pwm.c                            |   2 +-
>  drivers/leds/leds-sc27xx-bltc.c                    |  22 +-
>  drivers/leds/leds.h                                |   1 +
>  include/dt-bindings/leds/common.h                  |  55 ++++-
>  include/linux/led-class-flash.h                    |  15 +-
>  include/linux/leds.h                               |  79 +++++++-
>  tools/leds/get_led_device_info.sh                  | 201 +++++++++++++++++++
>  32 files changed, 1086 insertions(+), 272 deletions(-)
>  create mode 100644 Documentation/leds/led-functions.txt
>  create mode 100755 tools/leds/get_led_device_info.sh
> 



^ permalink raw reply

* Re: [PATCH v5 05/26] leds: core: Add support for composing LED class device names
From: Jacek Anaszewski @ 2019-07-17 21:03 UTC (permalink / raw)
  To: Pavel Machek
  Cc: linux-leds, devicetree, linux-kernel, robh, dtor, linux, dmurphy,
	Baolin Wang, Daniel Mack, Linus Walleij, Oleh Kravchenko,
	Sakari Ailus, Simon Shields
In-Reply-To: <20190703220043.GA876@amd>

Hi Pavel,

On 7/4/19 12:00 AM, Pavel Machek wrote:
> Hi!
> 
> Sorry for the delay.

No problem.

>> @@ -27,6 +29,18 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>>  LIST_HEAD(leds_list);
>>  EXPORT_SYMBOL_GPL(leds_list);
>>  
>> +const char *led_colors[LED_COLOR_ID_MAX] = {
> 
> const char * const , if we want to play that game?

Ack.

>> +	[LED_COLOR_ID_WHITE] = "white",
>> +	[LED_COLOR_ID_RED] = "red",
>> +	[LED_COLOR_ID_GREEN] = "green",
>> +	[LED_COLOR_ID_BLUE] = "blue",
>> +	[LED_COLOR_ID_AMBER] = "amber",
>> +	[LED_COLOR_ID_VIOLET] = "violet",
>> +	[LED_COLOR_ID_YELLOW] = "yellow",
>> +	[LED_COLOR_ID_IR] = "ir",
>> +};
>> +EXPORT_SYMBOL_GPL(led_colors);
>> +
> 
>> +	if (fwnode_property_present(fwnode, "label")) {
>> +		ret = fwnode_property_read_string(fwnode, "label", &props->label);
>> +		if (ret)
>> +			dev_err(dev, "Error parsing \'label\' property (%d)\n", ret);
>> +		return;
> 
> I don't think you need to escape ' with \.

Right.

>> +	if (fwnode_property_present(fwnode, "function")) {
>> +		ret = fwnode_property_read_string(fwnode, "function", &props->function);
>> +		if (ret) {
>> +			dev_err(dev,
>> +				"Error parsing \'function\' property (%d)\n",
>> +				ret);
>> +		}
>> +	} else {
>> +		return;
>> +	}
> 
>> +
>> +	if (fwnode_property_present(fwnode, "function-enumerator")) {
> 
> I'd do if (!fwnode_property_present()) return; in both occasions, to
> save an indentation level; but that's nitpicking.

Ack.

>> +	if (props.label) {
>> +		/*
>> +		 * If init_data.devicename is NULL, then it indicates that
>> +		 * DT label should be used as-is for LED class device name.
>> +		 * Otherwise the label is prepended with devicename to compose
>> +		 * the final LED class device name.
>> +		 */
>> +		if (!devicename) {
>> +			strncpy(led_classdev_name, props.label,
>> +				LED_MAX_NAME_SIZE);
>> +		} else {
>> +			snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>> +				 devicename, props.label);
>> +		}
> 
> Unlike snprintf(), strncpy() does not guarantee NULL termination.

Indeed. I'll change strncpy to strscpy.

> I did not check the shell script.
> 
> With that fixed,
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks!

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH 5/6] leds: apu: fix error on probing failure
From: Jacek Anaszewski @ 2019-07-17 16:37 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Pavel Machek,
	Enrico Weigelt, metux IT consult
  Cc: linux-kernel, dmurphy, linux-leds
In-Reply-To: <7ec18de5-f71b-4b9a-0db9-3c010a8e67e7@metux.net>

Hi Enrico

On 7/17/19 4:29 PM, Enrico Weigelt, metux IT consult wrote:
> On 16.07.19 21:28, Pavel Machek wrote:
> 
>> You may want to add here: "For APUv2,3 support, enable CONFIG_xxx".
>>
>> If you have any APUv2 users (and you may), this si chance to get their
>> attention.
> 
> Good idea. Shall I repost a changed patch ? (or repost the whole queue)

Please send only the patch in question (and bump the version).

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* Re: [PATCH 5/6] leds: apu: fix error on probing failure
From: Enrico Weigelt, metux IT consult @ 2019-07-17 14:29 UTC (permalink / raw)
  To: Pavel Machek, Enrico Weigelt, metux IT consult
  Cc: linux-kernel, jacek.anaszewski, dmurphy, linux-leds
In-Reply-To: <20190716192805.GE10400@amd>

On 16.07.19 21:28, Pavel Machek wrote:

> You may want to add here: "For APUv2,3 support, enable CONFIG_xxx".
> 
> If you have any APUv2 users (and you may), this si chance to get their
> attention.

Good idea. Shall I repost a changed patch ? (or repost the whole queue)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply

* Re: [PATCH 1/6] leds: apu: drop superseeded apu2/3 led support
From: Enrico Weigelt, metux IT consult @ 2019-07-17 14:28 UTC (permalink / raw)
  To: Pavel Machek, Enrico Weigelt, metux IT consult
  Cc: linux-kernel, jacek.anaszewski, dmurphy, linux-leds
In-Reply-To: <20190716193001.GG10400@amd>

On 16.07.19 21:30, Pavel Machek wrote:

Hi,

> Ok, so I understand the reasons, but people updating from old kernels
> (make oldconfig) will see nothing and their LEDs will stop working.
> 
> Can we do something to help them?

I could announce that in pcengines forum. There've been several
discussions on this matter, replying there should give the participants
an automatic mail.

So far, I haven't seen any distro that enabled the old driver, so I
guess the actual users all compiled the kernel on their own and should
notice the change early enough.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply

* Re: [PATCH 2/2] block: introduce LED block device activity trigger
From: Akinobu Mita @ 2019-07-17 14:27 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-block, linux-leds, linux-nvme, Frank Steiner, Pavel Machek,
	Dan Murphy, Jens Axboe
In-Reply-To: <89262967-667f-80cc-0fd5-ba480e879fe0@gmail.com>

2019年7月17日(水) 5:57 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>
> Hi Akinobu,
>
> Thank you for the patch set. It looks nice in general, but I'd like
> to maintain it under LED subsystem. See my below comments.

Thanks for reviewing. I'll apply your feedback.

> On 7/6/19 7:58 PM, Akinobu Mita wrote:
> > This allows LEDs to be controlled by block device activity.
> >
> > We already have ledtrig-disk (LED disk activity trigger), but the lower
> > level disk drivers need to utilize ledtrig_disk_activity() to make the
> > LED blink.
> >
> > The LED block device trigger doesn't require the lower level drivers to
> > have any instrumentation. The activity is collected by polling the disk
> > stats.
> >
> > Example:
> >
> > echo block-nvme0n1 > /sys/class/leds/diy/trigger
> >
> > Cc: Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Dan Murphy <dmurphy@ti.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  block/Makefile        |   1 +
> >  block/blk-ledtrig.c   | 219 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  block/blk.h           |  13 +++
> >  block/genhd.c         |   2 +
> >  include/linux/genhd.h |   4 +
> >  5 files changed, 239 insertions(+)
> >  create mode 100644 block/blk-ledtrig.c
> >
> > diff --git a/block/Makefile b/block/Makefile
> > index eee1b4c..c74d84e6 100644
> > --- a/block/Makefile
> > +++ b/block/Makefile
> > @@ -35,3 +35,4 @@ obj-$(CONFIG_BLK_DEBUG_FS)  += blk-mq-debugfs.o
> >  obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
> >  obj-$(CONFIG_BLK_SED_OPAL)   += sed-opal.o
> >  obj-$(CONFIG_BLK_PM)         += blk-pm.o
> > +obj-$(CONFIG_LEDS_TRIGGERS)  += blk-ledtrig.o
> > diff --git a/block/blk-ledtrig.c b/block/blk-ledtrig.c
>
> Please move the whole trigger implementation to
> drivers/leds/trigger and rename the file to ledtrig-blk.c

OK. Then we don't need to patch 1/2 ("leds: move declaration of
led_stop_software_blink() to linux/leds.h") anymore.

> > new file mode 100644
> > index 0000000..da93b06
> > --- /dev/null
> > +++ b/block/blk-ledtrig.c
> > @@ -0,0 +1,219 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// LED Kernel Blockdev Trigger
> > +// Derived from ledtrig-netdev.c
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/genhd.h>
> > +#include <linux/leds.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct blk_ledtrig_data {
> > +     struct delayed_work work;
> > +     struct led_classdev *led_cdev;
> > +
> > +     atomic_t interval;
> > +     u64 last_activity;
> > +
> > +     unsigned long mode;
> > +#define BLK_LEDTRIG_READ BIT(0)
> > +#define BLK_LEDTRIG_WRITE BIT(1)
> > +#define BLK_LEDTRIG_DISCARD BIT(2)
>
> s/BLK_LEDTRIG/LEDTRIG_BLK/

OK.

> > diff --git a/block/blk.h b/block/blk.h
> > index 7814aa2..dd4c230a 100644
> > --- a/block/blk.h
> > +++ b/block/blk.h
> > @@ -331,4 +331,17 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q);
> >  static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
> >  #endif
> >
> > +#ifdef CONFIG_LEDS_TRIGGERS
> > +int blk_ledtrig_register(struct gendisk *disk);
> > +void blk_ledtrig_unregister(struct gendisk *disk);
> > +#else
> > +static inline int blk_ledtrig_register(struct gendisk *disk)
> > +{
> > +     return 0;
> > +}
> > +static inline void blk_ledtrig_unregister(struct gendisk *disk)
> > +{
> > +}
> > +#endif /* CONFIG_LEDS_TRIGGERS */
>
> Please move this part to include/linux/leds.h, next to the other
> triggers' facilities.

OK.

^ permalink raw reply

* [PATCH v4 2/4] leds: Add managed API to get a LED from a device driver
From: Jean-Jacques Hiblot @ 2019-07-17 14:15 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot
In-Reply-To: <20190717141514.21171-1-jjhiblot@ti.com>

If the LED is acquired by a consumer device with devm_led_get(), it is
automatically released when the device is detached.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/led-class.c | 42 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  2 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 9f48798a713d..714b55f1da0f 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -263,6 +263,48 @@ void led_put(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_put);
 
+static void devm_led_release(struct device *dev, void *res)
+{
+	struct led_classdev **p = res;
+
+	led_put(*p);
+}
+
+/**
+ * devm_led_get - Resource-managed request of a LED device
+ * @dev:	LED consumer
+ * @idx:	index of the LED to obtain in the consumer
+ *
+ * The device node of the device is parse to find the request LED device.
+ * The LED device returned from this function is automatically released
+ * on driver detach.
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *__must_check devm_led_get(struct device *dev,
+					       int index)
+{
+	struct led_classdev *led;
+	struct led_classdev **dr;
+
+	led = of_led_get(dev->of_node, index);
+	if (IS_ERR(led))
+		return led;
+
+	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
+			  GFP_KERNEL);
+	if (!dr) {
+		led_put(led);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	*dr = led;
+	devres_add(dev, dr);
+
+	return led;
+}
+EXPORT_SYMBOL_GPL(devm_led_get);
+
 static int match_name(struct device *dev, const void *data)
 {
 	if (!dev_name(dev))
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 0a71c7cdd191..7fcec566d774 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -148,6 +148,8 @@ extern void led_classdev_resume(struct led_classdev *led_cdev);
 
 extern struct led_classdev *of_led_get(struct device_node *np, int index);
 extern void led_put(struct led_classdev *led_cdev);
+struct led_classdev *__must_check devm_led_get(struct device *dev,
+					       int index);
 
 /**
  * led_blink_set - set blinking with software fallback
-- 
2.17.1


^ permalink raw reply related

* [PATCH v4 3/4] dt-bindings: backlight: Add led-backlight binding
From: Jean-Jacques Hiblot @ 2019-07-17 14:15 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot
In-Reply-To: <20190717141514.21171-1-jjhiblot@ti.com>

Add DT binding for led-backlight.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 .../bindings/leds/backlight/led-backlight.txt | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
new file mode 100644
index 000000000000..4c7dfbe7f67a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
@@ -0,0 +1,28 @@
+led-backlight bindings
+
+This binding is used to describe a basic backlight device made of LEDs.
+It can also be used to describe a backlight device controlled by the output of
+a LED driver.
+
+Required properties:
+  - compatible: "led-backlight"
+  - leds: a list of LEDs
+
+Optional properties:
+  - brightness-levels: Array of distinct brightness levels. The levels must be
+                       in the range accepted by the underlying LED devices.
+                       This is used to translate a backlight brightness level
+                       into a LED brightness level. If it is not provided, the
+                       identity mapping is used.
+
+  - default-brightness-level: The default brightness level.
+
+Example:
+
+	backlight {
+		compatible = "led-backlight";
+
+		leds = <&led1>, <&led2>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+	};
-- 
2.17.1


^ permalink raw reply related

* [PATCH v4 1/4] leds: Add of_led_get() and led_put()
From: Jean-Jacques Hiblot @ 2019-07-17 14:15 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot
In-Reply-To: <20190717141514.21171-1-jjhiblot@ti.com>

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

This patch adds basic support for a kernel driver to get a LED device.
This will be used by the led-backlight driver.

Only OF version is implemented for now, and the behavior is similar to
PWM's of_pwm_get() and pwm_put().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c | 50 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  4 ++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cadd43c30d50..9f48798a713d 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <uapi/linux/uleds.h>
+#include <linux/of.h>
 #include "leds.h"
 
 static struct class *leds_class;
@@ -213,6 +214,55 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+static int led_match_led_node(struct device *led_dev, const void *data)
+{
+	return led_dev->of_node == data;
+}
+
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ * @index: the index of the LED
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ */
+struct led_classdev *of_led_get(struct device_node *np, int index)
+{
+	struct device *led_dev;
+	struct led_classdev *led_cdev;
+	struct device_node *led_node;
+
+	led_node = of_parse_phandle(np, "leds", index);
+	if (!led_node)
+		return ERR_PTR(-ENOENT);
+
+	led_dev = class_find_device(leds_class, NULL, led_node,
+		led_match_led_node);
+	of_node_put(led_node);
+
+	if (!led_dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	led_cdev = dev_get_drvdata(led_dev);
+
+	if (!try_module_get(led_cdev->dev->parent->driver->owner))
+		return ERR_PTR(-ENODEV);
+
+	return led_cdev;
+}
+EXPORT_SYMBOL_GPL(of_led_get);
+
+/**
+ * led_put() - release a LED device
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+	module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);
+
 static int match_name(struct device *dev, const void *data)
 {
 	if (!dev_name(dev))
diff --git a/include/linux/leds.h b/include/linux/leds.h
index bee8e3f8dddd..0a71c7cdd191 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -19,6 +19,7 @@
 
 struct device;
 struct led_pattern;
+struct device_node;
 /*
  * LED Core
  */
@@ -145,6 +146,9 @@ extern void devm_led_classdev_unregister(struct device *parent,
 extern void led_classdev_suspend(struct led_classdev *led_cdev);
 extern void led_classdev_resume(struct led_classdev *led_cdev);
 
+extern struct led_classdev *of_led_get(struct device_node *np, int index);
+extern void led_put(struct led_classdev *led_cdev);
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
-- 
2.17.1


^ permalink raw reply related

* [PATCH v4 4/4] backlight: add led-backlight driver
From: Jean-Jacques Hiblot @ 2019-07-17 14:15 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot
In-Reply-To: <20190717141514.21171-1-jjhiblot@ti.com>

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

This patch adds a led-backlight driver (led_bl), which is similar to
pwm_bl except the driver uses a LED class driver to adjust the
brightness in the HW. Multiple LEDs can be used for a single backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/video/backlight/Kconfig  |   7 +
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 268 +++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+)
 create mode 100644 drivers/video/backlight/led_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 8b081d61773e..585a1787618c 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -458,6 +458,13 @@ config BACKLIGHT_RAVE_SP
 	help
 	  Support for backlight control on RAVE SP device.
 
+config BACKLIGHT_LED
+	tristate "Generic LED based Backlight Driver"
+	depends on LEDS_CLASS && OF
+	help
+	  If you have a LCD backlight adjustable by LED class driver, say Y
+	  to enable this driver.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endmenu
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 63c507c07437..2a67642966a5 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
 obj-$(CONFIG_BACKLIGHT_ARCXCNN) 	+= arcxcnn_bl.o
 obj-$(CONFIG_BACKLIGHT_RAVE_SP)		+= rave-sp-backlight.o
+obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
new file mode 100644
index 000000000000..ac5ff78e7859
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2015-2019 Texas Instruments Incorporated -  http://www.ti.com/
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ */
+
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define BKL_FULL_BRIGHTNESS 255
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+	struct led_classdev	**leds;
+	bool			enabled;
+	int			nb_leds;
+	unsigned int		*levels;
+	unsigned int		default_brightness;
+	unsigned int		max_brightness;
+};
+
+static int to_led_brightness(struct led_classdev *led, int value)
+{
+	return (value * led->max_brightness) / BKL_FULL_BRIGHTNESS;
+}
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int level)
+{
+	int i;
+	int bkl_brightness;
+
+	if (priv->levels)
+		bkl_brightness = priv->levels[level];
+	else
+		bkl_brightness = level;
+
+	for (i = 0; i < priv->nb_leds; i++) {
+		int led_brightness;
+		struct led_classdev *led = priv->leds[i];
+
+		led_brightness = to_led_brightness(led, bkl_brightness);
+		led_set_brightness(led, led_brightness);
+	}
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	int i;
+
+	if (!priv->enabled)
+		return;
+
+	for (i = 0; i < priv->nb_leds; i++)
+		led_set_brightness(priv->leds[i], LED_OFF);
+
+	priv->enabled = false;
+}
+
+static int led_bl_update_status(struct backlight_device *bl)
+{
+	struct led_bl_data *priv = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	if (brightness > 0)
+		led_bl_set_brightness(priv, brightness);
+	else
+		led_bl_power_off(priv);
+
+	return 0;
+}
+
+static const struct backlight_ops led_bl_ops = {
+	.update_status	= led_bl_update_status,
+};
+
+static int led_bl_get_leds(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	int i, nb_leds, ret;
+	struct device_node *node = dev->of_node;
+	struct led_classdev **leds;
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+
+	ret = of_count_phandle_with_args(node, "leds", NULL);
+	if (ret < 0) {
+		dev_err(dev, "Unable to get led count\n");
+		return -EINVAL;
+	}
+
+	nb_leds = ret;
+	if (nb_leds < 1) {
+		dev_err(dev, "At least one LED must be specified!\n");
+		return -EINVAL;
+	}
+
+	leds = devm_kzalloc(dev, sizeof(struct led_classdev *) * nb_leds,
+			    GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	for (i = 0; i < nb_leds; i++) {
+		leds[i] = devm_led_get(dev, i);
+		if (IS_ERR(leds[i]))
+			return PTR_ERR(leds[i]);
+	}
+
+	/* check that the LEDs all have the same brightness range */
+	max_brightness = leds[0]->max_brightness;
+	for (i = 1; i < nb_leds; i++) {
+		if (max_brightness != leds[i]->max_brightness) {
+			dev_err(dev, "LEDs must have identical ranges\n");
+			return -EINVAL;
+		}
+	}
+
+	/* get the default brightness from the first LED from the list */
+	default_brightness = leds[0]->brightness;
+
+	priv->nb_leds = nb_leds;
+	priv->leds = leds;
+	priv->max_brightness = max_brightness;
+	priv->default_brightness = default_brightness;
+
+	return 0;
+}
+
+static int led_bl_parse_levels(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels > 1) {
+		int i;
+		unsigned int db;
+		u32 *levels = NULL;
+
+		levels = devm_kzalloc(dev, sizeof(u32) * num_levels,
+				      GFP_KERNEL);
+		if (!levels)
+			return -ENOMEM;
+
+		ret = of_property_read_u32_array(node, "brightness-levels",
+						levels,
+						num_levels);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * Try to map actual LED brightness to backlight brightness
+		 * level
+		 */
+		db = priv->default_brightness;
+		for (i = 0 ; i < num_levels; i++) {
+			if ((i && db > levels[i-1]) && db <= levels[i])
+				break;
+		}
+		priv->default_brightness = i;
+		priv->max_brightness = num_levels - 1;
+		priv->levels = levels;
+	} else if (num_levels >= 0)
+		dev_warn(dev, "Not enough levels defined\n");
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (!ret && value <= priv->max_brightness)
+		priv->default_brightness = value;
+	else if (!ret  && value > priv->max_brightness)
+		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->dev = &pdev->dev;
+
+	ret = led_bl_get_leds(&pdev->dev, priv);
+	if (ret)
+		return ret;
+
+	ret = led_bl_parse_levels(&pdev->dev, priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to parse DT data\n");
+		return ret;
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	props.brightness = priv->default_brightness;
+	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
+		      FB_BLANK_UNBLANK;
+	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
+			&pdev->dev, priv, &led_bl_ops, &props);
+	if (IS_ERR(priv->bl_dev)) {
+		dev_err(&pdev->dev, "Failed to register backlight\n");
+		return PTR_ERR(priv->bl_dev);
+	}
+
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+}
+
+static int led_bl_remove(struct platform_device *pdev)
+{
+	struct led_bl_data *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl = priv->bl_dev;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+
+	return 0;
+}
+
+static const struct of_device_id led_bl_of_match[] = {
+	{ .compatible = "led-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, led_bl_of_match);
+
+static struct platform_driver led_bl_driver = {
+	.driver		= {
+		.name		= "led-backlight",
+		.of_match_table	= of_match_ptr(led_bl_of_match),
+	},
+	.probe		= led_bl_probe,
+	.remove		= led_bl_remove,
+};
+
+module_platform_driver(led_bl_driver);
+
+MODULE_DESCRIPTION("LED based Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:led-backlight");
-- 
2.17.1


^ permalink raw reply related

* [PATCH v4 0/4] Add a generic driver for LED-based backlight
From: Jean-Jacques Hiblot @ 2019-07-17 14:15 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, jingoohan1
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

This series aims to add a led-backlight driver, similar to pwm-backlight,
but using a LED class device underneath.

A few years ago (2015), Tomi Valkeinen posted a series implementing a
backlight driver on top of a LED device:
https://patchwork.kernel.org/patch/7293991/
https://patchwork.kernel.org/patch/7294001/
https://patchwork.kernel.org/patch/7293981/

The discussion stopped because Tomi lacked the time to work on it.

changes in v4:
- fix dev_err() messages and commit logs following the advices of Pavel
- cosmetic changes (indents, getting rid of  "? 1 : 0" in
  led_match_led_node())

changes in v3:
- dt binding: don't limit the brightness range to 0-255. Use the range of
  the underlying LEDs. as a side-effect, all LEDs must now have the same
  range
- driver: Adapt to dt binding update.
- driver: rework probe() for clarity and remove the remaining goto.

changes in v2:
- handle more than one LED.
- don't make the backlight device a child of the LED controller.
- make brightness-levels and default-brightness-level optional
- removed the option to use a GPIO enable.
- removed the option to use a regulator. It should be handled by the LED
  core
- don't make any change to the LED core (not needed anymore)

Jean-Jacques Hiblot (2):
  leds: Add managed API to get a LED from a device driver
  dt-bindings: backlight: Add led-backlight binding

Tomi Valkeinen (2):
  leds: Add of_led_get() and led_put()
  backlight: add led-backlight driver

 .../bindings/leds/backlight/led-backlight.txt |  28 ++
 drivers/leds/led-class.c                      |  92 ++++++
 drivers/video/backlight/Kconfig               |   7 +
 drivers/video/backlight/Makefile              |   1 +
 drivers/video/backlight/led_bl.c              | 268 ++++++++++++++++++
 include/linux/leds.h                          |   6 +
 6 files changed, 402 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
 create mode 100644 drivers/video/backlight/led_bl.c

-- 
2.17.1


^ permalink raw reply

* [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, devicetree,
	Jean-Jacques Hiblot

This series makes it possible for the LED core to manage the power supply
of a LED. It uses the regulator API to disable/enable the power if when the
LED is turned on/off.
This is especially useful in situations where the LED driver/controller is
not supplying the power.

While at it, throw in a fix for led_set_brightness_sync() so that it can
work with drivers that don't provide brightness_set_blocking()

changes in v3:
- reword device-tree description
- reword commit log
- remove regulator updates from functions used in atomic context. If the
  regulator must be updated, it is defered to a workqueue.
- Fix led_set_brightness_sync() to work with the non-blocking function
  __led_set_brightness()

changes in v2:
- use devm_regulator_get_optional() to avoid using the dummy regulator and
  do some unnecessary work

Jean-Jacques Hiblot (3):
  dt-bindings: leds: document the "power-supply" property
  leds: Add control of the voltage/current regulator to the LED core
  leds: Make led_set_brightness_sync() also use __led_set_brightness()

 .../devicetree/bindings/leds/common.txt       |  4 ++
 drivers/leds/led-class.c                      | 15 ++++++
 drivers/leds/led-core.c                       | 53 +++++++++++++++++--
 drivers/leds/leds.h                           |  1 +
 include/linux/leds.h                          |  4 ++
 5 files changed, 73 insertions(+), 4 deletions(-)

-- 
2.17.1


^ permalink raw reply

* [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness()
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, devicetree,
	Jean-Jacques Hiblot
In-Reply-To: <20190717135948.19340-1-jjhiblot@ti.com>

There are some LED drivers that do not implement brightness_set_blocking(),
for those drivers led_set_brightness_sync() cannot work.
Fixing it by calling first __led_set_brightness() and falling back to
__led_set_brightness_blocking() if it failed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index dab32cf778f2..4a0506081c0e 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -320,15 +320,19 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
 		return -EBUSY;
 
-	led_cdev->brightness = min(value, led_cdev->max_brightness);
+	value = min(value, led_cdev->max_brightness);
 
 	if (led_cdev->flags & LED_SUSPENDED)
 		return 0;
 
-	ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+	ret = __led_set_brightness(led_cdev, value);
+	if (ret == -ENOTSUPP)
+		ret = __led_set_brightness_blocking(led_cdev, value);
 	if (ret)
 		return ret;
 
+	led_cdev->brightness = value;
+
 	return __led_handle_regulator(led_cdev, led_cdev->brightness);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
-- 
2.17.1


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox