[PATCH v2 0/2] Intel Cherry Trail Whiskey Cove LEDs support

[PATCH v2 0/2] Intel Cherry Trail Whiskey Cove LEDs support

[Yauhen Kharuzhy]

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

 drivers/leds/Kconfig               |  11 ++
 drivers/leds/Makefile              |   1 +
 drivers/leds/leds-cht-wcove.c      | 293 +++++++++++++++++++++++++++++
 drivers/mfd/intel_soc_pmic_chtwc.c |   1 +
 4 files changed, 306 insertions(+)
 create mode 100644 drivers/leds/leds-cht-wcove.c

-- 
2.20.1

[PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[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.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 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

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..8f50f38af57e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -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
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..1c1995d3441c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -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
diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
new file mode 100644
index 000000000000..ee4287f8e806
--- /dev/null
+++ b/drivers/leds/leds-cht-wcove.c
@@ -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 <jekhor@gmail.com>
+//
+// 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)
+
+#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
+
+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 <jekhor@gmail.com>");
+MODULE_LICENSE("GPL v2");
+
-- 
2.20.1

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 12-02-19 21:59, Yauhen Kharuzhy wrote:
> 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

> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   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
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a72f97fca57b..8f50f38af57e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -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
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4c1b0054f379..1c1995d3441c 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -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
> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
> new file mode 100644
> index 000000000000..ee4287f8e806
> --- /dev/null
> +++ b/drivers/leds/leds-cht-wcove.c
> @@ -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 <jekhor@gmail.com>
> +//
> +// 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)
> +
> +#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,

Hans




> +
> +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 <jekhor@gmail.com>");
> +MODULE_LICENSE("GPL v2");
> +
> 

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> On 12-02-19 21:59, Yauhen Kharuzhy wrote:
> >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.

Agreed.

> 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.

> 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.

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

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 14-02-19 00:07, Pavel Machek wrote:
> Hi!
> 
>> On 12-02-19 21:59, Yauhen Kharuzhy wrote:
>>> 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.
> 
> Agreed.
> 
>> 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.

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

And Yauhen seems to have discovered we can control
the brightness too (I did not find that out while
poking the hw myself).

So this means that we can control most aspects of the
LED even when it is in "On when charging" state


> 
>> 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.
> 
> 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.

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!


> >Agreed.
> >
> >>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.
> 
> 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

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.)

> 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.

> >>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.

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.

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 14-02-19 00:38, Pavel Machek wrote:
> Hi!
> 
> 
>>> Agreed.
>>>
>>>> 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.
>>
>> 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
> 
> 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.

Trying to shoe-horn all sort of other stuff into this API
just does not work well IMHO and is abuse of the original
LED trigger API.

>> 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.

I disagree I see no reason if we are adding a driver for
this why the user should not be able to select if the
LED is on, blinking or glowing when charging.

>>>> 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.
> 
> Wow. Now that's a broken hardware.

The PMIC is attached to the battery and retaining the state
of the logic controlling the charging LED when off seems sane
to me.

Arguably the firmware should set some sane defaults on boot,
but at least on the 2 devices with this PMIC I have it does
not do this.

> 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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >>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
> >
> >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

This is not really that much of a special case. I have LEDs on
thinkpads that can be either hardware controlled or sw controlled with
various functions. Both Droid 4 and N900 have hardware support for
showing charging state on the notification LED. N900 additionaly has
debug functionality for keyboard 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.

Yeah well more stuff to fix :-(.

> 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.

Well, triggers are also useful because userspace should already know
about them.... and because your hardware already behaves as if it had
"trigger" implemented in 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.
> 
> 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? :-).

> 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).

									Pavel

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 14-02-19 12:14, Pavel Machek wrote:
> Hi!

<snip>

>> 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.

>> 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.

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.

> 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.

> 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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

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

> >>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.
> 
> >>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.
> 
> 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.

> 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.

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

> >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.

> >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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi Pavel,

On 2/14/19 1:28 PM, Pavel Machek wrote:
> 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?

>>>> 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.
>>
>>>> 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.
>>
>> 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.
> 
>> 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.
> 
> I guess other option is to modify core so that it does not list
> "hardware" trigger for leds that don't support it.

Pattern trigger will not show hw_pattern file if pattern_set/get ops
are not provided.

>>> 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.
> 
>>> 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
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

On Fri 2019-02-15 22:41:31, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 2/14/19 1:28 PM, Pavel Machek wrote:
> >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?

Yes, we can do that.

But forget heartbeat and blinking now.

Problem I'm getting at is that this LED can either be software
controlled (showing for example disk activity) or it can show whether
the battery is charging (autonomously, in hardware, will work even
after kernel crash).

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi Hans and all,

On 2/14/19 12:31 PM, Hans de Goede wrote:
> Hi,
> 
> On 14-02-19 12:14, Pavel Machek wrote:
>> Hi!
> 
> <snip>
> 
>>> 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.
> 
>>> 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.
> 
> 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);

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?

> 
> 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.
> 
>> 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.
> 
>> 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
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >>>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.
> >
> >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);
> 
> 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?

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.

Best regards,

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Yauhen Kharuzhy]

On Fri, Feb 15, 2019 at 12:03:07AM +0100, Pavel Machek wrote:
> Hi!
> 
> > >>>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.
> > >
> > >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);
> > 
> > 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?
> 
> 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?

-- 
Yauhen Kharuzhy

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi Yauhen,

On 2/15/19 8:27 AM, Yauhen Kharuzhy wrote:
> On Fri, Feb 15, 2019 at 12:03:07AM +0100, Pavel Machek wrote:
>> Hi!
>>
>>>>>> 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.
>>>>
>>>> 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);
>>>
>>> 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?
>>
>> 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

Can we detect when charging is activated?

> 	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?

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

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Yauhen Kharuzhy]

On Fri, Feb 15, 2019 at 10:43:17PM +0100, Jacek Anaszewski wrote:
> Hi Yauhen,
> 
> On 2/15/19 8:27 AM, Yauhen Kharuzhy wrote:
> > On Fri, Feb 15, 2019 at 12:03:07AM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > > 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.
> > > > > 
> > > > > 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);
> > > > 
> > > > 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?
> > > 
> > > 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
> 
> Can we detect when charging is activated?

Depends on PMIC and charger configuration. Seems there are no common
way working for all configurations exists.

> 
> > 	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?
> 
> 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

Thanks, this approach seems easy to implement.

> 
> -- 
> Best regards,
> Jacek Anaszewski

-- 
Yauhen Kharuzhy

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 15-02-19 00:03, Pavel Machek wrote:
> Hi!
> 
>>>>> 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.
>>>
>>> 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);
>>
>> 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?
> 
> 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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >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:

For completeness, there are actually 3 issues:

> 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.

3) Your hardware supports "composed" triggers:

You can do "breathing while charging", can do "blinking while
charging" and you probably could do "blinking while disk is active" or
"breathing while microphone is muted".

We don't have such support in LED subsystem, AFAICT. Anyway, I'd say
3) Does not need to be solved now...

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi all,

On 2/15/19 12:27 PM, Hans de Goede wrote:
> Hi,
> 
> On 15-02-19 00:03, Pavel Machek wrote:
>> Hi!
>>
>>>>>> 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.
>>>>
>>>> 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);
>>>
>>> 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?
>>
>> 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.

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.

> 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

hw_pattern can handle that. Actually pattern trigger is a superset of
timer trigger. So hw blinking could be supported by both timer and
pattern triggers (needs presence of blink_set and pattern_set ops in the
driver), and hw breathing mode should be supported only by pattern
trigger.

> 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.

We have everything reedy for use.

Please also get acquainted with
Documentation/devicetree/bindings/leds/leds-trigger-pattern.txt -
it describes the gradual dimming feature of software pattern fallback.

> 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
> 
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/15/19 10:42 PM, Jacek Anaszewski wrote:
> Hi all,
> 
> On 2/15/19 12:27 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 15-02-19 00:03, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>> 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.
>>>>>
>>>>> 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);
>>>>
>>>> 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?
>>>
>>> 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.
> 
> 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.

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

On 2/15/19 11:26 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/15/19 10:42 PM, Jacek Anaszewski wrote:
>> Hi all,
>>
>> On 2/15/19 12:27 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 15-02-19 00:03, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> 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);
>>>>>
>>>>> 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?
>>>>
>>>> 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.
>>
>> 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 depends on how the pattern looks like. If it is breathing then
we'd need more than one value.

> 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.

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.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/15/19 11:31 PM, Jacek Anaszewski wrote:
> On 2/15/19 11:26 PM, Hans de Goede wrote:

<snip>

>> 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.
> 
> 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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi Hans,

On 2/16/19 12:14 AM, Hans de Goede wrote:
> Hi,
> 
> On 2/15/19 11:31 PM, Jacek Anaszewski wrote:
>> On 2/15/19 11:26 PM, Hans de Goede wrote:
> 
> <snip>
> 
>>> 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.
>>
>> 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.

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).

The hw_pattern is device specific, so the device specific ABI
documentation can state that when the pattern trigger is enabled
for this device in hw accelerated mode, then it will blink the
LED only when specific conditions are met - in this case it will
be activation of charging.

Since timer trigger does not allow to explicitly choose between
software and hardware timer engine, then I propose to not take
it into account for supporting hardware blinking offered by
this device. Enabling pattern trigger will just periodically
set brightness of the LED.

Since pattern trigger is a superset of timer, then it can handle
ordinary blinking as well.

> 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
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/16/19 6:02 PM, Jacek Anaszewski wrote:
> Hi Hans,
> 
> On 2/16/19 12:14 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 2/15/19 11:31 PM, Jacek Anaszewski wrote:
>>> On 2/15/19 11:26 PM, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> 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.
>>>
>>> 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.
> 
> 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 ?

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >>>>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.
> >>>
> >>>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.
> >
> >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 ?

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.

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/16/19 8:37 PM, Pavel Machek wrote:
> Hi!
> 
>>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>> 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 ?
> 
> 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").

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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


> >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.

> 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).

Best regards,

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/17/19 1:08 AM, Pavel Machek wrote:
> 
>>> 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.
> 
>> 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).

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.

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >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).
> 
> 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 the userland would need to know "I'm on yoga, so 3rd tupple sets up
breathing when charging"?

> 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.

> 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 17-02-19 18:45, Pavel Machek wrote:
> Hi!
> 
>>> 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).
>>
>> 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 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.

>> 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.

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?

<snip stuff about N900>

Regards,

Hans




> 
>> 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
> 

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi Hans,

On 2/18/19 12:12 PM, Hans de Goede wrote:
> Hi,
> 
> On 17-02-19 18:45, Pavel Machek wrote:
>> Hi!
>>
>>>> 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).
>>>
>>> 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 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

This is so far the only user of hw_pattern. Whereas as in case of the
mentioned driver, it allows to simplify the pattern description which
would have to be otherwise quite complex in case of breathing, still the
sequence of tuples needs to adhere to the common pattern syntax.

> Also userland is not really expected to touch this, the user himself
> could touch it if he/she wants to customize things.
> 
>>> 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.
> 
> 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

Pattern format is defined to:

brightness_1 duration_1 brightness_2 duration_2 brightness_3 duration_3
and so on...

 From what I understood blinking and breathing are different
patterns in case of discussed device, so they would have to be
initialized differently.

Documentation/devicetree/bindings/leds/leds-trigger-pattern.txt
shows how to approach both cases, i.e. for instant change
of brightness we need intervening tuple with duration time = 0.

Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
shows how hw breathing was approached in that case.

> 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

Yes, e.g. hw_control_set{_get}.

> 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 ?

Yes, since it will belong to LED core.

> 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?

In Documentation/ABI/testing/sysfs-class-led-cht-wc we should
have a description of hw_pattern semantics for Whiskey Cove PMIC LED,
with regard to how hw_control state impacts the mode of trigger
(manual/triggered when charging).

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

On 2/16/19 8:37 PM, Pavel Machek wrote:
> Hi!
> 
>>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>> 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 ?
> 
> 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.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 2/16/19 10:54 PM, Jacek Anaszewski wrote:
> On 2/16/19 8:37 PM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>>
>>>> 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 ?
>>
>> 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?

Regards,

Hans

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Jacek Anaszewski]

Hi,

On 2/16/19 11:03 PM, Hans de Goede wrote:
> Hi,
> 
> On 2/16/19 10:54 PM, Jacek Anaszewski wrote:
>> On 2/16/19 8:37 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> 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 ?
>>>
>>> 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?

OK, so we'll need to add hardware_controlled file to the LED core.
It should be exposed only when supported by the hardware, so we will
need the flag in leds.h:

#define LED_DEV_CAP_HW_CONTROL BIT(24)

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Pavel Machek]

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

Hi!

> >>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
> >
> >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.

BTW... all this should really go in the comment to the top of the
file. It is non-trivial.

> >>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.

As should this.... as it is quite unusual.

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

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

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Yauhen Kharuzhy]

Hi!

On Thu, Feb 14, 2019 at 10:57:13AM +0100, Hans de Goede wrote:
 
> > 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]

Leds class API already has pattern_set()/pattern_get() callbacks
which can be used for breathing mode set/request. How this API should
deal with specific 'breathing' sysfs attribute? I like the idea about
specific 'pattern' attribute because this is very simple, anyway. 

Second issue: the 'pattern' led trigger supports default pattern initialization
by device properties only, not by hardware request. Setting breathing
parameters via 'pattern' led trigger may looks too complicated for
users, given that only one hw-supported option exists. Pattern
corresponded to hw-supported breating should contain too many
(brightness, duration) tuples with minimal stepping and has no
practical sense.


If hw-controlled 'breathing' mode is quite common case for leds
controllers, maybe it makes sense to introduce something like
'lighting mode' attribute with values 'continuous'/'breathing' without
clarification of breathing parameters? What do you think about this?

The same question about frequency of breathing setting. Blinking
frequency can be set with existing API, but not a breathing frequency.

> 
> 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

-- 
Yauhen Kharuzhy

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Yauhen Kharuzhy]

On Wed, Feb 13, 2019 at 11:43:29PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12-02-19 21:59, Yauhen Kharuzhy wrote:
> > 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

Maybe you are right, I will check but at Linux Yoga Book this LED
doesn't work as HW-controlled charging status indicator, so I didn't
discovery this.

I used this source as reference; https://github.com/jekhor/yogabook-linux-android-kernel/blob/cm-13.0/drivers/misc/charger_gp_led.c

> 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.

Jacek? Pavel? I thought about pattern_set() implementation for 'breathing'
mode in future but this doesn't seem simple, maybe custom attribute will has
sense?

> 
> 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
> 
> > +
> > +#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)
> > +
> > +#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,

Hm, it seems lost in refactoring time, thanks. My original defines were:

#define CHT_WC_LED_EFF_ON               (1<<1)
#define CHT_WC_LED_EFF_BLINKING         (2<<1)
#define CHT_WC_LED_EFF_BREATHING        (3<<1)
#define CHT_WC_LED_EFF_MASK             0x06

I will revert this in next version.

-- 
Yauhen Kharuzhy

Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs

[Hans de Goede]

Hi,

On 13-02-19 23:43, Hans de Goede wrote:
> Hi,
> 
> On 12-02-19 21:59, Yauhen Kharuzhy wrote:
>> 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.

<snip>

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

Ok, scrap that I don't know where this info in my notes
came from (probably user error), the power-on-reset
value for bits 0-1 is 00 and then the hw controls the
charging LED just fine. So your code is right, bit 0
switches between hw-control (0) and user-control (1) and
bit 1 is on/off when user-control is enabled.

Regards,

Hans

[PATCH v2 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc

[Yauhen Kharuzhy]

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

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index 64a3aece9c5e..be84bb2aa837 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -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", },
 };
 
 /*
-- 
2.20.1

Re: [PATCH v2 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc

[Jacek Anaszewski]

Cc Lee.

On 2/12/19 9:59 PM, Yauhen Kharuzhy wrote:
> Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
> mfd device driver.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   drivers/mfd/intel_soc_pmic_chtwc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 64a3aece9c5e..be84bb2aa837 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -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", },
>   };
>   
>   /*
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc

[Lee Jones]

On Tue, 12 Feb 2019, Yauhen Kharuzhy wrote:

> Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
> mfd device driver.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>  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>

> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 64a3aece9c5e..be84bb2aa837 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -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", },
>  };
>  
>  /*

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc

[Lee Jones]

On Wed, 20 Mar 2019, Lee Jones wrote:

> On Tue, 12 Feb 2019, Yauhen Kharuzhy wrote:
> 
> > Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC
> > mfd device driver.
> > 
> > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > ---
> >  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>

Applied, by the way, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/2] Intel Cherry Trail Whiskey Cove LEDs support

[Hans de Goede]

Hi Yauhen,

On 12-02-19 21:58, Yauhen Kharuzhy wrote:
> 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

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.

Regards,

Hans

Re: [PATCH v2 0/2] Intel Cherry Trail Whiskey Cove LEDs support

[Yauhen Kharuzhy]

On Sun, Apr 21, 2019 at 09:28:42PM +0200, Hans de Goede wrote:
> 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.
> 

Hi,
Unfortunately I am too busy now at fulltime job, so I don't know when I
will return to this. But I still plan to do  :)
 

-- 
Yauhen Kharuzhy