Intel Cherry Trail Whiskey Cove LEDs support

Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not.

This driver was tested with Lenovo Yoga Book notebook.

Signed-off-by Yauhen Kharuzhy <[email protected]>

---
  drivers/leds/Kconfig          |  11 ++
  drivers/leds/Makefile         |   1 +
  drivers/leds/leds-cht-wcove.c | 293 ++++++++++++++++++++++++++++++++++
  3 files changed, 305 insertions(+)

create mode 100644 drivers/leds/leds-cht-wcove.c

drivers/leds/Kconfig +11 -0

index a72f97fca57b..8f50f38af57e 100644
@@ -106,6 +106,17 @@ config LEDS_BCM6358
  	  This option enables support for LEDs connected to the BCM6358
  	  LED HW controller accessed via MMIO registers.
 
+config LEDS_CHT_WCOVE
+	tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC"
+	depends on LEDS_CLASS
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This option enables support for charger and general purpose LEDs
+	  connected to the Intel Cherrytrail Whiskey Cove PMIC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-cht-wcove.
+
  config LEDS_CPCAP
  	tristate "LED Support for Motorola CPCAP"
  	depends on LEDS_CLASS

drivers/leds/Makefile +1 -0

index 4c1b0054f379..1c1995d3441c 100644
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
+obj-$(CONFIG_LEDS_CHT_WCOVE)		+= leds-cht-wcove.o
  obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
  obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
  obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o

drivers/leds/leds-cht-wcove.c new file +33 -0

new file mode 100644
index 000000000000..ee4287f8e806
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Driver for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC
+//
+// Copyright 2019 Yauhen Kharuzhy <[email protected]>
+//
+// Based on Lenovo Yoga Book Android kernel sources
+
+#include <linux/kernel.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define CHT_WC_LED1_CTRL		0x5e1f
+#define CHT_WC_LED1_FSM			0x5e20
+#define CHT_WC_LED1_PWM			0x5e21
+
+#define CHT_WC_LED2_CTRL		0x4fdf
+#define CHT_WC_LED2_FSM			0x4fe0
+#define CHT_WC_LED2_PWM			0x4fe1
+
+/* HW or SW control of charging led */
+#define CHT_WC_LED1_SWCTL		BIT(0)
+#define CHT_WC_LED1_ON			BIT(1)
+
+#define CHT_WC_LED2_ON			BIT(0)
+#define CHT_WC_LED_I_MA2_5		(2 << 2)
+/* LED current limit */
+#define CHT_WC_LED_I_MASK		GENMASK(3, 2)
+

drivers/leds/leds-cht-wcove.c new file +250 -0

+
+struct cht_wc_led {
+	struct led_classdev cdev;
+	struct intel_soc_pmic *pmic;
+	const char *name;
+	const char *default_trigger;
+	struct mutex mutex;
+	u16	ctrl_reg;
+	u8	enable_mask;
+	u16	fsm_reg;
+	u16	pwm_reg;
+};
+
+static struct cht_wc_led cht_wc_leds[] = {
+	{
+		.name = "platform::charging",
+		.default_trigger = "bq27542-0-charging-blink-full-solid",
+		.ctrl_reg = CHT_WC_LED1_CTRL,
+		.fsm_reg = CHT_WC_LED1_FSM,
+		.pwm_reg = CHT_WC_LED1_PWM,
+		.enable_mask = CHT_WC_LED1_ON,
+	},
+	{
+		.name = "status-led::",
+		.default_trigger = "default-on",
+		.ctrl_reg = CHT_WC_LED2_CTRL,
+		.fsm_reg = CHT_WC_LED2_FSM,
+		.pwm_reg = CHT_WC_LED2_PWM,
+		.enable_mask = CHT_WC_LED2_ON,
+	},
+};
+
+static int cht_wc_leds_brightness_set(struct led_classdev *cdev,
+				      enum led_brightness value)
+{
+	struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+	int ret;
+
+	mutex_lock(&led->mutex);
+
+	if (!value) {
+		ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+					 led->enable_mask, 0);
+		if (ret)
+			dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
+
+		ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+					 CHT_WC_LED_EFF_MASK,
+					 CHT_WC_LED_EFF_ON);
+		if (ret < 0)
+			dev_err(cdev->dev,
+				"Failed to update LED FSM reg: %d\n", ret);
+	} else {
+		ret = regmap_write(led->pmic->regmap, led->pwm_reg, value);
+		if (ret)
+			dev_err(cdev->dev,
+				"Failed to set brightness: %d\n", ret);
+
+		ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+					 led->enable_mask, led->enable_mask);
+		if (ret)
+			dev_err(cdev->dev, "Failed to turn on: %d\n", ret);
+	}
+
+	mutex_unlock(&led->mutex);
+
+	return ret;
+}
+
+enum led_brightness cht_wc_leds_brightness_get(struct led_classdev *cdev)
+{
+	struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+	int ret;
+	unsigned int val;
+
+	mutex_lock(&led->mutex);
+
+	ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &val);
+	if (ret < 0) {
+		dev_err(cdev->dev, "Failed to read LED CTRL reg: %d\n", ret);
+		ret = LED_OFF;
+		goto done;
+	}
+
+	val &= led->enable_mask;
+
+	if (!val) {
+		ret = LED_OFF;
+		goto done;
+	}
+
+	ret = regmap_read(led->pmic->regmap, led->pwm_reg, &val);
+	if (ret < 0) {
+		dev_err(cdev->dev, "Failed to read LED PWM reg: %d\n", ret);
+		ret = LED_ON;
+		goto done;
+	}
+
+	ret = val;
+done:
+	mutex_unlock(&led->mutex);
+
+	return ret;
+}
+
+/* Return blinking period for given CTRL reg value */
+static unsigned long cht_wc_leds_get_period(int ctrl)
+{
+	ctrl &= CHT_WC_LED_F_MASK;
+
+	switch (ctrl) {
+	case CHT_WC_LED_F_1_4_HZ:
+		return 1000 * 4;
+	case CHT_WC_LED_F_1_2_HZ:
+		return 1000 * 2;
+	case CHT_WC_LED_F_1_HZ:
+		return 1000;
+	case CHT_WC_LED_F_2_HZ:
+		return 1000 / 2;
+	};
+
+	return 0;
+}
+
+/*
+ * Find suitable hardware blink mode for given period.
+ * period < 750 ms - select 2 HZ
+ * 750 ms <= period < 1500 ms - select 1 HZ
+ * 1500 ms <= period < 3000 ms - select 1/2 HZ
+ * 3000 ms <= period < 5000 ms - select 1/4 HZ
+ * 5000 ms <= period - return -1
+ */
+static int cht_wc_leds_find_freq(unsigned long period)
+{
+	if (period < 750)
+		return CHT_WC_LED_F_2_HZ;
+	else if (period < 1500)
+		return CHT_WC_LED_F_1_HZ;
+	else if (period < 3000)
+		return CHT_WC_LED_F_1_2_HZ;
+	else if (period < 5000)
+		return CHT_WC_LED_F_1_4_HZ;
+	else
+		return -1;
+}
+
+static int cht_wc_leds_blink_set(struct led_classdev *cdev,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
+	unsigned int ctrl;
+	int ret;
+
+	mutex_lock(&led->mutex);
+
+	if (!*delay_on && !*delay_off)
+		*delay_on = *delay_off = 1000;
+
+	ctrl = cht_wc_leds_find_freq(*delay_on + *delay_off);
+	if (ctrl < 0) {
+		/* Disable HW blinking */
+		ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+					 CHT_WC_LED_EFF_MASK,
+					 CHT_WC_LED_EFF_ON);
+		if (ret < 0)
+			dev_err(cdev->dev,
+				"Failed to update LED FSM reg: %d\n", ret);
+
+		/* Fallback to software timer */
+		*delay_on = *delay_off = 0;
+		ret = -EINVAL;
+		goto done;
+	}
+
+	ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg,
+				 CHT_WC_LED_EFF_MASK, CHT_WC_LED_EFF_BLINKING);
+	if (ret < 0)
+		dev_err(cdev->dev,
+			"Failed to update LED FSM reg: %d\n", ret);
+
+	ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg,
+				 CHT_WC_LED_F_MASK, ctrl);
+	if (ret < 0)
+		dev_err(cdev->dev,
+			"Failed to update LED CTRL reg: %d\n", ret);
+
+	*delay_off = *delay_on = cht_wc_leds_get_period(ctrl) / 2;
+
+done:
+	mutex_unlock(&led->mutex);
+
+	return ret;
+}
+
+static int cht_wc_leds_probe(struct platform_device *pdev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cht_wc_leds); i++) {
+		struct cht_wc_led *led = &cht_wc_leds[i];
+
+		led->pmic = pmic;
+		mutex_init(&led->mutex);
+		led->cdev.name = cht_wc_leds[i].name;
+		led->cdev.brightness_set_blocking = cht_wc_leds_brightness_set;
+		led->cdev.brightness_get = cht_wc_leds_brightness_get;
+		led->cdev.blink_set = cht_wc_leds_blink_set;
+		led->cdev.max_brightness = 255;
+		led->cdev.default_trigger = led->default_trigger;
+
+		ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = regmap_update_bits(pmic->regmap, CHT_WC_LED1_CTRL,
+				 CHT_WC_LED1_SWCTL, 1);
+
+	if (ret)
+		dev_err(&pdev->dev,
+			"Failed to set SW control bit for charger LED: %d\n",
+			ret);
+
+	platform_set_drvdata(pdev, cht_wc_leds);
+
+	return 0;
+}
+
+static const struct platform_device_id cht_wc_leds_table[] = {
+	{ .name = "cht_wcove_leds" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, cht_wc_leds_table);
+
+static struct platform_driver cht_wc_leds_driver = {
+	.probe = cht_wc_leds_probe,
+	.id_table = cht_wc_leds_table,
+	.driver = {
+		.name = "cht_wcove_leds",
+	},
+};
+module_platform_driver(cht_wc_leds_driver);
+
+MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove PMIC LEDs driver");
+MODULE_AUTHOR("Yauhen Kharuzhy <[email protected]>");
+MODULE_LICENSE("GPL v2");
+

Quoted 5 lines

Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not.

This driver was tested with Lenovo Yoga Book notebook.

Thank you for working on this. The CHT Whiskey Cove PMIC is also used on the GPD win and GPD pocket devices and there LED1 by default indicates the charging status.

Since your driver forces the LED into SWCTL mode on probe() this means that any kernel with it enabled will break the charging LEDs OOTB function, this is undesirable.

I believe it would be best to add a custom "mode" attribute to the led classdev, with "manual" and "on-when-charging" modes, this would then control bits 0-1 of reg 0x5e1f and by default these bits should be left as is when the driver loads.

Note that in my experience the "charging" mode only works when bits 0-1 have the value 10. I've some written notes from when I played with this myself and they say:

   -CHT WC powerled control 0x5e1f: bits 0-1:
    0: ????
    1: Off
    2: On when charging
    3: On
   -CHT WC powerled pattern control 0x5e20: bits 1-2:
    0: Off
    1: On
    2: Blinking
    3: Glowing

As for the 0x5e20 settings, I believe another custom sysfs attribute, called "breathing" would be a good idea to export the breathing functionality.

Hi!

Quoted 7 lines · Yauhen Kharuzhy

Quoted 5 lines

Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not.

This driver was tested with Lenovo Yoga Book notebook.

Quoted 7 lines

Thank you for working on this. The CHT Whiskey Cove PMIC is also used on the GPD win and GPD pocket devices and there LED1 by default indicates the charging status.

Since your driver forces the LED into SWCTL mode on probe() this means that any kernel with it enabled will break the charging LEDs OOTB function, this is undesirable.

Agreed.

Quoted 5 lines

I believe it would be best to add a custom "mode" attribute to the led classdev, with "manual" and "on-when-charging" modes, this would then control bits 0-1 of reg 0x5e1f and by default these bits should be left as is when the driver loads.

No. This is not first hardware when we have something like this, and we need something generic here.

One possibility would be magic "hardware drives this led" trigger. Hmm? (Jacek disliked this idea before, but maybe we can convince him).

Generic "is this driven by hardware or not" attribute might be possible, too... but its interaction with triggers/brightness/etc would be confusing.

Quoted 16 lines

Note that in my experience the "charging" mode only works when bits 0-1 have the value 10. I've some written notes from when I played with this myself and they say:

    -CHT WC powerled control 0x5e1f: bits 0-1:
     0: ????
     1: Off
     2: On when charging
     3: On
    -CHT WC powerled pattern control 0x5e20: bits 1-2:
     0: Off
     1: On
     2: Blinking
     3: Glowing

Quoted 3 lines

As for the 0x5e20 settings, I believe another custom sysfs attribute, called "breathing" would be a good idea to export the breathing functionality.

We have "pattern" trigger that can do this kind of stuff in software. But I'm not sure if this is worth supporting.

Quoted 11 lines

Agreed.

Quoted 5 lines

I believe it would be best to add a custom "mode" attribute to the led classdev, with "manual" and "on-when-charging" modes, this would then control bits 0-1 of reg 0x5e1f and by default these bits should be left as is when the driver loads.

No. This is not first hardware when we have something like this, and we need something generic here.

Quoted 7 lines

One possibility would be magic "hardware drives this led" trigger. Hmm? (Jacek disliked this idea before, but maybe we can convince him).

Generic "is this driven by hardware or not" attribute might be possible, too... but its interaction with triggers/brightness/etc would be confusing.

In this case the interaction is not that tricky, but it will likely be different per led controller, so I do not think that we can ever come up with a truely generic solution.

Basically the charge led has 3 states:

1) Off
2) On
3) On when charging

And then when on it has 4 patterns:

1) Permanently off (so still not really on)
2) Permanently on
3) Blinking
4) Breathing

These 4 patterns can be selected when on, independent of being perma-on or ondemand-on

Quoted 6 lines

Quoted 3 lines

As for the 0x5e20 settings, I believe another custom sysfs attribute, called "breathing" would be a good idea to export the breathing functionality.

We have "pattern" trigger that can do this kind of stuff in software. But I'm not sure if this is worth supporting.

The problem is that any changes made are permanent, they survice reboots and the default is Breathing, so we need a way to restore that which does not involve removing the internal battery of these devices.

Hi!

Quoted 29 lines

Quoted 11 lines

Agreed.

Quoted 5 lines

I believe it would be best to add a custom "mode" attribute to the led classdev, with "manual" and "on-when-charging" modes, this would then control bits 0-1 of reg 0x5e1f and by default these bits should be left as is when the driver loads.

No. This is not first hardware when we have something like this, and we need something generic here.

Quoted 7 lines

One possibility would be magic "hardware drives this led" trigger. Hmm? (Jacek disliked this idea before, but maybe we can convince him).

Generic "is this driven by hardware or not" attribute might be possible, too... but its interaction with triggers/brightness/etc would be confusing.

In this case the interaction is not that tricky, but it will likely be different per led controller, so I do not think that we can ever come up with a truely generic solution.

Basically the charge led has 3 states:

1) Off
2) On
3) On when charging

Quoted 6 lines

And then when on it has 4 patterns:

1) Permanently off (so still not really on)
2) Permanently on
3) Blinking
4) Breathing

Ok, so you don't really need to support _both_ off methods.

Still sounds like a normal LED, with special "yoga-charging" and "yoga-breathing" triggers. (All the normal triggers should still work, too.)

Quoted 2 lines

These 4 patterns can be selected when on, independent of being perma-on or ondemand-on

Yeah, but we don't really want to expose that to userspace.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

As for the 0x5e20 settings, I believe another custom sysfs attribute, called "breathing" would be a good idea to export the breathing functionality.

We have "pattern" trigger that can do this kind of stuff in software. But I'm not sure if this is worth supporting.

Quoted 4 lines

The problem is that any changes made are permanent, they survice reboots and the default is Breathing, so we need a way to restore that which does not involve removing the internal battery of these devices.

Wow. Now that's a broken hardware.

Anyway, in such case I'd propose having rmmod/reboot/poweroff hook that just sets it to breathing. No need to expose it to userspace.

Quoted 13 lines

Quoted 6 lines

Basically the charge led has 3 states:

1) Off
2) On
3) On when charging

Quoted 6 lines

And then when on it has 4 patterns:

1) Permanently off (so still not really on)
2) Permanently on
3) Blinking
4) Breathing

Quoted 5 lines

Ok, so you don't really need to support _both_ off methods.

Still sounds like a normal LED, with special "yoga-charging" and "yoga-breathing" triggers. (All the normal triggers should still work, too.)

Except that when in yoga-charging mode, the user should still be able to select if the LED is simply on when charging, blinking when charging, or breathing when charging.

Given that there are 2 independent settings model-ing this as a trigger does not work well. Also the trigger names should not contain yoga as the PMIC in question is used in other devices too.

I really think we should not try to shoe-horn special cases like this into the generic API and just use custom sysfs attributes for this. Like for example how the kbd-backlight

led classdev from the dell-laptop has custom attributes to select if the timeout for going off on keyboard/mouse activity and another custom attribute to select if only the keyboard or both the keyboard and mouse count as relevant activity.

Triggers are great for when we actually want to add a link between an event coming from some part of code in the kernel, to a LED classdevice, so that that event can control the LED.

Quoted 2 lines

Anyway, in such case I'd propose having rmmod/reboot/poweroff hook that just sets it to breathing. No need to expose it to userspace.

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to

userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Hi!

Quoted 2 lines

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to

Save the status on boot, then restore it on rmmod/reboot/poweroff? :-).

Quoted 8 lines

userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

Alternatively I could imagine "hardware" sysfs attribute, containing 0/1, with 0==software controlled, 1==hardware controlled.

The rest of attributes would have to be Cove-specific, yes (but still should fit with the rest of LED subsystem).

Quoted 4 lines

Quoted 2 lines

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to

Save the status on boot, then restore it on rmmod/reboot/poweroff? :-).

Which works until the system freezes one time. I believe that if we are going to do a LED driver for the charging LED on these devices, we MUST offer a way to put it back in its original state, even if the state is foo-barred at bootup.

Quoted 9 lines

Quoted 8 lines

userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Callback to struct led_classdev which when implemented by a driver like the current PMIC LED controller would do what it says.

2) Have the core create and register a virtual hardware trigger the first time a LED cdev which has this callback gets registered.

When configured as the trigger for this LED device this trigger calls hw_control_set(cdev, true) and when unregistered calls hw_control_set(cdev, false)

Taking a quick look at the trigger code, a problem with this is that normally any trigger can work with any led device, so this "hardware" trigger will show up in the list of possible triggers for each device.

This problem can be solved by making the activate method for the hardware trigger check the classdev has a hw_control_set callback and if not return -EINVAL, or maybe -ENXIO but still this is somewhat inconsistent with other triggers, which AFAIK work with any LED.

Quoted 2 lines

Alternatively I could imagine "hardware" sysfs attribute, containing 0/1, with 0==software controlled, 1==hardware controlled.

Hmm, maybe call it "hardware_controlled" instead ? Otherwise this would work for me and I would personally prefer this solution. This could even be done in the LED core using the hw_control_set callback I proposed, to make sure it is handled consistently between devices.

Quoted 2 lines

The rest of attributes would have to be Cove-specific, yes (but still should fit with the rest of LED subsystem).

Right, I see that the triggers attribute already uses the fmt where on "cat" all options are listed and the current active one has [] around it, so I think the pattern and frequency attributes I proposed should work well, although thinking more about this I believe the freq. attribute should be called pattern_freq to make clear it applies to blinking / breathing set through the pattern attribute.

Hi!

Jacek, could we get you to comment here? I'd prefer "hardware" trigger...

Quoted 28 lines

Quoted 4 lines

Quoted 2 lines

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to

Save the status on boot, then restore it on rmmod/reboot/poweroff? :-).

Which works until the system freezes one time. I believe that if we are going to do a LED driver for the charging LED on these devices, we MUST offer a way to put it back in its original state, even if the state is foo-barred at bootup.

Quoted 9 lines

Quoted 8 lines

userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

Quoted 8 lines

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

Agreed about the LED core.

Quoted 19 lines

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Callback to struct led_classdev which when implemented by a driver like the current PMIC LED controller would do what it says.

2) Have the core create and register a virtual hardware trigger the first time a LED cdev which has this callback gets registered.

When configured as the trigger for this LED device this trigger calls hw_control_set(cdev, true) and when unregistered calls hw_control_set(cdev, false)

Taking a quick look at the trigger code, a problem with this is that normally any trigger can work with any led device, so this "hardware" trigger will show up in the list of possible triggers for each device.

Quoted 4 lines

This problem can be solved by making the activate method for the hardware trigger check the classdev has a hw_control_set callback and if not return -EINVAL, or maybe -ENXIO but still this is somewhat inconsistent with other triggers, which AFAIK work with any LED.

I guess other option is to modify core so that it does not list "hardware" trigger for leds that don't support it.

Quoted 7 lines

Quoted 2 lines

Alternatively I could imagine "hardware" sysfs attribute, containing 0/1, with 0==software controlled, 1==hardware controlled.

Hmm, maybe call it "hardware_controlled" instead ? Otherwise this would work for me and I would personally prefer this solution. This could even be done in the LED core using the hw_control_set callback I proposed, to make sure it is handled consistently between devices.

This should be in LED core, yes.

Quoted 9 lines

Quoted 2 lines

The rest of attributes would have to be Cove-specific, yes (but still should fit with the rest of LED subsystem).

Right, I see that the triggers attribute already uses the fmt where on "cat" all options are listed and the current active one has [] around it, so I think the pattern and frequency attributes I proposed should work well, although thinking more about this I believe the freq. attribute should be called pattern_freq to make clear it applies to blinking / breathing set through the pattern attribute.

Take a look at blinking trigger. It can already do hardware acceleration, but uses different format than what you proposed.

Best regards,
Pavel

Hi Pavel,

Quoted 3 lines · Pavel Machek

Hi!

Jacek, could we get you to comment here? I'd prefer "hardware" trigger...

What prevents use from using pattern trigger with its hw_pattern file?

Do you remember drivers/leds/leds-sc27xx-bltc.c and its breathing mode, for which pattern trigger was introduced?

Hi,

Quoted 1 line · Pavel Machek

Hi!

<snip>

Quoted 4 lines

Quoted 2 lines

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to

Save the status on boot, then restore it on rmmod/reboot/poweroff? :-).

Which works until the system freezes one time. I believe that if we are going to do a LED driver for the charging LED on these devices, we MUST offer a way to put it back in its original state, even if the state is foo-barred at bootup.

Quoted 9 lines

Quoted 8 lines

userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Callback to struct led_classdev which when implemented by a driver like the current PMIC LED controller would do what it says.

2) Have the core create and register a virtual hardware trigger the first time a LED cdev which has this callback gets registered.

When configured as the trigger for this LED device this trigger calls hw_control_set(cdev, true) and when unregistered calls hw_control_set(cdev, false)

Taking a quick look at the trigger code, a problem with this is that normally any trigger can work with any led device, so this "hardware" trigger will show up in the list of possible triggers for each device.

This problem can be solved by making the activate method for the hardware trigger check the classdev has a hw_control_set callback and if not return -EINVAL, or maybe -ENXIO but still this is somewhat inconsistent with other triggers, which AFAIK work with any LED.

Quoted 2 lines

Alternatively I could imagine "hardware" sysfs attribute, containing 0/1, with 0==software controlled, 1==hardware controlled.

Hmm, maybe call it "hardware_controlled" instead ? Otherwise this would work for me and I would personally prefer this solution. This could even be done in the LED core using the hw_control_set callback I proposed, to make sure it is handled consistently between devices.

Quoted 2 lines

The rest of attributes would have to be Cove-specific, yes (but still should fit with the rest of LED subsystem).

Right, I see that the triggers attribute already uses the fmt where on "cat" all options are listed and the current active one has [] around it, so I think the pattern and frequency attributes I proposed should work well, although thinking more about this I believe the freq. attribute should be called pattern_freq to make clear it applies to blinking / breathing set through the pattern attribute.

Regards,

Hans

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Quoted 41 lines · Pavel Machek

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

Quoted 8 lines

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

But there are two kinds of 'hardware control' here in discussion:
1:
a) PMIC switching LED off/on/breathing/blinking reflected the charging state (charger is controlled by PMIC entirely without of software intervention) – 'hw controlled' mode

b) Software controls when LED is on/off/breathing/blinking but patterns are generated by PMIC – 'sw controlled' mode.

2:
a) parameters of lighting (continuous/blinking/breathing) are set in the PMIC and the PMIC generates patterns
b) blinking/breathing patterns are generated by the software entirely.

It seems that we sometimes confuse this two kinds of hw control definitions in the discussion.

The simple use case question: with this hardware and existing LED class API, what user/app should to do to make LED breathing with hw-generated pattern?

As I see, user should activate 'pattern' trigger and write to its hw_pattern attribute... hm... big pattern which describes every step of breathing? Some
simplified pattern which should be interpreted as 'enable the breathing
an set its frequency' by driver? Which level of simplification will be suitable? Which criteria of pattern rejection should be?

Hi Yauhen,

Quoted 51 lines · Yauhen Kharuzhy

Quoted 49 lines · Pavel Machek

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Quoted 5 lines

But there are two kinds of 'hardware control' here in discussion:
1:
a) PMIC switching LED off/on/breathing/blinking reflected the charging state (charger is controlled by PMIC entirely without of software intervention) – 'hw controlled' mode

Can we detect when charging is activated?

Quoted 18 lines

b) Software controls when LED is on/off/breathing/blinking but patterns are generated by PMIC – 'sw controlled' mode.

2:
a) parameters of lighting (continuous/blinking/breathing) are set in the PMIC and the PMIC generates patterns
b) blinking/breathing patterns are generated by the software entirely.

It seems that we sometimes confuse this two kinds of hw control definitions in the discussion.

The simple use case question: with this hardware and existing LED class API, what user/app should to do to make LED breathing with hw-generated pattern?

Quoted 5 lines

As I see, user should activate 'pattern' trigger and write to its hw_pattern attribute... hm... big pattern which describes every step of breathing? Some
simplified pattern which should be interpreted as 'enable the breathing
an set its frequency' by driver? Which level of simplification will be suitable? Which criteria of pattern rejection should be?

Please see my reply to Hans. We need some simplified device specific syntax of hw_pattern for this device, similarly like in:
Documentation/ABI/testing/sysfs-class-led-driver-sc27xx

Signature · 3 lines

-- 
Best regards,
Jacek Anaszewski

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Quoted 6 lines

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Right, so there are really 2 orthogonal issues here:

1) With this hardware the LED is either turned on/off automatically by the PMIC based on charging state; or it is under user control.

This is different from the led_classdev_notify_brightness_hw_changed case, where the hardware may update the state underneath the driver, but the driver can still always update the state itself. In this case if the LED is in hw-control mode then the driver cannot turn it on/off.

Pavel suggested modeling this with a new "hardware' trigger, where setting the trigger to this trigger will enable the hw-controlled mode and setting any other trigger will switch thing to the user-controlled mode.

2) This hardware can do blinking / breathing. There are various issues with this:

2.1) Blinking is more or less covered by the timer trigger. But breathing is not the pattern trigger is a poor match since there is only 1 fixed pattern

2.2) The supported blinking frequencies are very limited, so it might be better to keep the standard software blinking mode and have a special sysfs attribute to select the hardware blink support

2.3) The user can also select between continuous on / blinking / breathing when the LED is under hardware control (it will then be on / blinking / breathing) when charging.

Hi,

Quoted 41 lines · Pavel Machek

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

Quoted 8 lines

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Right, so there are really 2 orthogonal issues here:

1) With this hardware the LED is either turned on/off automatically by the PMIC based on charging state; or it is under user control.

This is different from the led_classdev_notify_brightness_hw_changed case, where the hardware may update the state underneath the driver, but the driver can still always update the state itself. In this case if the LED is in hw-control mode then the driver cannot turn it on/off.

Pavel suggested modeling this with a new "hardware' trigger, where setting the trigger to this trigger will enable the hw-controlled mode and setting any other trigger will switch thing to the user-controlled mode.

2) This hardware can do blinking / breathing. There are various issues with this:

2.1) Blinking is more or less covered by the timer trigger. But breathing is not the pattern trigger is a poor match since there is only 1 fixed pattern

2.2) The supported blinking frequencies are very limited, so it might be better to keep the standard software blinking mode and have a special sysfs attribute to select the hardware blink support

2.3) The user can also select between continuous on / blinking / breathing when the LED is under hardware control (it will then be on / blinking / breathing) when charging.

My suggestion for dealing with this is 2 new device specific sysfs attributes.

a) "pattern" which when read outputs e.g.:
on blinking [breathing]
And

b) "pattern_delay_on_off" which sets the on and off times for the hardware patterns on milliseconds, mirroring the delay_on / delay_off attributes from the timer trigger.

Note we could alternatively use: "hw_pattern" and "hw_pattern_delay_on_off" to make it clear that this is done in hardware.

To deal with interactions with the standard API I suggest we reset pattern to "on" when brightness gets set to 0, similar to how we stop the timer trigger then, etc.

Regards,

Hans

Hi all,

Quoted 63 lines · Hans de Goede

Hi,

Quoted 41 lines · Pavel Machek

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

Quoted 8 lines

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Right, so there are really 2 orthogonal issues here:

1) With this hardware the LED is either turned on/off automatically by the PMIC based on charging state; or it is under user control.

This is different from the led_classdev_notify_brightness_hw_changed case, where the hardware may update the state underneath the driver, but the driver can still always update the state itself. In this case if the LED is in hw-control mode then the driver cannot turn it on/off.

Quoted 4 lines

Pavel suggested modeling this with a new "hardware' trigger, where setting the trigger to this trigger will enable the hw-controlled mode and setting any other trigger will switch thing to the user-controlled mode.

We already do have hw_pattern file exposed by pattern trigger. It can be used to set hw breathing mode using some device specific syntax semantics, documented in dedicated ABI documentation. It was introduced for similar case, see Documentation/ABI/testing/sysfs-class-led-driver-sc27xx.

Hi,

Quoted 71 lines · Jacek Anaszewski

Hi all,

Quoted 63 lines · Hans de Goede

Hi,

Quoted 41 lines · Pavel Machek

Hi!

Quoted 33 lines

Quoted 20 lines

Quoted 6 lines

Quoted 5 lines

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

Quoted 8 lines

While this allows _user on console_ to control everything using echo, it is not suitable for applications trying to control LEDs.

As there's nothing special about the case here, I believe we should have generic solution here.

My preffered solution would be "hardware" trigger that leaves the LED in hardware control.

As you explained in the parts which I snipped, there are many devices which have a similar choice for a LED being under hw or user control. I can see how this looks like a trigger and how we could use the trigger API for this.

Quoted 7 lines

I believe though, that if we implement a "virtual" (for lack of a better word) trigger for this, that this should be done in the LED core. I can envision this working like this:

1) Add a:

hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);

Please note that we have support for hw patterns in the pattern trigger. (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its breathing pattern).
We have also support for hw blinking in timer trigger via blink_set op.

Quoted 5 lines

In addition to that there is brightness_hw_changed sysfs attribute with led_classdev_notify_brightness_hw_changed() LED API.

Couldn't they be used in concert to support the specific features of the device in question?

Quoted 8 lines

I believe main issue here is this:

Hardware can automatically control the LED according to the charging status, or it can be used as normal software-controlled LED.

I believe we should use trigger to select if hardware controls it or not (and then add driver-specific files to describe the details). Other proposal is in the mail thread, too.

Right, so there are really 2 orthogonal issues here:

1) With this hardware the LED is either turned on/off automatically by the PMIC based on charging state; or it is under user control.

This is different from the led_classdev_notify_brightness_hw_changed case, where the hardware may update the state underneath the driver, but the driver can still always update the state itself. In this case if the LED is in hw-control mode then the driver cannot turn it on/off.

Quoted 4 lines

Pavel suggested modeling this with a new "hardware' trigger, where setting the trigger to this trigger will enable the hw-controlled mode and setting any other trigger will switch thing to the user-controlled mode.

Quoted 5 lines

We already do have hw_pattern file exposed by pattern trigger. It can be used to set hw breathing mode using some device specific syntax semantics, documented in dedicated ABI documentation. It was introduced for similar case, see Documentation/ABI/testing/sysfs-class-led-driver-sc27xx.

Ah I see, yes that could be used, the pattern would just be a single integer specifying the cycle time in milliseconds, as nothing else is configurable.

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

On 2/15/19 11:26 PM, Hans de Goede wrote:

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

Hi,

Quoted 1 line · Jacek Anaszewski

On 2/15/19 11:26 PM, Hans de Goede wrote:

<snip>

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Since there cannot be more then 1 trigger active, with the hardware trigger solution we cannot say we want automatic control (hardware trigger) and blinking when the automatic mode says the LED should be on (timer trigger) as that would mean having 2 triggers active with the hardware trigger solution.

As such I think the switch between automatic and manual control would be best exported through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Regards,

Hans

Hi Hans,

Quoted 43 lines · Hans de Goede

Hi,

Quoted 1 line · Jacek Anaszewski

On 2/15/19 11:26 PM, Hans de Goede wrote:

<snip>

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

Quoted 43 lines

Quoted 36 lines

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Quoted 8 lines

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples with the following meaning for the time part of each tupple

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Where only the times in tupple 0-2; or the times in 3-5 can be non-zero. Having non zero times for both some charging and some manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time must be 0.

When configured to blink then blinking_off must be either 0 (continuously on); or it must be the same as blinking_on.

I believe this will work, does this sound ok to you ?

Hi!

Quoted 77 lines

Quoted 43 lines

Quoted 36 lines

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Quoted 8 lines

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples with the following meaning for the time part of each tupple

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Where only the times in tupple 0-2; or the times in 3-5 can be non-zero. Having non zero times for both some charging and some manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time must be 0.

When configured to blink then blinking_off must be either 0 (continuously on); or it must be the same as blinking_on.

Quoted 1 line

I believe this will work, does this sound ok to you ?

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

Quoted 4 lines

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

Then we are back to step 1 of the discussion, that we need another mechanism outside of the trigger to select if the LED shows the configured pattern always, or only when the charger is on.

These really are 2 orthogonal settings, there is a pattern which can be set and the LED can either show that pattern always; or only when charging the battery. Note that the same pattern is used in both cases.

This is why I previously suggested having a custom sysfs hardware_control attribute which selects between the "only show pattern when charging" modes ("hardware_control=1" or "always show the pattern mode" ("hardware_control=0").

Quoted 8 lines

Quoted 4 lines

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

Then we are back to step 1 of the discussion, that we need another mechanism outside of the trigger to select if the LED shows the configured pattern always, or only when the charger is on.

Yep, sorry.

Quoted 8 lines

These really are 2 orthogonal settings, there is a pattern which can be set and the LED can either show that pattern always; or only when charging the battery. Note that the same pattern is used in both cases.

This is why I previously suggested having a custom sysfs hardware_control attribute which selects between the "only show pattern when charging" modes ("hardware_control=1" or "always show the pattern mode" ("hardware_control=0").

I see... and yes, that would be the easiest solution.

But somehow I see "this LED is controlled by charging state" as primary and "it shows pulses instead of staying on" as secondary eye-candy.

This week there was another driver for charger LED.. but that one does not do pulses. Ideally, we'd like consistent interface to the userland.

(To make it complex, the other driver supports things like:
LED solid on -- fully charged
LED blinking slowly -- charging
LED blinking fast -- charge error
LED off -- not charging).

Quoted 10 lines

I see... and yes, that would be the easiest solution.

But somehow I see "this LED is controlled by charging state" as primary and "it shows pulses instead of staying on" as secondary eye-candy.

This week there was another driver for charger LED.. but that one does not do pulses. Ideally, we'd like consistent interface to the userland.

Quoted 5 lines

(To make it complex, the other driver supports things like:
LED solid on -- fully charged
LED blinking slowly -- charging
LED blinking fast -- charge error
LED off -- not charging).

Something like that could be supported with my original hw_pattern proposal where we simply encode all of this in the hw-pattern file:

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

So for this chip you mention, we do not need the breathing time (no breathing support), so we would get the following tupples:

tupple0: not charging blinking_on_time
tupple1: not charging blinking_off_time
tupple2: slow charging blinking_on_time
tupple3: slow charging blinking_off_time
tupple4: fast charging blinking_on_time
tupple5: fast charging blinking_off_time
tupple6: charging error blinking_on_time
tupple7: charging error blinking_off_time

Where by solid on/off can be done by setting one of the blinking times to 0.

Having hw_pattern ABIs like this where some of the tupples only activate on certain conditions might be better then a hardware_control sysfs file as it offers more flexibility.

Hi!

Quoted 19 lines

Quoted 10 lines

I see... and yes, that would be the easiest solution.

But somehow I see "this LED is controlled by charging state" as primary and "it shows pulses instead of staying on" as secondary eye-candy.

This week there was another driver for charger LED.. but that one does not do pulses. Ideally, we'd like consistent interface to the userland.

Quoted 5 lines

(To make it complex, the other driver supports things like:
LED solid on -- fully charged
LED blinking slowly -- charging
LED blinking fast -- charge error
LED off -- not charging).

Something like that could be supported with my original hw_pattern proposal where we simply encode all of this in the hw-pattern file:

Quoted 6 lines

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

So the userland would need to know "I'm on yoga, so 3rd tupple sets up breathing when charging"?

Quoted 11 lines

So for this chip you mention, we do not need the breathing time (no breathing support), so we would get the following tupples:

tupple0: not charging blinking_on_time
tupple1: not charging blinking_off_time
tupple2: slow charging blinking_on_time
tupple3: slow charging blinking_off_time
tupple4: fast charging blinking_on_time
tupple5: fast charging blinking_off_time
tupple6: charging error blinking_on_time
tupple7: charging error blinking_off_time

Patterns and their meanings are fixed (or almost) on that driver, so fortunately that will not be neccessary.

Quoted 8 lines

Where by solid on/off can be done by setting one of the blinking times to 0.

Having hw_pattern ABIs like this where some of the tupples only activate on certain conditions might be better then a hardware_control sysfs file as it offers more flexibility.

Agreed about flexibility, but I don't think it does enough to provide hardware abstraction. It might be too much flexibility.

Also it only works with hw controlled LEDs.

With RGB LED multiple patterns per LED make a lot of sense (as there's typically just one led.

For example on N900, we have

green: fully charged.
yellow pulsing: charging.
solid yellow: hardware feature, emergency charging. white pulsing: happy phone, no events
blue fast pulses: message waiting

On N900, I'm handling that in userspace, but...

Best regards,
Pavel

Hi,

Quoted 28 lines · Pavel Machek

Hi!

Quoted 25 lines

Quoted 10 lines

I see... and yes, that would be the easiest solution.

But somehow I see "this LED is controlled by charging state" as primary and "it shows pulses instead of staying on" as secondary eye-candy.

This week there was another driver for charger LED.. but that one does not do pulses. Ideally, we'd like consistent interface to the userland.

Quoted 5 lines

(To make it complex, the other driver supports things like:
   LED solid on -- fully charged
   LED blinking slowly -- charging
   LED blinking fast -- charge error
   LED off -- not charging).

Something like that could be supported with my original hw_pattern proposal where we simply encode all of this in the hw-pattern file:

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Quoted 2 lines

So the userland would need to know "I'm on yoga, so 3rd tupple sets up breathing when charging"?

Yes this is for the hw_pattern file not the regular pattern file and if I've understood things correctly then the hw_pattern file is often (always?) specific to the LED controller model. See e.g. :
Documentation/ABI/testing/sysfs-class-led-driver-sc27xx

Also userland is not really expected to touch this, the user himself could touch it if he/she wants to customize things.

Quoted 12 lines

Quoted 11 lines

So for this chip you mention, we do not need the breathing time (no breathing support), so we would get the following tupples:

tupple0: not charging blinking_on_time
tupple1: not charging blinking_off_time
tupple2: slow charging blinking_on_time
tupple3: slow charging blinking_off_time
tupple4: fast charging blinking_on_time
tupple5: fast charging blinking_off_time
tupple6: charging error blinking_on_time
tupple7: charging error blinking_off_time

Quoted 2 lines

Patterns and their meanings are fixed (or almost) on that driver, so fortunately that will not be neccessary.

Ok, so back the Whiskey Cove PMIC LED controller, I think there was some agreement to add a hw_control sysfs attribute and simplify the hw_pattern ABI to:

tupple0: blinking_on_time
tupple1: blinking_off_time
tupple2: breathing_time

You suggested adding support for a hw_control
sysfs attribute to the core, activated by a flag.

I assume that there will then be a callback into the

driver when that file gets written. The semantics for the Whiskey Cove PMIC LED controller are clear, but how should the patch adding support for this to the LED core describe the new hw_control sysfs attribute in:
Documentation/ABI/testing/sysfs-class-led ?

Or do you just want to have the basic handling in the core and then describe the semantics in a per LED controller way like how we do for hw_pattern, so for the Whiskey Cove PMIC LED controller we would add a:
Documentation/ABI/testing/sysfs-class-led-cht-wc file, which we need to do anyways for the hw_pattern file?

Hi!

Quoted 77 lines

Quoted 43 lines

Quoted 36 lines

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Quoted 8 lines

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples with the following meaning for the time part of each tupple

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Where only the times in tupple 0-2; or the times in 3-5 can be non-zero. Having non zero times for both some charging and some manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time must be 0.

When configured to blink then blinking_off must be either 0 (continuously on); or it must be the same as blinking_on.

Quoted 1 line

I believe this will work, does this sound ok to you ?

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

Quoted 81 lines · Pavel Machek

Hi!

Quoted 78 lines

Quoted 43 lines

Quoted 36 lines

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Quoted 8 lines

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples with the following meaning for the time part of each tupple

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Where only the times in tupple 0-2; or the times in 3-5 can be non-zero. Having non zero times for both some charging and some manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time must be 0.

When configured to blink then blinking_off must be either 0 (continuously on); or it must be the same as blinking_on.

I believe this will work, does this sound ok to you ?

Quoted 3 lines

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

This is hardware specific and is supposed to have dedicated ABI documentation. There's no reason to introduce new mechanisms when existing ones fit. It will still describe a pattern but activated on some condition.

Hi,

Quoted 82 lines · Jacek Anaszewski

Quoted 81 lines · Pavel Machek

Hi!

Quoted 78 lines

Quoted 43 lines

Quoted 36 lines

Quoted 10 lines

Quoted 10 lines

I think that should work fine, which means that we can use the timer and pattern trigger support for the blinking and breathing modes.

That still leaves the switching between user and hw-control modes, as discussed the hw-controlled mode could be modelled as a new "hardware" trigger, but then we cannot choose between on/blink/breathing when in hw-controlled mode. As Pavel mentioned, that would require some sort of composed trigger, where we have both the hardware and timer triggers active for example.

Quoted 7 lines

Quoted 3 lines

I think it might be easier to just allow turning on/off the hardware control mode through a special "hardware_control" sysfs attribute and then use the existing timer and pattern triggers for blinking / breathing.

Pattern trigger exposes pattern file by default and hw_pattern if pattern_set/get ops are provided. Writing them enables software and hardware pattern respectively.

This is not about software vs hardware pattern.

There are 2 *orthogonal*, separate problems/challenges with this LED controller:

1) It has hardware blinking and breathing, as discussed this can be controlled through the timer and pattern triggers, so this problem is solved.

2) It has 2 operating modes:

a) Automatic/hardware controlled, in this mode the LED is turned off or on (where on can be continues on, blinking or breathing) by the hardware itself, when in this mode we / userspace is not in control of the LED

b) Manual/user controlled mode, in this mode we / userspace can control of the LED.

Quoted 6 lines

Currently there is no API in the ledclass to switch a LED from automatic controlled to user controlled and back, This is what the proposed hardware trigger was for, to switch to automatic mode. A problem with this is that we still want to be able to chose between continues on, blinking or breathing (when on), configure the max brightness, etc.

Quoted 8 lines

Yes, we do have the API to switch a LED from automatic (hardware accelerated) control to software control and back. This is pattern trigger, which exposes two files for setting pattern: pattern and hw_pattern. Writing pattern file switches the device to software control mode and writing hw_pattern switches it to the hardware control, with the possibility of defining device specific ABI syntax to enable particular pattern (blinking, breathing or event permanently on in case of this device).

OK, I see. So we would use the hw_pattern for this and the driver would implement the pattern_set led_classdev callback.

The pattern_set callback would then expect 6 brightness/time tuples with the following meaning for the time part of each tupple

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

Where only the times in tupple 0-2; or the times in 3-5 can be non-zero. Having non zero times for both some charging and some manual values is not allowed.

If a breathing time is set, none of the other times may be non
0. If blinkig_on and blinking_off are used then breathing_time must be 0.

When configured to blink then blinking_off must be either 0 (continuously on); or it must be the same as blinking_on.

I believe this will work, does this sound ok to you ?

Quoted 8 lines

Quoted 3 lines

I don't pretend to fully understand it, _but_ hw_pattern should really describe the pattern LED should do, not whether it reacts to charging or not.

This is hardware specific and is supposed to have dedicated ABI documentation. There's no reason to introduce new mechanisms when existing ones fit. It will still describe a pattern but activated on some condition.

Right, but we can control the condition, so either we need to make the condition part of the pattern as in my recent proposal with:

tupple0: charging blinking_on_time
tupple1: charging blinking_off_time
tupple2: charging breathing_time
tupple3: manual blinking_on_time
tupple4: manual blinking_off_time
tupple5: manual breathing_time

As hw_pattern ABI; or we need to add an extra sysfs file to set the condition.

So do you prefer the driver to code the condition into the hw_pattern (see above); or do you prefer a separate sysfs attribute for the condition?

Quoted 15 lines

Quoted 6 lines

Basically the charge led has 3 states:

1) Off
2) On
3) On when charging

Quoted 6 lines

And then when on it has 4 patterns:

1) Permanently off (so still not really on)
2) Permanently on
3) Blinking
4) Breathing

Ok, so you don't really need to support _both_ off methods.

Quoted 3 lines

Still sounds like a normal LED, with special "yoga-charging" and "yoga-breathing" triggers. (All the normal triggers should still work, too.)

Except that when in yoga-charging mode, the user should still be able to select if the LED is simply on when charging, blinking when charging, or breathing when charging.

Quoted 4 lines

Quoted 4 lines

The problem is that any changes made are permanent, they survice reboots and the default is Breathing, so we need a way to restore that which does not involve removing the internal battery of these devices.

Quoted 2 lines

Anyway, in such case I'd propose having rmmod/reboot/poweroff hook that just sets it to breathing. No need to expose it to userspace.

That assumes that breathing is the default setting on all hardware and again I don't see why not to export this functionality to userspace. Just because something does not fit in the existing API is IMHO not a good reason to not expose it to userspace.

I suggest that we deal with this special case by adding 3 custom sysfs attributes:

1) "mode" which when read, prints, e.g. :
manual [on-when-charging]

With the [] indicating the current mode, and writes accepting all 3 values and updating the hw accordingly.

This format is used in more places in the kernel and allows for the user to easily discover the possible values.

2) "pattern" which when read, prints, e.g. :
on blinking [breathing]

3) "frequency", when read prints, e.g. :
0.25hz [0.5hz] 1hz 2hz
Note this controls both the blinking and breathing freq.

Note I've not listed off anywhere, this can be achieved by setting the brightness to 0 as we do with normal leds.

For interactions with other APIs we can do the standard thing where writing 0 to brightness resets things, in this case this would reset mode to manual and pattern to on.

And if the blinking API gets used, then too the mode should be switched to manual, and the pattern obviously becomes blinking.

I believe this will work well with this hardware and nicely allow the user to control all settings.

Regards,

Hans

Hi,

Quoted 5 lines · Yauhen Kharuzhy

Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not.

This driver was tested with Lenovo Yoga Book notebook.

Thank you for working on this. The CHT Whiskey Cove PMIC is also used on the GPD win and GPD pocket devices and there LED1 by default indicates the charging status.

Since your driver forces the LED into SWCTL mode on probe() this means that any kernel with it enabled will break the charging LEDs OOTB function, this is undesirable.

I believe it would be best to add a custom "mode" attribute to the led classdev, with "manual" and "on-when-charging" modes, this would then control bits 0-1 of reg 0x5e1f and by default these bits should be left as is when the driver loads.

Note that in my experience the "charging" mode only works when bits 0-1 have the value 10. I've some written notes from when I played with this myself and they say:

   -CHT WC powerled control 0x5e1f: bits 0-1:
    0: ????
    1: Off
    2: On when charging
    3: On
   -CHT WC powerled pattern control 0x5e20: bits 1-2:
    0: Off
    1: On
    2: Blinking
    3: Glowing

Also note that the 0x5e20 notes do not match with your defines, I believe this is a small bug in your code, see comments in line below.

As for the 0x5e20 settings, I believe another custom sysfs attribute, called "breathing" would be a good idea to export the breathing functionality.

The way I see this working is that writing "1" to this will turn on glowing mode, and writing 0 to it, or 0 to brightness will turn it off. Reading it will return 1/0 depending on whether the LED is in glowing mode or not.

For an example of adding custom sysfs attributes to a led-class device see kbd_led_groups and kbd_led_attrs in:
drivers/platform/x86/dell-laptop.c

Quoted 18 lines

drivers/leds/leds-cht-wcove.c new file +18 -0

+
+#define CHT_WC_LED1_CTRL		0x5e1f
+#define CHT_WC_LED1_FSM			0x5e20
+#define CHT_WC_LED1_PWM			0x5e21
+
+#define CHT_WC_LED2_CTRL		0x4fdf
+#define CHT_WC_LED2_FSM			0x4fe0
+#define CHT_WC_LED2_PWM			0x4fe1
+
+/* HW or SW control of charging led */
+#define CHT_WC_LED1_SWCTL		BIT(0)
+#define CHT_WC_LED1_ON			BIT(1)
+
+#define CHT_WC_LED2_ON			BIT(0)
+#define CHT_WC_LED_I_MA2_5		(2 << 2)
+/* LED current limit */
+#define CHT_WC_LED_I_MASK		GENMASK(3, 2)
+

drivers/leds/leds-cht-wcove.c line 43

Quoted 10 lines

drivers/leds/leds-cht-wcove.c new file +10 -0

+#define CHT_WC_LED_F_1_4_HZ		(0 << 4)
+#define CHT_WC_LED_F_1_2_HZ		(1 << 4)
+#define CHT_WC_LED_F_1_HZ		(2 << 4)
+#define CHT_WC_LED_F_2_HZ		(3 << 4)
+#define CHT_WC_LED_F_MASK		0x30
+
+#define CHT_WC_LED_EFF_ON		BIT(1)
+#define CHT_WC_LED_EFF_BLINKING		BIT(2)
+#define CHT_WC_LED_EFF_BREATHING	BIT(3)
+#define CHT_WC_LED_EFF_MASK		0x06

So your MASK is correct here, but the values used should be based on that, so you get:

#define CHT_WC_LED_EFF_ON (1 << 1)
#define CHT_WC_LED_EFF_BLINKING (2 << 1)
#define CHT_WC_LED_EFF_BREATHING (3 << 1)

Note that this effectively only changes the value of CHT_WC_LED_EFF_BREATHING, so that it now to fits in your mask.

Regards,

Hi,

Quoted 5 lines · Yauhen Kharuzhy

Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not.

This driver was tested with Lenovo Yoga Book notebook.

Note that in my experience the "charging" mode only works when bits 0-1 have the value 10.

Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC mfd device driver.

Signed-off-by Yauhen Kharuzhy <[email protected]>

---
  drivers/mfd/intel_soc_pmic_chtwc.c | 1 +
  1 file changed, 1 insertion(+)

drivers/mfd/intel_soc_pmic_chtwc.c +1 -0

index 64a3aece9c5e..be84bb2aa837 100644
@@ -60,6 +60,7 @@ static struct mfd_cell cht_wc_dev[] = {
  		.resources = cht_wc_ext_charger_resources,
  	},
  	{	.name = "cht_wcove_region", },
+	{	.name = "cht_wcove_leds", },
  };
 
  /*

Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC mfd device driver.

Signed-off-by Yauhen Kharuzhy <[email protected]>

---
 drivers/mfd/intel_soc_pmic_chtwc.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mfd/intel_soc_pmic_chtwc.c +1 -0

index 64a3aece9c5e..be84bb2aa837 100644
@@ -60,6 +60,7 @@ static struct mfd_cell cht_wc_dev[] = {
 		.resources = cht_wc_ext_charger_resources,
 	},
 	{	.name = "cht_wcove_region", },
+	{	.name = "cht_wcove_leds", },
 };
 
 /*

Quoted 7 lines · Yauhen Kharuzhy

Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC mfd device driver.

Signed-off-by Yauhen Kharuzhy <[email protected]>

---
 drivers/mfd/intel_soc_pmic_chtwc.c | 1 +
 1 file changed, 1 insertion(+)

I'll fix up the subject line for you.

In future, please do the following to see what is expected:

  git log --oneline -- <subsystem>

This patch series introduces new driver for controlling LEDs connected to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger status led). Only simple 'always on' and blinking modes are supported for now, no breathing.

Driver was tested only with Lenovo Yoga Book notebook, and I don't have any documentation for the PMIC, so proposals and testing are welcome.

v2:
- Fix comments and code style
- Add mutex to protect led state
- Add defaults triggers
- Fix module license declaration

Yauhen Kharuzhy (2):
leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs
mfd: Add leds MFD cell for intel_soc_pmic_chtwc

Hi Yauhen,

I had a discussion with Jacek Anaszewski about this in another thread and I believe we have come up with a solution for this which should work nicely and should allow us to move forward with your driver (after it is reworked to match the solution. So the solution we've come up with is:

1) After thinking a bit more about the primary use-case for this, I've come to the conclusion that putting LED1 / the charging LED in software-controlled mode is also the correct thing to do on the GPD win / pocket. The reason for this is that ideally the LED would glow while charging and be simply solid on when the battery is full, the hw control does not allow this, so the GPD win/pocket can benefit from sw-control too.

2) To allow the desired behavior we need to define a new "<supply-name>-charging-glow-full-solid" trigger in drivers/power/supply/power_supply_leds.c; and this must be the default trigger for the Intel Cherry Trail Whiskey Cove LED driver so that everything will just work. Also we must restore the original hw control setting on reboot/shut-down so that this is used on the GPD win/pocket when Linux is not running.

3) To be able to actually implement this new trigger we first need 2 things in the kernel internal LED APIs:

3a) An API for triggers to put the LED in glowing mode, we've come up with the following prototype for this:

void led_trigger_glow(struct led_trigger *trigger, unsigned long *cylce_time);

Where cycle_time is the number of milliseconds for a full glow cycle (from off to full-on to off again). So if cylce_time is set to 1000 then the LED glows at 1 Hz, 500, 2 HZ, etc. Note as with led_trigger_blink() the time passed to led_trigger_glow is passed by reference as the LED driver may round it to match what the hardware can do and the rounded value is returned to the caller through the reference.

3b) 3a) in turn will require adding a new optional glow_set callback to struct led_classdev which will then get called by led_trigger_glow if available.

We've not discussed yet what to do if led_trigger_glow gets called on a led_classdev which does not implement the new glow_set callback, I guess the most sensible thing to do then is to fallback to blinking with delay_on and delay_off set to cylce_time / 2.

If you can make some time to work on this solution that would be great. Please let me know if you've any questions about the solution outlined below.

Note that glowing is only exported as in kernel functionality, I see no use-case for exporting this to userspace and keeping this in kernel allows us to keep things nice and simple.