* [PATCH v7 0/2] Subject: [PATCH v7 0/2] leds: Add control of the voltage/current regulator to the LED core @ 2019-10-21 17:47 Jean-Jacques Hiblot 2019-10-21 17:47 ` [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-10-21 17:47 ` [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 0 siblings, 2 replies; 6+ messages in thread From: Jean-Jacques Hiblot @ 2019-10-21 17:47 UTC (permalink / raw) To: jacek.anaszewski, pavel Cc: linux-leds, linux-kernel, tomi.valkeinen, jjhiblot 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. Because updating a regulator state can block, it is always a defered job. Note: this series relies on led_cdev->dev->of_node being populated [0] [0] https://lkml.org/lkml/2019/10/3/139 changes in v7: - Add sysfs interface to control the auto-off feature changes in v6: - Introduce a new property in DT binding to delay turning OFF the regulator The idea is to keep the regulator ON for some time after the LED is turned off in order to not change the regulator state when the LED is blinking. - Use an atomic to track the state of the regulator to ensure consistency. - Remove changes in led_set_brightness_sync(). changes in v5: - fixed build error in led_set_brightness_sync(). Explain the role of flush__work() changes in v4: - Add a new patch to make led_set_brightness_sync() use led_set_brightness_nosleep() and then wait the work to be done - Rework how the core knows how the regulator needs to be updated. 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 (2): dt-bindings: leds: document the "power-supply" property leds: Add control of the voltage/current regulator to the LED core .../devicetree/bindings/leds/common.txt | 14 ++ drivers/leds/led-class.c | 156 +++++++++++++++++- drivers/leds/led-core.c | 129 ++++++++++++++- drivers/leds/leds.h | 18 ++ include/linux/leds.h | 9 + 5 files changed, 318 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property 2019-10-21 17:47 [PATCH v7 0/2] Subject: [PATCH v7 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-10-21 17:47 ` Jean-Jacques Hiblot 2019-10-24 22:51 ` Rob Herring 2019-10-21 17:47 ` [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 1 sibling, 1 reply; 6+ messages in thread From: Jean-Jacques Hiblot @ 2019-10-21 17:47 UTC (permalink / raw) To: jacek.anaszewski, pavel Cc: linux-leds, linux-kernel, tomi.valkeinen, jjhiblot, devicetree Most of the LEDs are powered by a voltage/current regulator. Describing it in the device-tree makes it possible for the LED core to enable/disable it when needed. Cc: devicetree@vger.kernel.org To: robh+dt@kernel.org To: mark.rutland@arm.com Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- Documentation/devicetree/bindings/leds/common.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 6d44f0c38487..dacaf863d687 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -96,6 +96,19 @@ Optional properties for child nodes: - panic-indicator : This property specifies that the LED should be used, if at all possible, as a panic indicator. +- power-supply : A voltage/current regulator used to to power the LED. When a + LED is turned off, the LED core disable its regulator. The + same regulator can power many LED (or other) devices. It is + turned off only when all of its users disabled it. + +- power-off-delays-ms: This property specifies the delay between the time a LED + is turned off and the time the regulator is turned off. + It can be used to limit the overhead of the regulator + handling if the LED is toggling fast. + ex: if power-off-delays-ms is set to 500 ms, the + regulator will not be turned off until the LED is turned + off for more than 500ms. + - trigger-sources : List of devices which should be used as a source triggering this LED activity. Some LEDs can be related to a specific device and should somehow indicate its state. E.g. USB 2.0 @@ -143,6 +156,7 @@ led-controller@0 { function = LED_FUNCTION_STATUS; linux,default-trigger = "heartbeat"; gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>; + power-supply = <&led_regulator>; }; led1 { -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property 2019-10-21 17:47 ` [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot @ 2019-10-24 22:51 ` Rob Herring 0 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2019-10-24 22:51 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: jacek.anaszewski, pavel, linux-leds, linux-kernel, tomi.valkeinen, jjhiblot, devicetree On Mon, 21 Oct 2019 19:47:50 +0200, Jean-Jacques Hiblot wrote: > Most of the LEDs are powered by a voltage/current regulator. Describing it > in the device-tree makes it possible for the LED core to enable/disable it > when needed. > > Cc: devicetree@vger.kernel.org > To: robh+dt@kernel.org > To: mark.rutland@arm.com > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > Documentation/devicetree/bindings/leds/common.txt | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-10-21 17:47 [PATCH v7 0/2] Subject: [PATCH v7 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-10-21 17:47 ` [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot @ 2019-10-21 17:47 ` Jean-Jacques Hiblot 2019-12-04 12:37 ` Pavel Machek 1 sibling, 1 reply; 6+ messages in thread From: Jean-Jacques Hiblot @ 2019-10-21 17:47 UTC (permalink / raw) To: jacek.anaszewski, pavel Cc: linux-leds, linux-kernel, tomi.valkeinen, jjhiblot A LED is usually powered by a voltage/current regulator. Let the LED core know about it. This allows the LED core to turn on or off the power supply as needed. Because turning ON/OFF a regulator might block, it is not done synchronously but done in a workqueue. Turning ON the regulator is always done as soon as possible, turning it off can be delayed by setting the "power-off-delays-ms" property. The intent is to keep the regulator powered on when the blink rate of the LED is high. A sysfs interface is made available to dynamically enable/disable this feature and tune the off-delay. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/leds/led-class.c | 156 +++++++++++++++++++++++++++++++++++++-- drivers/leds/led-core.c | 129 +++++++++++++++++++++++++++++++- drivers/leds/leds.h | 18 +++++ include/linux/leds.h | 9 +++ 4 files changed, 304 insertions(+), 8 deletions(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index c5ff1d3d00a3..c1ad89d56129 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -25,6 +25,112 @@ static struct class *leds_class; +static ssize_t regulator_auto_off_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + + return sprintf(buf, "%s\n", + led_cdev->reg_auto_off ? "enabled" : "disabled"); +} + +static ssize_t regulator_auto_off_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t size) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + ssize_t ret = size; + bool auto_off; + + if (strncmp(buf, "enable\n", size) == 0) + auto_off = true; + else if (strncmp(buf, "disable\n", size) == 0) + auto_off = false; + else + return -EINVAL; + + mutex_lock(&led_cdev->led_access); + + if (led_sysfs_is_disabled(led_cdev)) { + ret = -EBUSY; + goto unlock; + } + + /* no changes ? */ + if (led_cdev->reg_auto_off == auto_off) + goto unlock; + led_cdev->reg_auto_off = auto_off; + + /* cancel a possibly pending auto-off operation */ + if (!auto_off) + cancel_delayed_work(&led_cdev->reg_off_work); + + /* + * call led_set_brightness_nopm() to trigger an update of the + * regulator's state. + */ + led_set_brightness_nopm(led_cdev, led_cdev->brightness); + flush_work(&led_cdev->set_brightness_work); + +unlock: + mutex_unlock(&led_cdev->led_access); + return ret; +} +static DEVICE_ATTR_RW(regulator_auto_off); + +static ssize_t regulator_auto_off_delay_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + + return sprintf(buf, "%u ms\n", led_cdev->reg_off_delay); +} + +static ssize_t regulator_auto_off_delay_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t size) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + unsigned long delay; + ssize_t ret; + + mutex_lock(&led_cdev->led_access); + + if (led_sysfs_is_disabled(led_cdev)) { + ret = -EBUSY; + goto unlock; + } + + ret = kstrtoul(buf, 10, &delay); + if (ret) + goto unlock; + ret = size; + + if (led_cdev->reg_off_delay == delay) + goto unlock; + led_cdev->reg_off_delay = delay; + + if (!led_cdev->reg_auto_off) + goto unlock; + + /* + * If a auto-off operation is in progress cancel it, + * and re-trigger it with the new delay + */ + if (cancel_delayed_work(&led_cdev->reg_off_work)) + schedule_delayed_work(&led_cdev->reg_off_work, + msecs_to_jiffies(led_cdev->reg_off_delay)); +unlock: + mutex_unlock(&led_cdev->led_access); + return ret; +} +static DEVICE_ATTR_RW(regulator_auto_off_delay); + +static struct attribute *auto_off_attrs[] = { + &dev_attr_regulator_auto_off.attr, + &dev_attr_regulator_auto_off_delay.attr, + NULL +}; +ATTRIBUTE_GROUPS(auto_off); + static ssize_t brightness_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -351,6 +457,7 @@ int led_classdev_register_ext(struct device *parent, char final_name[LED_MAX_NAME_SIZE]; const char *proposed_name = composed_name; int ret; + struct regulator *regulator; if (init_data) { if (init_data->devname_mandatory && !init_data->devicename) { @@ -385,14 +492,38 @@ int led_classdev_register_ext(struct device *parent, 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 (regulator != ERR_PTR(-ENODEV)) { + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n", + led_cdev->name); + ret = PTR_ERR(regulator); + goto error; + } + led_cdev->regulator = NULL; + } else { + mutex_init(&led_cdev->regulator_access); + led_cdev->regulator = regulator; + if (fwnode_property_read_u32(init_data->fwnode, "off-delay-ms", + &led_cdev->reg_off_delay)) { + led_cdev->reg_auto_off = false; + atomic_set(&led_cdev->regulator_state, REG_R_ON_U_OFF); + ret = regulator_enable(led_cdev->regulator); + if (ret) + goto error; + } else { + led_cdev->reg_auto_off = true; + atomic_set(&led_cdev->regulator_state, REG_R_OFF_U_OFF); + } + ret = device_add_groups(led_cdev->dev, auto_off_groups); + if (ret) + goto error; + } + if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) { ret = led_add_brightness_hw_changed(led_cdev); - if (ret) { - device_unregister(led_cdev->dev); - led_cdev->dev = NULL; - mutex_unlock(&led_cdev->led_access); - return ret; - } + if (ret) + goto error; } led_cdev->work_flags = 0; @@ -424,7 +555,14 @@ int led_classdev_register_ext(struct device *parent, led_cdev->name); return 0; + +error: + device_unregister(led_cdev->dev); + led_cdev->dev = NULL; + mutex_unlock(&led_cdev->led_access); + return ret; } + EXPORT_SYMBOL_GPL(led_classdev_register_ext); /** @@ -450,6 +588,7 @@ void led_classdev_unregister(struct led_classdev *led_cdev) /* Stop blinking */ led_stop_software_blink(led_cdev); + led_cdev->reg_off_delay = 0; led_set_brightness(led_cdev, LED_OFF); flush_work(&led_cdev->set_brightness_work); @@ -464,6 +603,11 @@ void led_classdev_unregister(struct led_classdev *led_cdev) up_write(&leds_list_lock); mutex_destroy(&led_cdev->led_access); + if (led_cdev->regulator) { + if (!led_cdev->reg_auto_off) + regulator_disable(led_cdev->regulator); + mutex_destroy(&led_cdev->regulator_access); + } } EXPORT_SYMBOL_GPL(led_classdev_unregister); diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index f1f718dbe0f8..d2253524670d 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -37,6 +37,74 @@ const char * const led_colors[LED_COLOR_ID_MAX] = { }; EXPORT_SYMBOL_GPL(led_colors); +static void turn_off_regulator_if_needed(struct led_classdev *led_cdev) +{ + int old = atomic_cmpxchg(&led_cdev->regulator_state, REG_R_ON_U_OFF, + REG_R_OFF_U_OFF); + + if (old == REG_R_ON_U_OFF) { + int rc = regulator_disable(led_cdev->regulator); + + if (rc) { + dev_err(led_cdev->dev, + "Failed to disable the regulator (%d)\n", rc); + atomic_or(REG_R_ON, &led_cdev->regulator_state); + } + } +} + +static void try_turn_on_regulator_if_needed(struct led_classdev *led_cdev) +{ + int old = atomic_cmpxchg(&led_cdev->regulator_state, REG_R_OFF_U_ON, + REG_R_ON_U_ON); + if (old == REG_R_OFF_U_ON) { + int rc = regulator_enable(led_cdev->regulator); + + if (rc) { + dev_err(led_cdev->dev, + "Failed to enable the regulator (%d)\n", rc); + atomic_and(~REG_R_ON, &led_cdev->regulator_state); + } + } +} + +static void turn_off_regulator_delayed(struct work_struct *ws) +{ + struct led_classdev *led_cdev = + container_of(ws, struct led_classdev, reg_off_work.work); + + /* + * Take the lock to prevent very unlikely updates due to + * led_handle_regulator() being executed at the same time + */ + mutex_lock(&led_cdev->regulator_access); + + turn_off_regulator_if_needed(led_cdev); + + mutex_unlock(&led_cdev->regulator_access); +} + +static void led_handle_regulator(struct led_classdev *led_cdev) +{ + int delay = (led_cdev->flags & LED_SUSPENDED) ? 0 : + led_cdev->reg_off_delay; + + if (!led_cdev->regulator) + return; + + /* + * Take the lock to prevent very unlikely updates due to + * turn_off_regulator_delayed() being executed at the same time + */ + mutex_lock(&led_cdev->regulator_access); + + try_turn_on_regulator_if_needed(led_cdev); + if (!delay) + turn_off_regulator_if_needed(led_cdev); + + mutex_unlock(&led_cdev->regulator_access); +} + static int __led_set_brightness(struct led_classdev *led_cdev, enum led_brightness value) { @@ -135,6 +203,8 @@ static void set_brightness_delayed(struct work_struct *ws) (led_cdev->flags & LED_HW_PLUGGABLE))) dev_err(led_cdev->dev, "Setting an LED's brightness failed (%d)\n", ret); + + led_handle_regulator(led_cdev); } static void led_set_software_blink(struct led_classdev *led_cdev, @@ -189,6 +259,7 @@ static void led_blink_setup(struct led_classdev *led_cdev, void led_init_core(struct led_classdev *led_cdev) { INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed); + INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed); timer_setup(&led_cdev->blink_timer, led_timer_function, 0); } @@ -269,8 +340,62 @@ EXPORT_SYMBOL_GPL(led_set_brightness); 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)) + bool defer_work = false; + bool regulator_on = !!value; + + + if (led_cdev->regulator) { + int old; + + /* + * If the regulator must not be turned OFF automatically, + * make sure it is turned ON. + */ + if (!led_cdev->reg_auto_off) + regulator_on = true; + + if (regulator_on) + old = atomic_fetch_or(REG_U_ON, + &led_cdev->regulator_state); + else + old = atomic_fetch_and(~REG_U_ON, + &led_cdev->regulator_state); + + if (!regulator_on && old == REG_R_ON_U_ON) { + int delay = (led_cdev->flags & LED_SUSPENDED) ? 0 : + led_cdev->reg_off_delay; + + /* + * the regulator must be turned off. This cannot be + * here because we can't sleep. Defer the job or, if a + * delay is asked for, schedule a delayed work + */ + if (delay) + schedule_delayed_work(&led_cdev->reg_off_work, + msecs_to_jiffies(delay)); + else + defer_work = true; + } else if (regulator_on && old == REG_R_OFF_U_OFF) { + /* + * the regulator must be enabled. This cannot be here + * because we can't sleep. Defer the job + */ + defer_work = true; + } + /* + * small optimization. Cancel the work that had been started + * to turn OFF the regulator. It wouldn't have been turned off + * anyway because regulator_state has changed to REG_R_x_U_ON + */ + if (regulator_on && old == REG_R_ON_U_OFF) + cancel_delayed_work(&led_cdev->reg_off_work); + } + + /* + * If defered work is not asked for, use brightness_set + * op now if available, it is guaranteed not to sleep + */ + if (!defer_work && !__led_set_brightness(led_cdev, value)) return; /* If brightness setting can sleep, delegate it to a work queue task */ diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index 0b577cece8f7..5cb76e547b7d 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -11,6 +11,24 @@ #include <linux/rwsem.h> #include <linux/leds.h> +#include <linux/regulator/consumer.h> + +/* + * The regulator state tracks 2 boolean variables: + * - the state of regulator (or more precisely the state required by + * led core layer, as many users can interact with the same regulator). + * It is tracked by bit 0. + * - the state last asked-for by the LED user. It is tracked by bit 1. + */ +#define REG_R_ON BIT(0) +#define REG_U_ON BIT(1) + +enum { REG_R_OFF_U_OFF = 0, + REG_R_ON_U_OFF = REG_R_ON, + REG_R_OFF_U_ON = REG_U_ON, + REG_R_ON_U_ON = REG_R_ON | REG_U_ON +}; + static inline int led_get_brightness(struct led_classdev *led_cdev) { diff --git a/include/linux/leds.h b/include/linux/leds.h index 9b94cf752012..b0a4d1f56825 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -149,6 +149,15 @@ struct led_classdev { /* Ensures consistent access to the LED Flash Class device */ struct mutex led_access; + + /* regulator */ + struct regulator *regulator; + struct mutex regulator_access; + atomic_t regulator_state; + bool reg_auto_off; + int reg_off_delay; + struct delayed_work reg_off_work; + }; /** -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-10-21 17:47 ` [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot @ 2019-12-04 12:37 ` Pavel Machek 2020-04-24 12:47 ` Tomi Valkeinen 0 siblings, 1 reply; 6+ messages in thread From: Pavel Machek @ 2019-12-04 12:37 UTC (permalink / raw) To: Jean-Jacques Hiblot Cc: jacek.anaszewski, linux-leds, linux-kernel, tomi.valkeinen [-- Attachment #1: Type: text/plain, Size: 3097 bytes --] Hi! > A LED is usually powered by a voltage/current regulator. Let the LED core > know about it. This allows the LED core to turn on or off the power supply > as needed. > > Because turning ON/OFF a regulator might block, it is not done > synchronously but done in a workqueue. Turning ON the regulator is > always How will this interact with LEDs that can be used from atomic context? > +static ssize_t regulator_auto_off_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + ssize_t ret = size; > + bool auto_off; > + > + if (strncmp(buf, "enable\n", size) == 0) > + auto_off = true; > + else if (strncmp(buf, "disable\n", size) == 0) > + auto_off = false; > + else > + return -EINVAL; Sounds like device power management to me. Is it compatible with that? > @@ -135,6 +203,8 @@ static void set_brightness_delayed(struct work_struct *ws) > (led_cdev->flags & LED_HW_PLUGGABLE))) > dev_err(led_cdev->dev, > "Setting an LED's brightness failed (%d)\n", ret); > + > + led_handle_regulator(led_cdev); > } > You only modify set_brigthness_delays, so this will not work at all for non-blocking LEDs, right? > static void led_set_software_blink(struct led_classdev *led_cdev, > @@ -189,6 +259,7 @@ static void led_blink_setup(struct led_classdev *led_cdev, > void led_init_core(struct led_classdev *led_cdev) > { > INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed); > + INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed); > Could this re-use the workqueue? Many systems will not need regulators, so this is overhead... > + /* > + * the regulator must be turned off. This cannot be Use "The", and fix double spaces between must and be. > + } else if (regulator_on && old == REG_R_OFF_U_OFF) { > + /* > + * the regulator must be enabled. This cannot be here "The" > + /* > + * small optimization. Cancel the work that had been started "Small." > +#include <linux/regulator/consumer.h> > + > +/* > + * The regulator state tracks 2 boolean variables: > + * - the state of regulator (or more precisely the state required by > + * led core layer, as many users can interact with the same regulator). > + * It is tracked by bit 0. > + * - the state last asked-for by the LED user. It is tracked by bit 1. > + */ > +#define REG_R_ON BIT(0) > +#define REG_U_ON BIT(1) > + > +enum { REG_R_OFF_U_OFF = 0, > + REG_R_ON_U_OFF = REG_R_ON, > + REG_R_OFF_U_ON = REG_U_ON, > + REG_R_ON_U_ON = REG_R_ON | REG_U_ON > +}; That's quite weird use of enum. > +++ b/include/linux/leds.h > @@ -149,6 +149,15 @@ struct led_classdev { > > /* Ensures consistent access to the LED Flash Class device */ > struct mutex led_access; > + > + /* regulator */ "Regulator". Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core 2019-12-04 12:37 ` Pavel Machek @ 2020-04-24 12:47 ` Tomi Valkeinen 0 siblings, 0 replies; 6+ messages in thread From: Tomi Valkeinen @ 2020-04-24 12:47 UTC (permalink / raw) To: Pavel Machek, jacek.anaszewski, Dan Murphy; +Cc: linux-leds, linux-kernel Hi, On 04/12/2019 14:37, Pavel Machek wrote: > Hi! > >> A LED is usually powered by a voltage/current regulator. Let the LED core >> know about it. This allows the LED core to turn on or off the power supply >> as needed. >> >> Because turning ON/OFF a regulator might block, it is not done >> synchronously but done in a workqueue. Turning ON the regulator is >> always JJ had to leave this unfinished, and now I'm trying to pick this work up. > How will this interact with LEDs that can be used from atomic context? I think the idea here is that if the LED uses a regulator, and the regulator needs to be enabled, then the whole work is scheduled. If there's no regulator or if the regulator is already enabled, the brightness is set directly. >> +static ssize_t regulator_auto_off_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t size) >> +{ >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >> + ssize_t ret = size; >> + bool auto_off; >> + >> + if (strncmp(buf, "enable\n", size) == 0) >> + auto_off = true; >> + else if (strncmp(buf, "disable\n", size) == 0) >> + auto_off = false; >> + else >> + return -EINVAL; > > Sounds like device power management to me. Is it compatible with that? Can you elaborate what you mean? >> @@ -135,6 +203,8 @@ static void set_brightness_delayed(struct work_struct *ws) >> (led_cdev->flags & LED_HW_PLUGGABLE))) >> dev_err(led_cdev->dev, >> "Setting an LED's brightness failed (%d)\n", ret); >> + >> + led_handle_regulator(led_cdev); >> } >> > > You only modify set_brigthness_delays, so this will not work at all > for non-blocking LEDs, right? I'm not that familiar with led framework yet, but if setting of the brightness always goes via led_set_brightness_nopm(), then I think this would work for all LEDs, as led_set_brightness_nopm will schedule the work if needed (which goes to set_brightness_delayed). >> static void led_set_software_blink(struct led_classdev *led_cdev, >> @@ -189,6 +259,7 @@ static void led_blink_setup(struct led_classdev *led_cdev, >> void led_init_core(struct led_classdev *led_cdev) >> { >> INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed); >> + INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed); >> > > Could this re-use the workqueue? Many systems will not need > regulators, so this is overhead... You mean re-use the 'set_brightness_work' work? I'm not sure how that would be done, they're a bit different things. set_brightness_work is used to change the context from atomic to non-atomic, and reg_off_work is used to turn the regulator off after a specified delay. >> + /* >> + * the regulator must be turned off. This cannot be > > Use "The", and fix double spaces between must and be. > >> + } else if (regulator_on && old == REG_R_OFF_U_OFF) { >> + /* >> + * the regulator must be enabled. This cannot be here > > "The" > >> + /* >> + * small optimization. Cancel the work that had been started > > "Small." > >> +#include <linux/regulator/consumer.h> >> + >> +/* >> + * The regulator state tracks 2 boolean variables: >> + * - the state of regulator (or more precisely the state required by >> + * led core layer, as many users can interact with the same regulator). >> + * It is tracked by bit 0. >> + * - the state last asked-for by the LED user. It is tracked by bit 1. >> + */ >> +#define REG_R_ON BIT(0) >> +#define REG_U_ON BIT(1) >> + >> +enum { REG_R_OFF_U_OFF = 0, >> + REG_R_ON_U_OFF = REG_R_ON, >> + REG_R_OFF_U_ON = REG_U_ON, >> + REG_R_ON_U_ON = REG_R_ON | REG_U_ON >> +}; > > That's quite weird use of enum. Yes, these could as well be defines, if that's what you mean. >> +++ b/include/linux/leds.h >> @@ -149,6 +149,15 @@ struct led_classdev { >> >> /* Ensures consistent access to the LED Flash Class device */ >> struct mutex led_access; >> + >> + /* regulator */ > > "Regulator". Fixed this and the other cosmetic changes, thanks! Overall, I wonder if all this is worth it as this is somewhat complex change, compared to just having the regulator always enabled... Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-24 12:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-21 17:47 [PATCH v7 0/2] Subject: [PATCH v7 0/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-10-21 17:47 ` [PATCH v7 1/2] dt-bindings: leds: document the "power-supply" property Jean-Jacques Hiblot 2019-10-24 22:51 ` Rob Herring 2019-10-21 17:47 ` [PATCH v7 2/2] leds: Add control of the voltage/current regulator to the LED core Jean-Jacques Hiblot 2019-12-04 12:37 ` Pavel Machek 2020-04-24 12:47 ` Tomi Valkeinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).