[PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

Hi All,

Here is a patch series to add support for the LED controller on
Intel Cherry Trail Whiskey Cove PMICs.

This is based on the original patch for this from Yauhen Kharuzhy,
with additional work on top by me.

This addresses the review remarks on the v2 posting from Yauhen:
- Since the PMIC is connected to the battery any changes we make to
  the LED settings are permanent, even surviving reboot / poweroff.
  Save LED1 register settings on probe() and if auto-/hw-control was
  enabled on probe() restore the settings on remove() and shutdown().
- Add support for the pattern trigger to select breathing mode

This makes the charging LED on devices with these PMICs properly
reflect the charging status (this relies on sw control on most
devices) and this also allows control of the LED behind the pen
(digitizer on) symbol on the keyboard half of the Lenovo Yoga Book
1 models.

Regards,

Hans


Hans de Goede (4):
  leds: cht-wcove: Add suspend/resume handling
  leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs
    API
  leds: cht-wcove: Set default trigger for charging LED
  leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set

Yauhen Kharuzhy (1):
  leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

 Documentation/leds/index.rst          |   1 +
 Documentation/leds/leds-cht-wcove.rst |  29 ++
 drivers/leds/Kconfig                  |  11 +
 drivers/leds/Makefile                 |   1 +
 drivers/leds/leds-cht-wcove.c         | 466 ++++++++++++++++++++++++++
 5 files changed, 508 insertions(+)
 create mode 100644 Documentation/leds/leds-cht-wcove.rst
 create mode 100644 drivers/leds/leds-cht-wcove.c

-- 
2.39.1

[PATCH 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

From: Yauhen Kharuzhy <jekhor@gmail.com>

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.

Changes by Hans de Goede (in response to review of v2):
- Since the PMIC is connected to the battery any changes we make to
  the LED settings are permanent, even surviving reboot / poweroff.
  Save LED1 register settings on probe() and if auto-/hw-control was
  enabled on probe() restore the settings on remove() and shutdown().
- Delay switching LED1 to software control mode to first brightness write.
- Use dynamically allocated drvdata instead of a global drvdata variable.
- Ensure the LED is on when activating blinking.
- Fix CHT_WC_LED_EFF_BREATHING val ((3 << 1) rather then BIT(3)).

Link: https://lore.kernel.org/r/20190212205901.13037-2-jekhor@gmail.com
Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
Co-developed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/Kconfig          |  11 +
 drivers/leds/Makefile         |   1 +
 drivers/leds/leds-cht-wcove.c | 368 ++++++++++++++++++++++++++++++++++
 3 files changed, 380 insertions(+)
 create mode 100644 drivers/leds/leds-cht-wcove.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dbce09eabac..90835716f14a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -122,6 +122,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 d30395d11fd8..78b5b69f9c54 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
+obj-$(CONFIG_LEDS_CHT_WCOVE)		+= leds-cht-wcove.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
 obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
new file mode 100644
index 000000000000..06447804d050
--- /dev/null
+++ b/drivers/leds/leds-cht-wcove.c
@@ -0,0 +1,368 @@
+// 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>
+ * Copyright 2023 Hans de Goede <hansg@kernel.org>
+ *
+ * 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		GENMASK(5, 4)
+
+#define CHT_WC_LED_EFF_OFF		(0 << 1)
+#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		GENMASK(2, 1)
+
+#define CHT_WC_LED_COUNT		2
+
+struct cht_wc_led_regs {
+	/* Register addresses */
+	u16 ctrl;
+	u16 fsm;
+	u16 pwm;
+	/* Mask + values for turning the LED on/off */
+	u8 on_off_mask;
+	u8 on_val;
+	u8 off_val;
+};
+
+struct cht_wc_led_saved_regs {
+	unsigned int ctrl;
+	unsigned int fsm;
+	unsigned int pwm;
+};
+
+struct cht_wc_led {
+	struct led_classdev cdev;
+	const struct cht_wc_led_regs *regs;
+	struct regmap *regmap;
+	struct mutex mutex;
+};
+
+struct cht_wc_leds {
+	struct cht_wc_led leds[CHT_WC_LED_COUNT];
+	/* Saved LED1 initial register values */
+	struct cht_wc_led_saved_regs led1_initial_regs;
+};
+
+static const struct cht_wc_led_regs cht_wc_led_regs[CHT_WC_LED_COUNT] = {
+	{
+		.ctrl		= CHT_WC_LED1_CTRL,
+		.fsm		= CHT_WC_LED1_FSM,
+		.pwm		= CHT_WC_LED1_PWM,
+		.on_off_mask	= CHT_WC_LED1_SWCTL | CHT_WC_LED1_ON,
+		.on_val		= CHT_WC_LED1_SWCTL | CHT_WC_LED1_ON,
+		.off_val	= CHT_WC_LED1_SWCTL,
+	},
+	{
+		.ctrl		= CHT_WC_LED2_CTRL,
+		.fsm		= CHT_WC_LED2_FSM,
+		.pwm		= CHT_WC_LED2_PWM,
+		.on_off_mask	= CHT_WC_LED2_ON,
+		.on_val		= CHT_WC_LED2_ON,
+		.off_val	= 0,
+	},
+};
+
+static const char * const cht_wc_leds_names[CHT_WC_LED_COUNT] = {
+	"cht-wc::" LED_FUNCTION_CHARGING,
+	"cht-wc::" LED_FUNCTION_INDICATOR,
+};
+
+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->regmap, led->regs->ctrl,
+					 led->regs->on_off_mask, led->regs->off_val);
+		if (ret)
+			dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
+
+		/* Disable HW blinking */
+		ret = regmap_update_bits(led->regmap, led->regs->fsm,
+					 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->regmap, led->regs->pwm, value);
+		if (ret)
+			dev_err(cdev->dev, "Failed to set brightness: %d\n", ret);
+
+		ret = regmap_update_bits(led->regmap, led->regs->ctrl,
+					 led->regs->on_off_mask, led->regs->on_val);
+		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);
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&led->mutex);
+
+	ret = regmap_read(led->regmap, led->regs->ctrl, &val);
+	if (ret < 0) {
+		dev_err(cdev->dev, "Failed to read LED CTRL reg: %d\n", ret);
+		ret = LED_OFF;
+		goto done;
+	}
+
+	val &= led->regs->on_off_mask;
+	if (val != led->regs->on_val) {
+		ret = LED_OFF;
+		goto done;
+	}
+
+	ret = regmap_read(led->regmap, led->regs->pwm, &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->regmap, led->regs->fsm,
+					 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->regmap, led->regs->fsm,
+				 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);
+
+	/* Set the frequency and make sure the LED is on */
+	ret = regmap_update_bits(led->regmap, led->regs->ctrl,
+				 CHT_WC_LED_F_MASK | led->regs->on_off_mask,
+				 ctrl | led->regs->on_val);
+	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_led_save_regs(struct cht_wc_led *led,
+				struct cht_wc_led_saved_regs *saved_regs)
+{
+	int ret;
+
+	ret = regmap_read(led->regmap, led->regs->ctrl, &saved_regs->ctrl);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(led->regmap, led->regs->fsm, &saved_regs->fsm);
+	if (ret < 0)
+		return ret;
+
+	return regmap_read(led->regmap, led->regs->pwm, &saved_regs->pwm);
+}
+
+static void cht_wc_led_restore_regs(struct cht_wc_led *led,
+				    const struct cht_wc_led_saved_regs *saved_regs)
+{
+	regmap_write(led->regmap, led->regs->ctrl, saved_regs->ctrl);
+	regmap_write(led->regmap, led->regs->fsm, saved_regs->fsm);
+	regmap_write(led->regmap, led->regs->pwm, saved_regs->pwm);
+}
+
+static int cht_wc_leds_probe(struct platform_device *pdev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	struct cht_wc_leds *leds;
+	int ret;
+	int i;
+
+	leds = devm_kzalloc(&pdev->dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	/*
+	 * LED1 might be in hw-controlled mode when this driver gets loaded; and
+	 * since the PMIC is always powered by the battery any changes made are
+	 * permanent. Save LED1 regs to restore them on remove() or shutdown().
+	 */
+	leds->leds[0].regs = &cht_wc_led_regs[0];
+	leds->leds[0].regmap = pmic->regmap;
+	ret = cht_wc_led_save_regs(&leds->leds[0], &leds->led1_initial_regs);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < CHT_WC_LED_COUNT; i++) {
+		struct cht_wc_led *led = &leds->leds[i];
+
+		led->regs = &cht_wc_led_regs[i];
+		led->regmap = pmic->regmap;
+		mutex_init(&led->mutex);
+		led->cdev.name = cht_wc_leds_names[i];
+		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;
+
+		ret = led_classdev_register(&pdev->dev, &led->cdev);
+		if (ret < 0)
+			return ret;
+	}
+
+	platform_set_drvdata(pdev, leds);
+	return 0;
+}
+
+static void cht_wc_leds_remove(struct platform_device *pdev)
+{
+	struct cht_wc_leds *leds = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < CHT_WC_LED_COUNT; i++)
+		led_classdev_unregister(&leds->leds[i].cdev);
+
+	/* Restore LED1 regs if hw-control was active, else leave LED1 off. */
+	if (!(leds->led1_initial_regs.ctrl & CHT_WC_LED1_SWCTL))
+		cht_wc_led_restore_regs(&leds->leds[0], &leds->led1_initial_regs);
+}
+
+static void cht_wc_leds_disable(struct platform_device *pdev)
+{
+	struct cht_wc_leds *leds = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < CHT_WC_LED_COUNT; i++)
+		cht_wc_leds_brightness_set(&leds->leds[i].cdev, LED_OFF);
+
+	/* Restore LED1 regs if hw-control was active, else leave LED1 off. */
+	if (!(leds->led1_initial_regs.ctrl & CHT_WC_LED1_SWCTL))
+		cht_wc_led_restore_regs(&leds->leds[0], &leds->led1_initial_regs);
+}
+
+static struct platform_driver cht_wc_leds_driver = {
+	.probe = cht_wc_leds_probe,
+	.remove_new = cht_wc_leds_remove,
+	.shutdown = cht_wc_leds_disable,
+	.driver = {
+		.name = "cht_wcove_leds",
+	},
+};
+module_platform_driver(cht_wc_leds_driver);
+
+MODULE_ALIAS("platform:cht_wcove_leds");
+MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove PMIC LEDs driver");
+MODULE_AUTHOR("Yauhen Kharuzhy <jekhor@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.39.1

[PATCH 2/5] leds: cht-wcove: Add suspend/resume handling

[Hans de Goede]

When LED1 is showing the tablet is charging and then the device gets
suspended followed by unplugging the charger, then it will incorrectly
still show it is charging.

To avoid this turn both LEDs off on suspend, just like the PMIC always
turns them off when the tablet is powered off (even if the tablet is
charging). If hw-control is supported for LED1, then restore the
initial hw-control settings to let the hw control LED1 while suspended.

To restore the state the LEDs had before suspending, save it before
turning the LEDs off and restore it on resume.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/leds-cht-wcove.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
index 06447804d050..2d968ddd18c5 100644
--- a/drivers/leds/leds-cht-wcove.c
+++ b/drivers/leds/leds-cht-wcove.c
@@ -15,6 +15,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/suspend.h>
 
 #define CHT_WC_LED1_CTRL		0x5e1f
 #define CHT_WC_LED1_FSM			0x5e20
@@ -69,6 +70,7 @@ struct cht_wc_led {
 	const struct cht_wc_led_regs *regs;
 	struct regmap *regmap;
 	struct mutex mutex;
+	struct cht_wc_led_saved_regs saved_regs;
 };
 
 struct cht_wc_leds {
@@ -352,12 +354,43 @@ static void cht_wc_leds_disable(struct platform_device *pdev)
 		cht_wc_led_restore_regs(&leds->leds[0], &leds->led1_initial_regs);
 }
 
+/* On suspend save current settings and turn LEDs off */
+static int cht_wc_leds_suspend(struct device *dev)
+{
+	struct cht_wc_leds *leds = dev_get_drvdata(dev);
+	int i, ret;
+
+	for (i = 0; i < CHT_WC_LED_COUNT; i++) {
+		ret = cht_wc_led_save_regs(&leds->leds[i], &leds->leds[i].saved_regs);
+		if (ret < 0)
+			return ret;
+	}
+
+	cht_wc_leds_disable(to_platform_device(dev));
+	return 0;
+}
+
+/* On resume restore the saveds settings */
+static int cht_wc_leds_resume(struct device *dev)
+{
+	struct cht_wc_leds *leds = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < CHT_WC_LED_COUNT; i++)
+		cht_wc_led_restore_regs(&leds->leds[i], &leds->leds[i].saved_regs);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(cht_wc_leds_pm, cht_wc_leds_suspend, cht_wc_leds_resume);
+
 static struct platform_driver cht_wc_leds_driver = {
 	.probe = cht_wc_leds_probe,
 	.remove_new = cht_wc_leds_remove,
 	.shutdown = cht_wc_leds_disable,
 	.driver = {
 		.name = "cht_wcove_leds",
+		.pm = pm_sleep_ptr(&cht_wc_leds_pm),
 	},
 };
 module_platform_driver(cht_wc_leds_driver);
-- 
2.39.1

[PATCH 3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

[Hans de Goede]

The hw-blinking of the LED controller in the Whiskey Cove PMIC can also
be used for a hw-breathing effect.

As discussed during review of v2 of the submission of the new
leds-cht-wcove driver, the LED subsystem already supports breathing mode
on several other LED controllers using the hw_pattern interface.

Implement a pattern_set callback to implement breathing mode modelled
after the breathing mode supported by the SC27xx breathing light and
Crane EL15203000 LED drivers. The Whiskey Cove PMIC's breathing mode
is closer to the EL15203000 one then to the SC27xx one since it does
not support staying high / low for a specific time, it only supports
rise and fall times.

As such the supported hw_pattern and the documentation for this is almost
a 1:1 copy of the pattern/docs for the EL15203000 breathing mode.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/leds/index.rst          |  1 +
 Documentation/leds/leds-cht-wcove.rst | 29 ++++++++++++++++++
 drivers/leds/leds-cht-wcove.c         | 42 ++++++++++++++++++++++++---
 3 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/leds/leds-cht-wcove.rst

diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index b9ca081fac71..c92612271e25 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -17,6 +17,7 @@ LEDs
    uleds
 
    leds-blinkm
+   leds-cht-wcove
    leds-el15203000
    leds-lm3556
    leds-lp3944
diff --git a/Documentation/leds/leds-cht-wcove.rst b/Documentation/leds/leds-cht-wcove.rst
new file mode 100644
index 000000000000..fa79d8fd7ef8
--- /dev/null
+++ b/Documentation/leds/leds-cht-wcove.rst
@@ -0,0 +1,29 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================================================
+Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
+===========================================================
+
+/sys/class/leds/<led>/hw_pattern
+--------------------------------
+
+Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
+
+The only supported pattern is hardware breathing mode::
+
+    "0 2000 1 2000"
+
+	^
+	|
+    Max-|     ---
+	|    /   \
+	|   /     \
+	|  /       \     /
+	| /         \   /
+    Min-|-           ---
+	|
+	0------2------4--> time (sec)
+
+The rise and fall times must be the same value.
+Supported values are 2000, 1000, 500 and 250 for
+breathing frequencies of 1/4, 1/2, 1 and 2 Hz.
diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
index 2d968ddd18c5..2b6c6f39d406 100644
--- a/drivers/leds/leds-cht-wcove.c
+++ b/drivers/leds/leds-cht-wcove.c
@@ -214,9 +214,10 @@ static int cht_wc_leds_find_freq(unsigned long period)
 		return -1;
 }
 
-static int cht_wc_leds_blink_set(struct led_classdev *cdev,
-				 unsigned long *delay_on,
-				 unsigned long *delay_off)
+static int cht_wc_leds_set_effect(struct led_classdev *cdev,
+				  unsigned long *delay_on,
+				  unsigned long *delay_off,
+				  u8 effect)
 {
 	struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
 	unsigned int ctrl;
@@ -242,7 +243,7 @@ static int cht_wc_leds_blink_set(struct led_classdev *cdev,
 	}
 
 	ret = regmap_update_bits(led->regmap, led->regs->fsm,
-				 CHT_WC_LED_EFF_MASK, CHT_WC_LED_EFF_BLINKING);
+				 CHT_WC_LED_EFF_MASK, effect);
 	if (ret < 0)
 		dev_err(cdev->dev, "Failed to update LED FSM reg: %d\n", ret);
 
@@ -261,6 +262,37 @@ static int cht_wc_leds_blink_set(struct led_classdev *cdev,
 	return ret;
 }
 
+static int cht_wc_leds_blink_set(struct led_classdev *cdev,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	return cht_wc_leds_set_effect(cdev, delay_on, delay_off, CHT_WC_LED_EFF_BLINKING);
+}
+
+static int cht_wc_leds_pattern_set(struct led_classdev *cdev,
+				   struct led_pattern *pattern,
+				   u32 len, int repeat)
+{
+	unsigned long delay_off, delay_on;
+
+	if (repeat > 0 || len != 2 ||
+	    pattern[0].brightness != LED_OFF || pattern[1].brightness != LED_ON ||
+	    pattern[0].delta_t != pattern[1].delta_t ||
+	    (pattern[0].delta_t != 250 && pattern[0].delta_t != 500 &&
+	     pattern[0].delta_t != 1000 && pattern[0].delta_t != 2000))
+		return -EINVAL;
+
+	delay_off = pattern[0].delta_t;
+	delay_on  = pattern[1].delta_t;
+
+	return cht_wc_leds_set_effect(cdev, &delay_on, &delay_off, CHT_WC_LED_EFF_BREATHING);
+}
+
+static int cht_wc_leds_pattern_clear(struct led_classdev *cdev)
+{
+	return cht_wc_leds_brightness_set(cdev, LED_OFF);
+}
+
 static int cht_wc_led_save_regs(struct cht_wc_led *led,
 				struct cht_wc_led_saved_regs *saved_regs)
 {
@@ -317,6 +349,8 @@ static int cht_wc_leds_probe(struct platform_device *pdev)
 		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.pattern_set = cht_wc_leds_pattern_set;
+		led->cdev.pattern_clear = cht_wc_leds_pattern_clear;
 		led->cdev.max_brightness = 255;
 
 		ret = led_classdev_register(&pdev->dev, &led->cdev);
-- 
2.39.1

[PATCH 4/5] leds: cht-wcove: Set default trigger for charging LED

[Hans de Goede]

Set a default trigger for the charging LED based on the machine-model
as set by the PMIC MFD driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/leds-cht-wcove.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
index 2b6c6f39d406..597bfbe19cc2 100644
--- a/drivers/leds/leds-cht-wcove.c
+++ b/drivers/leds/leds-cht-wcove.c
@@ -339,6 +339,25 @@ static int cht_wc_leds_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	/* Set LED1 default trigger based on machine model */
+	switch (pmic->cht_wc_model) {
+	case INTEL_CHT_WC_GPD_WIN_POCKET:
+		leds->leds[0].cdev.default_trigger = "max170xx_battery-charging-blink-full-solid";
+		break;
+	case INTEL_CHT_WC_XIAOMI_MIPAD2:
+		leds->leds[0].cdev.default_trigger = "bq27520-0-charging-blink-full-solid";
+		break;
+	case INTEL_CHT_WC_LENOVO_YOGABOOK1:
+		leds->leds[0].cdev.default_trigger = "bq27542-0-charging-blink-full-solid";
+		break;
+	case INTEL_CHT_WC_LENOVO_YT3_X90:
+		leds->leds[0].cdev.default_trigger = "bq27500-1-charging-blink-full-solid";
+		break;
+	default:
+		dev_warn(&pdev->dev, "Unknown model, no default charging trigger\n");
+		break;
+	}
+
 	for (i = 0; i < CHT_WC_LED_COUNT; i++) {
 		struct cht_wc_led *led = &leds->leds[i];
 
-- 
2.39.1

[PATCH 5/5] leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set

[Hans de Goede]

The desired default behavior of LED1 / the charge LED is breathing
while charging and on/solid when full. Since triggers cannot select
breathing, blink_set() gets called when charging. Use breathing
when the default "charging-blink-full-solid" trigger is used to
achieve the desired default behavior.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/leds-cht-wcove.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
index 597bfbe19cc2..125e9331348c 100644
--- a/drivers/leds/leds-cht-wcove.c
+++ b/drivers/leds/leds-cht-wcove.c
@@ -266,7 +266,19 @@ static int cht_wc_leds_blink_set(struct led_classdev *cdev,
 				 unsigned long *delay_on,
 				 unsigned long *delay_off)
 {
-	return cht_wc_leds_set_effect(cdev, delay_on, delay_off, CHT_WC_LED_EFF_BLINKING);
+	u8 effect = CHT_WC_LED_EFF_BLINKING;
+
+	/*
+	 * The desired default behavior of LED1 / the charge LED is breathing
+	 * while charging and on/solid when full. Since triggers cannot select
+	 * breathing, blink_set() gets called when charging. Use breathing
+	 * when the default "charging-blink-full-solid" trigger is used to
+	 * achieve the desired default behavior.
+	 */
+	if (cdev->flags & LED_INIT_DEFAULT_TRIGGER)
+		effect = CHT_WC_LED_EFF_BREATHING;
+
+	return cht_wc_leds_set_effect(cdev, delay_on, delay_off, effect);
 }
 
 static int cht_wc_leds_pattern_set(struct led_classdev *cdev,
-- 
2.39.1

Re: [PATCH 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Pavel Machek]

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

On Thu 2023-04-13 17:18:04, Hans de Goede wrote:
> From: Yauhen Kharuzhy <jekhor@gmail.com>
> 
> 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.

leds->LEDs.

> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
> new file mode 100644
> index 000000000000..06447804d050
> --- /dev/null
> +++ b/drivers/leds/leds-cht-wcove.c
> @@ -0,0 +1,368 @@
> +// 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>
> + * Copyright 2023 Hans de Goede <hansg@kernel.org>
> + *
> + * Based on Lenovo Yoga Book Android kernel sources
> + */

"sources." Should copyrights from Android be copied here?

> +static const char * const cht_wc_leds_names[CHT_WC_LED_COUNT] = {
> +	"cht-wc::" LED_FUNCTION_CHARGING,
> +	"cht-wc::" LED_FUNCTION_INDICATOR,
> +};

No need for "cht-wc". Mention color. At least charging LED is
going to be common - document in Documentation/leds/well-known-leds.txt. 

> +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->regmap, led->regs->ctrl,
> +					 led->regs->on_off_mask, led->regs->off_val);
> +		if (ret)
> +			dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
> +
> +		/* Disable HW blinking */
> +		ret = regmap_update_bits(led->regmap, led->regs->fsm,
> +					 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);

This overwrites previous error. Not sure if it is big deal.

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

That's really slow blink; I'd do something faster by default.

> +	/*
> +	 * LED1 might be in hw-controlled mode when this driver gets loaded; and
> +	 * since the PMIC is always powered by the battery any changes made are
> +	 * permanent. Save LED1 regs to restore them on remove() or shutdown().
> +	 */

Fun :-).

Thank you.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/5] leds: cht-wcove: Add suspend/resume handling

[Pavel Machek]

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

Hi!

> +
> +/* On resume restore the saveds settings */

-> saved.

Acked-by: Pavel Machek <pavel@ucw.cz>
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Yauhen Kharuzhy]

On Thu, Apr 13, 2023 at 05:18:03PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is a patch series to add support for the LED controller on
> Intel Cherry Trail Whiskey Cove PMICs.
> 
> This is based on the original patch for this from Yauhen Kharuzhy,
> with additional work on top by me.
> 
> This addresses the review remarks on the v2 posting from Yauhen:
> - Since the PMIC is connected to the battery any changes we make to
>   the LED settings are permanent, even surviving reboot / poweroff.
>   Save LED1 register settings on probe() and if auto-/hw-control was
>   enabled on probe() restore the settings on remove() and shutdown().
> - Add support for the pattern trigger to select breathing mode
> 
> This makes the charging LED on devices with these PMICs properly
> reflect the charging status (this relies on sw control on most
> devices) and this also allows control of the LED behind the pen
> (digitizer on) symbol on the keyboard half of the Lenovo Yoga Book
> 1 models.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (4):
>   leds: cht-wcove: Add suspend/resume handling
>   leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs
>     API
>   leds: cht-wcove: Set default trigger for charging LED
>   leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set
> 
> Yauhen Kharuzhy (1):
>   leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver
> 
>  Documentation/leds/index.rst          |   1 +
>  Documentation/leds/leds-cht-wcove.rst |  29 ++
>  drivers/leds/Kconfig                  |  11 +
>  drivers/leds/Makefile                 |   1 +
>  drivers/leds/leds-cht-wcove.c         | 466 ++++++++++++++++++++++++++
>  5 files changed, 508 insertions(+)
>  create mode 100644 Documentation/leds/leds-cht-wcove.rst
>  create mode 100644 drivers/leds/leds-cht-wcove.c

Hi Hans,

Thanks for reviving this patch!

I haven't tested it on linux-next yet but on v6.2.11 (with few patches for
Yoabook) I catched following trace. I will investigate it later but maybe you
can take a look also?

[  192.585809] bq25890-charger i2c-bq25892: S:CHG/PG/VSYS=2/1/0, F:CHG/BOOST/BAT/NTC=0/0/0/0
[  196.753095] bq25890-charger i2c-bq25892: Start to request input voltage increasing
[  196.770555] bq25890-charger i2c-bq25892: input voltage = 4000000 uV
[  200.473777] bq25890-charger i2c-bq25892: input voltage = 5900000 uV
[  204.187438] bq25890-charger i2c-bq25892: input voltage = 7900000 uV
[  207.864890] bq25890-charger i2c-bq25892: input voltage = 11000000 uV
[  211.573333] bq25890-charger i2c-bq25892: input voltage = 10900000 uV
[  215.222692] bq25890-charger i2c-bq25892: input voltage = 11000000 uV
[  218.937871] bq25890-charger i2c-bq25892: Hi-voltage charging requested, input voltage is 11000000 mV
[  242.786148] systemd-journald[270]: Data hash table of /var/log/journal/39241a358af746c292cb608baea5be4c/system.journal has a fill level at 75.0 (8533 of 11377 items, 6553600 file size, 768 bytes per hash table item), suggesting rotation.
[  242.786185] systemd-journald[270]: /var/log/journal/39241a358af746c292cb608baea5be4c/system.journal: Journal header limits reached or header out-of-date, rotating.
[  257.790169] ------------[ cut here ]------------
[  257.790223] Voluntary context switch within RCU read-side critical section!
[  257.790256] WARNING: CPU: 3 PID: 69 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x4e3/0x6c0
[  257.790337] Modules linked in: rfcomm(E) snd_seq_dummy(E) snd_hrtimer(E) snd_seq(E) snd_seq_device(E) cmac(E) algif_hash(E) algif_skcipher(E) af_alg(E) bnep(E) uinput(E) binfmt_misc(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) snd_soc_sst_cht_yogabook(E) snd_soc_ts3a227e(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) ghash_clmulni_intel(E) snd_sof_acpi_intel_byt(E) snd_sof_acpi(E) sha512_ssse3(E) snd_sof_intel_atom(E) snd_sof(E) snd_sof_utils(E) snd_sof_xtensa_dsp(E) aesni_intel(E) snd_intel_sst_acpi(E) crypto_simd(E) snd_intel_sst_core(E) cryptd(E) gpio_keys(E) intel_rapl_msr(E) brcmfmac_wcc(E) intel_cstate(E) lenovo_yogabook_wmi(E) wmi_bmof(E) pcspkr(E) hci_uart(E) btqca(E) snd_soc_sst_atom_hifi2_platform(E) btbcm(E) snd_soc_rt5677(E) bq25890_charger(E) btintel(E) snd_soc_rt5677_spi(E) snd_soc_acpi_intel_match(E) joydev(E) leds_cht_wcove(E) extcon_intel_cht_wc(E) hid_multitouch(E) brcmfmac(E) snd_soc_acpi(E) snd_soc_rl6231(E) bluetooth(E) brcmutil(E) bq27xxx_battery_i2c(E) nls_ascii(E)
[  257.790879]  bq27xxx_battery(E) nls_cp437(E) hid_sensor_accel_3d(E) hid_sensor_custom_intel_hinge(E) hid_sensor_als(E) spi_nor(E) snd_soc_core(E) iTCO_wdt(E) hid_sensor_trigger(E) vfat(E) snd_compress(E) cfg80211(E) mtd(E) iTCO_vendor_support(E) hid_sensor_iio_common(E) snd_hdmi_lpe_audio(E) fat(E) snd_intel_dspcfg(E) jitterentropy_rng(E) sx9310(E) snd_pcm(E) sx_common(E) tpm_crb(E) snd_timer(E) industrialio_triggered_buffer(E) goodix_ts(E) drbg(E) kfifo_buf(E) intel_hid(E) sparse_keymap(E) snd(E) rfkill_gpio(E) soundcore(E) tpm_tis(E) industrialio(E) ansi_cprng(E) tpm_tis_core(E) ecdh_generic(E) rfkill(E) ecc(E) tpm(E) processor_thermal_device_pci_legacy(E) processor_thermal_device(E) processor_thermal_rfim(E) rng_core(E) processor_thermal_mbox(E) processor_thermal_rapl(E) intel_rapl_common(E) int3400_thermal(E) int3406_thermal(E) int3403_thermal(E) soc_button_array(E) acpi_thermal_rel(E) int340x_thermal_zone(E) intel_int0002_vgpio(E) evdev(E) intel_soc_dts_iosf(E) acpi_pad(E)
[  257.791393]  squashfs(E) loop(E) ramoops(E) reed_solomon(E) intel_pstore_pram(E) msr(E) parport_pc(E) ppdev(E) lp(E) parport(E) fuse(E) efi_pstore(E) configfs(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) hid_sensor_custom(E) hid_sensor_hub(E) intel_ishtp_hid(E) wacom(E) dwc3(E) usbhid(E) ulpi(E) hid_generic(E) udc_core(E) i915(E) xhci_pci(E) spi_intel_platform(E) i2c_algo_bit(E) spi_intel(E) mmc_block(E) xhci_hcd(E) drm_buddy(E) drm_display_helper(E) cec(E) ttm(E) usbcore(E) video(E) drm_kms_helper(E) intel_ish_ipc(E) crc32_pclmul(E) spi_pxa2xx_platform(E) crc32c_intel(E) i2c_cht_wc(E) i2c_hid_acpi(E) lpc_ich(E) dwc3_pci(E) usb_common(E) drm(E) intel_ishtp(E) thermal(E) i2c_hid(E) hid(E) wmi(E) sdhci_acpi(E) dw_dmac(E) sdhci(E) pwm_lpss_platform(E) pwm_lpss(E) dw_dmac_core(E) mmc_core(E) button(E)
[  257.792047] CPU: 3 PID: 69 Comm: kworker/3:2 Tainted: G            E      6.2.11-yogabook1 #57
[  257.792084] Hardware name: LENOVO Lenovo YB1-X91L/INVALID, BIOS 04WT18WW 08/26/2016
[  257.792110] Workqueue: events power_supply_changed_work
[  257.792163] RIP: 0010:rcu_note_context_switch+0x4e3/0x6c0
[  257.792206] Code: 49 89 3e 49 83 bc 24 98 00 00 00 00 0f 85 6c fe ff ff e9 5e fe ff ff 48 c7 c7 f0 5e d0 be c6 05 e7 bb 45 01 01 e8 ed 66 f6 ff <0f> 0b e9 7c fb ff ff a9 ff ff ff 7f 0f 84 38 fc ff ff 65 48 8b 3c
[  257.792240] RSP: 0018:ffffb357003ef830 EFLAGS: 00010046
[  257.792275] RAX: 0000000000000000 RBX: ffff95dd3abae280 RCX: 0000000000000000
[  257.792302] RDX: 0000000000000003 RSI: ffffffffbed4bfb7 RDI: 00000000ffffffff
[  257.792327] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffb357003ef6e0
[  257.792352] R10: 0000000000000003 R11: ffff95dd3fbfefe8 R12: 000000000002d400
[  257.792376] R13: ffff95dcca22cc40 R14: 0000000000000000 R15: ffff95dcca22cc40
[  257.792403] FS:  0000000000000000(0000) GS:ffff95dd3ab80000(0000) knlGS:0000000000000000
[  257.792431] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  257.792456] CR2: 000055cdc7a88670 CR3: 0000000155c0a000 CR4: 00000000001006e0
[  257.792485] Call Trace:
[  257.792511]  <TASK>
[  257.792535]  ? _raw_spin_lock_irqsave+0x23/0x50
[  257.792579]  ? preempt_count_add+0x62/0xa0
[  257.792625]  __schedule+0xac/0x9d0
[  257.792663]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[  257.792700]  ? __mod_timer+0x28a/0x3c0
[  257.792743]  schedule+0x5d/0xe0
[  257.792775]  schedule_timeout+0x8a/0x160
[  257.792812]  ? __bpf_trace_tick_stop+0x10/0x10
[  257.792849]  msleep+0x29/0x40
[  257.792885]  acpi_lpss_resume+0x6e/0x160
[  257.792924]  ? acpi_lpss_resume+0x160/0x160
[  257.792958]  acpi_lpss_runtime_resume+0xe/0x20
[  257.792993]  __rpm_callback+0x44/0x170
[  257.793029]  ? acpi_lpss_resume+0x160/0x160
[  257.793065]  rpm_callback+0x59/0x70
[  257.793100]  ? acpi_lpss_resume+0x160/0x160
[  257.793135]  rpm_resume+0x5f8/0x850
[  257.793168]  ? acpi_ut_update_ref_count.part.0+0x7a/0x8f0
[  257.793215]  __pm_runtime_resume+0x3c/0x60
[  257.793251]  i2c_dw_xfer+0x47/0x490
[  257.793291]  __i2c_transfer+0x16e/0x6c0
[  257.793335]  i2c_smbus_xfer_emulated+0x26f/0x9c0
[  257.793375]  ? __switch_to+0x7e/0x420
[  257.793410]  ? update_load_avg+0x80/0x790
[  257.793447]  __i2c_smbus_xfer+0xab/0x410
[  257.793477]  ? enqueue_entity+0x19a/0x4f0
[  257.793514]  i2c_smbus_xfer+0x6a/0xe0
[  257.793547]  i2c_smbus_read_byte_data+0x45/0x70
[  257.793578]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[  257.793615]  ? try_to_wake_up+0x95/0x590
[  257.793653]  cht_wc_byte_reg_read+0x2e/0x60
[  257.793701]  _regmap_read+0x5a/0x110
[  257.793737]  _regmap_update_bits+0xb4/0xf0
[  257.793775]  regmap_update_bits_base+0x59/0x80
[  257.793816]  cht_wc_leds_set_effect+0xcb/0x1b0 [leds_cht_wcove]
[  257.793875]  led_blink_setup+0x25/0x100
[  257.793912]  led_trigger_blink+0x45/0x70
[  257.793947]  power_supply_update_leds+0x1d6/0x1e0
[  257.793991]  power_supply_changed_work+0x6f/0xf0
[  257.794029]  process_one_work+0x1c7/0x3c0
[  257.794065]  worker_thread+0x4e/0x3a0
[  257.794095]  ? _raw_spin_lock_irqsave+0x23/0x50
[  257.794133]  ? rescuer_thread+0x3b0/0x3b0
[  257.794161]  kthread+0xe9/0x110
[  257.794195]  ? kthread_complete_and_exit+0x20/0x20
[  257.794233]  ret_from_fork+0x1f/0x30
[  257.794280]  </TASK>
[  257.794298] ---[ end trace 0000000000000000 ]---
[  257.854563] bq25890-charger i2c-bq25892: S:CHG/PG/VSYS=2/1/0, F:CHG/BOOST/BAT/NTC=0/0/0/0

-- 
Yauhen Kharuzhy

Re: [PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

Hi Yauhen,

On 4/15/23 22:32, Yauhen Kharuzhy wrote:
> On Thu, Apr 13, 2023 at 05:18:03PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is a patch series to add support for the LED controller on
>> Intel Cherry Trail Whiskey Cove PMICs.
>>
>> This is based on the original patch for this from Yauhen Kharuzhy,
>> with additional work on top by me.
>>
>> This addresses the review remarks on the v2 posting from Yauhen:
>> - Since the PMIC is connected to the battery any changes we make to
>>   the LED settings are permanent, even surviving reboot / poweroff.
>>   Save LED1 register settings on probe() and if auto-/hw-control was
>>   enabled on probe() restore the settings on remove() and shutdown().
>> - Add support for the pattern trigger to select breathing mode
>>
>> This makes the charging LED on devices with these PMICs properly
>> reflect the charging status (this relies on sw control on most
>> devices) and this also allows control of the LED behind the pen
>> (digitizer on) symbol on the keyboard half of the Lenovo Yoga Book
>> 1 models.
>>
>> Regards,
>>
>> Hans
>>
>>
>> Hans de Goede (4):
>>   leds: cht-wcove: Add suspend/resume handling
>>   leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs
>>     API
>>   leds: cht-wcove: Set default trigger for charging LED
>>   leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set
>>
>> Yauhen Kharuzhy (1):
>>   leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver
>>
>>  Documentation/leds/index.rst          |   1 +
>>  Documentation/leds/leds-cht-wcove.rst |  29 ++
>>  drivers/leds/Kconfig                  |  11 +
>>  drivers/leds/Makefile                 |   1 +
>>  drivers/leds/leds-cht-wcove.c         | 466 ++++++++++++++++++++++++++
>>  5 files changed, 508 insertions(+)
>>  create mode 100644 Documentation/leds/leds-cht-wcove.rst
>>  create mode 100644 drivers/leds/leds-cht-wcove.c
> 
> Hi Hans,
> 
> Thanks for reviving this patch!

You're welcome.

> I haven't tested it on linux-next yet but on v6.2.11 (with few patches for
> Yoabook) I catched following trace. I will investigate it later but maybe you
> can take a look also?

Right, this is an unrelated pre-existing kernel bug when using
led_trigger_blink().

I already hit that myself and I have a fix for it, see this series:
https://lore.kernel.org/linux-leds/20230412215855.593541-1-hdegoede@redhat.com/

And for the blinking -> solid transition to work when the battery goes
from charging to full you also need:
https://lore.kernel.org/linux-pm/20230413100941.115529-1-hdegoede@redhat.com/

I guess I should have mentioned both in the cover-letter, sorry about that.

And related to this there are also these 2 power-supply patch series
which are also relevant for the yogabook:

https://lore.kernel.org/linux-pm/20230415160734.70475-1-hdegoede@redhat.com/
https://lore.kernel.org/linux-pm/20230415182341.86916-1-hdegoede@redhat.com/

Regards,

Hans







> [  192.585809] bq25890-charger i2c-bq25892: S:CHG/PG/VSYS=2/1/0, F:CHG/BOOST/BAT/NTC=0/0/0/0
> [  196.753095] bq25890-charger i2c-bq25892: Start to request input voltage increasing
> [  196.770555] bq25890-charger i2c-bq25892: input voltage = 4000000 uV
> [  200.473777] bq25890-charger i2c-bq25892: input voltage = 5900000 uV
> [  204.187438] bq25890-charger i2c-bq25892: input voltage = 7900000 uV
> [  207.864890] bq25890-charger i2c-bq25892: input voltage = 11000000 uV
> [  211.573333] bq25890-charger i2c-bq25892: input voltage = 10900000 uV
> [  215.222692] bq25890-charger i2c-bq25892: input voltage = 11000000 uV
> [  218.937871] bq25890-charger i2c-bq25892: Hi-voltage charging requested, input voltage is 11000000 mV
> [  242.786148] systemd-journald[270]: Data hash table of /var/log/journal/39241a358af746c292cb608baea5be4c/system.journal has a fill level at 75.0 (8533 of 11377 items, 6553600 file size, 768 bytes per hash table item), suggesting rotation.
> [  242.786185] systemd-journald[270]: /var/log/journal/39241a358af746c292cb608baea5be4c/system.journal: Journal header limits reached or header out-of-date, rotating.
> [  257.790169] ------------[ cut here ]------------
> [  257.790223] Voluntary context switch within RCU read-side critical section!
> [  257.790256] WARNING: CPU: 3 PID: 69 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x4e3/0x6c0
> [  257.790337] Modules linked in: rfcomm(E) snd_seq_dummy(E) snd_hrtimer(E) snd_seq(E) snd_seq_device(E) cmac(E) algif_hash(E) algif_skcipher(E) af_alg(E) bnep(E) uinput(E) binfmt_misc(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) snd_soc_sst_cht_yogabook(E) snd_soc_ts3a227e(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) ghash_clmulni_intel(E) snd_sof_acpi_intel_byt(E) snd_sof_acpi(E) sha512_ssse3(E) snd_sof_intel_atom(E) snd_sof(E) snd_sof_utils(E) snd_sof_xtensa_dsp(E) aesni_intel(E) snd_intel_sst_acpi(E) crypto_simd(E) snd_intel_sst_core(E) cryptd(E) gpio_keys(E) intel_rapl_msr(E) brcmfmac_wcc(E) intel_cstate(E) lenovo_yogabook_wmi(E) wmi_bmof(E) pcspkr(E) hci_uart(E) btqca(E) snd_soc_sst_atom_hifi2_platform(E) btbcm(E) snd_soc_rt5677(E) bq25890_charger(E) btintel(E) snd_soc_rt5677_spi(E) snd_soc_acpi_intel_match(E) joydev(E) leds_cht_wcove(E) extcon_intel_cht_wc(E) hid_multitouch(E) brcmfmac(E) snd_soc_acpi(E) snd_soc_rl6231(E) bluetooth(E) brcmutil(E) bq27xxx_battery_i2c(E) nls_ascii(E)
> [  257.790879]  bq27xxx_battery(E) nls_cp437(E) hid_sensor_accel_3d(E) hid_sensor_custom_intel_hinge(E) hid_sensor_als(E) spi_nor(E) snd_soc_core(E) iTCO_wdt(E) hid_sensor_trigger(E) vfat(E) snd_compress(E) cfg80211(E) mtd(E) iTCO_vendor_support(E) hid_sensor_iio_common(E) snd_hdmi_lpe_audio(E) fat(E) snd_intel_dspcfg(E) jitterentropy_rng(E) sx9310(E) snd_pcm(E) sx_common(E) tpm_crb(E) snd_timer(E) industrialio_triggered_buffer(E) goodix_ts(E) drbg(E) kfifo_buf(E) intel_hid(E) sparse_keymap(E) snd(E) rfkill_gpio(E) soundcore(E) tpm_tis(E) industrialio(E) ansi_cprng(E) tpm_tis_core(E) ecdh_generic(E) rfkill(E) ecc(E) tpm(E) processor_thermal_device_pci_legacy(E) processor_thermal_device(E) processor_thermal_rfim(E) rng_core(E) processor_thermal_mbox(E) processor_thermal_rapl(E) intel_rapl_common(E) int3400_thermal(E) int3406_thermal(E) int3403_thermal(E) soc_button_array(E) acpi_thermal_rel(E) int340x_thermal_zone(E) intel_int0002_vgpio(E) evdev(E) intel_soc_dts_iosf(E) acpi_pad(E)
> [  257.791393]  squashfs(E) loop(E) ramoops(E) reed_solomon(E) intel_pstore_pram(E) msr(E) parport_pc(E) ppdev(E) lp(E) parport(E) fuse(E) efi_pstore(E) configfs(E) efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) hid_sensor_custom(E) hid_sensor_hub(E) intel_ishtp_hid(E) wacom(E) dwc3(E) usbhid(E) ulpi(E) hid_generic(E) udc_core(E) i915(E) xhci_pci(E) spi_intel_platform(E) i2c_algo_bit(E) spi_intel(E) mmc_block(E) xhci_hcd(E) drm_buddy(E) drm_display_helper(E) cec(E) ttm(E) usbcore(E) video(E) drm_kms_helper(E) intel_ish_ipc(E) crc32_pclmul(E) spi_pxa2xx_platform(E) crc32c_intel(E) i2c_cht_wc(E) i2c_hid_acpi(E) lpc_ich(E) dwc3_pci(E) usb_common(E) drm(E) intel_ishtp(E) thermal(E) i2c_hid(E) hid(E) wmi(E) sdhci_acpi(E) dw_dmac(E) sdhci(E) pwm_lpss_platform(E) pwm_lpss(E) dw_dmac_core(E) mmc_core(E) button(E)
> [  257.792047] CPU: 3 PID: 69 Comm: kworker/3:2 Tainted: G            E      6.2.11-yogabook1 #57
> [  257.792084] Hardware name: LENOVO Lenovo YB1-X91L/INVALID, BIOS 04WT18WW 08/26/2016
> [  257.792110] Workqueue: events power_supply_changed_work
> [  257.792163] RIP: 0010:rcu_note_context_switch+0x4e3/0x6c0
> [  257.792206] Code: 49 89 3e 49 83 bc 24 98 00 00 00 00 0f 85 6c fe ff ff e9 5e fe ff ff 48 c7 c7 f0 5e d0 be c6 05 e7 bb 45 01 01 e8 ed 66 f6 ff <0f> 0b e9 7c fb ff ff a9 ff ff ff 7f 0f 84 38 fc ff ff 65 48 8b 3c
> [  257.792240] RSP: 0018:ffffb357003ef830 EFLAGS: 00010046
> [  257.792275] RAX: 0000000000000000 RBX: ffff95dd3abae280 RCX: 0000000000000000
> [  257.792302] RDX: 0000000000000003 RSI: ffffffffbed4bfb7 RDI: 00000000ffffffff
> [  257.792327] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffb357003ef6e0
> [  257.792352] R10: 0000000000000003 R11: ffff95dd3fbfefe8 R12: 000000000002d400
> [  257.792376] R13: ffff95dcca22cc40 R14: 0000000000000000 R15: ffff95dcca22cc40
> [  257.792403] FS:  0000000000000000(0000) GS:ffff95dd3ab80000(0000) knlGS:0000000000000000
> [  257.792431] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  257.792456] CR2: 000055cdc7a88670 CR3: 0000000155c0a000 CR4: 00000000001006e0
> [  257.792485] Call Trace:
> [  257.792511]  <TASK>
> [  257.792535]  ? _raw_spin_lock_irqsave+0x23/0x50
> [  257.792579]  ? preempt_count_add+0x62/0xa0
> [  257.792625]  __schedule+0xac/0x9d0
> [  257.792663]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> [  257.792700]  ? __mod_timer+0x28a/0x3c0
> [  257.792743]  schedule+0x5d/0xe0
> [  257.792775]  schedule_timeout+0x8a/0x160
> [  257.792812]  ? __bpf_trace_tick_stop+0x10/0x10
> [  257.792849]  msleep+0x29/0x40
> [  257.792885]  acpi_lpss_resume+0x6e/0x160
> [  257.792924]  ? acpi_lpss_resume+0x160/0x160
> [  257.792958]  acpi_lpss_runtime_resume+0xe/0x20
> [  257.792993]  __rpm_callback+0x44/0x170
> [  257.793029]  ? acpi_lpss_resume+0x160/0x160
> [  257.793065]  rpm_callback+0x59/0x70
> [  257.793100]  ? acpi_lpss_resume+0x160/0x160
> [  257.793135]  rpm_resume+0x5f8/0x850
> [  257.793168]  ? acpi_ut_update_ref_count.part.0+0x7a/0x8f0
> [  257.793215]  __pm_runtime_resume+0x3c/0x60
> [  257.793251]  i2c_dw_xfer+0x47/0x490
> [  257.793291]  __i2c_transfer+0x16e/0x6c0
> [  257.793335]  i2c_smbus_xfer_emulated+0x26f/0x9c0
> [  257.793375]  ? __switch_to+0x7e/0x420
> [  257.793410]  ? update_load_avg+0x80/0x790
> [  257.793447]  __i2c_smbus_xfer+0xab/0x410
> [  257.793477]  ? enqueue_entity+0x19a/0x4f0
> [  257.793514]  i2c_smbus_xfer+0x6a/0xe0
> [  257.793547]  i2c_smbus_read_byte_data+0x45/0x70
> [  257.793578]  ? _raw_spin_unlock_irqrestore+0x23/0x40
> [  257.793615]  ? try_to_wake_up+0x95/0x590
> [  257.793653]  cht_wc_byte_reg_read+0x2e/0x60
> [  257.793701]  _regmap_read+0x5a/0x110
> [  257.793737]  _regmap_update_bits+0xb4/0xf0
> [  257.793775]  regmap_update_bits_base+0x59/0x80
> [  257.793816]  cht_wc_leds_set_effect+0xcb/0x1b0 [leds_cht_wcove]
> [  257.793875]  led_blink_setup+0x25/0x100
> [  257.793912]  led_trigger_blink+0x45/0x70
> [  257.793947]  power_supply_update_leds+0x1d6/0x1e0
> [  257.793991]  power_supply_changed_work+0x6f/0xf0
> [  257.794029]  process_one_work+0x1c7/0x3c0
> [  257.794065]  worker_thread+0x4e/0x3a0
> [  257.794095]  ? _raw_spin_lock_irqsave+0x23/0x50
> [  257.794133]  ? rescuer_thread+0x3b0/0x3b0
> [  257.794161]  kthread+0xe9/0x110
> [  257.794195]  ? kthread_complete_and_exit+0x20/0x20
> [  257.794233]  ret_from_fork+0x1f/0x30
> [  257.794280]  </TASK>
> [  257.794298] ---[ end trace 0000000000000000 ]---
> [  257.854563] bq25890-charger i2c-bq25892: S:CHG/PG/VSYS=2/1/0, F:CHG/BOOST/BAT/NTC=0/0/0/0
> 

Re: [PATCH 3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

[Jacek Anaszewski]

Hi Hans,

Thanks for the patch.

On 4/13/23 17:18, Hans de Goede wrote:
> The hw-blinking of the LED controller in the Whiskey Cove PMIC can also
> be used for a hw-breathing effect.
> 
> As discussed during review of v2 of the submission of the new
> leds-cht-wcove driver, the LED subsystem already supports breathing mode
> on several other LED controllers using the hw_pattern interface.
> 
> Implement a pattern_set callback to implement breathing mode modelled
> after the breathing mode supported by the SC27xx breathing light and
> Crane EL15203000 LED drivers. The Whiskey Cove PMIC's breathing mode
> is closer to the EL15203000 one then to the SC27xx one since it does
> not support staying high / low for a specific time, it only supports
> rise and fall times.
> 
> As such the supported hw_pattern and the documentation for this is almost
> a 1:1 copy of the pattern/docs for the EL15203000 breathing mode.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   Documentation/leds/index.rst          |  1 +
>   Documentation/leds/leds-cht-wcove.rst | 29 ++++++++++++++++++
>   drivers/leds/leds-cht-wcove.c         | 42 ++++++++++++++++++++++++---
>   3 files changed, 68 insertions(+), 4 deletions(-)
>   create mode 100644 Documentation/leds/leds-cht-wcove.rst
> 
> diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
> index b9ca081fac71..c92612271e25 100644
> --- a/Documentation/leds/index.rst
> +++ b/Documentation/leds/index.rst
> @@ -17,6 +17,7 @@ LEDs
>      uleds
>   
>      leds-blinkm
> +   leds-cht-wcove
>      leds-el15203000
>      leds-lm3556
>      leds-lp3944
> diff --git a/Documentation/leds/leds-cht-wcove.rst b/Documentation/leds/leds-cht-wcove.rst
> new file mode 100644
> index 000000000000..fa79d8fd7ef8
> --- /dev/null
> +++ b/Documentation/leds/leds-cht-wcove.rst
> @@ -0,0 +1,29 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===========================================================
> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
> +===========================================================
> +
> +/sys/class/leds/<led>/hw_pattern
> +--------------------------------
> +
> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
> +
> +The only supported pattern is hardware breathing mode::
> +
> +    "0 2000 1 2000"

Why 1? What is the peek brightness in this mode?

> +
> +	^
> +	|
> +    Max-|     ---
> +	|    /   \
> +	|   /     \
> +	|  /       \     /
> +	| /         \   /
> +    Min-|-           ---
> +	|
> +	0------2------4--> time (sec)
v);

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

[Hans de Goede]

Hi Jacek,

On 4/16/23 17:17, Jacek Anaszewski wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On 4/13/23 17:18, Hans de Goede wrote:
>> The hw-blinking of the LED controller in the Whiskey Cove PMIC can also
>> be used for a hw-breathing effect.
>>
>> As discussed during review of v2 of the submission of the new
>> leds-cht-wcove driver, the LED subsystem already supports breathing mode
>> on several other LED controllers using the hw_pattern interface.
>>
>> Implement a pattern_set callback to implement breathing mode modelled
>> after the breathing mode supported by the SC27xx breathing light and
>> Crane EL15203000 LED drivers. The Whiskey Cove PMIC's breathing mode
>> is closer to the EL15203000 one then to the SC27xx one since it does
>> not support staying high / low for a specific time, it only supports
>> rise and fall times.
>>
>> As such the supported hw_pattern and the documentation for this is almost
>> a 1:1 copy of the pattern/docs for the EL15203000 breathing mode.
>>
>> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   Documentation/leds/index.rst          |  1 +
>>   Documentation/leds/leds-cht-wcove.rst | 29 ++++++++++++++++++
>>   drivers/leds/leds-cht-wcove.c         | 42 ++++++++++++++++++++++++---
>>   3 files changed, 68 insertions(+), 4 deletions(-)
>>   create mode 100644 Documentation/leds/leds-cht-wcove.rst
>>
>> diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
>> index b9ca081fac71..c92612271e25 100644
>> --- a/Documentation/leds/index.rst
>> +++ b/Documentation/leds/index.rst
>> @@ -17,6 +17,7 @@ LEDs
>>      uleds
>>        leds-blinkm
>> +   leds-cht-wcove
>>      leds-el15203000
>>      leds-lm3556
>>      leds-lp3944
>> diff --git a/Documentation/leds/leds-cht-wcove.rst b/Documentation/leds/leds-cht-wcove.rst
>> new file mode 100644
>> index 000000000000..fa79d8fd7ef8
>> --- /dev/null
>> +++ b/Documentation/leds/leds-cht-wcove.rst
>> @@ -0,0 +1,29 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +===========================================================
>> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
>> +===========================================================
>> +
>> +/sys/class/leds/<led>/hw_pattern
>> +--------------------------------
>> +
>> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
>> +
>> +The only supported pattern is hardware breathing mode::
>> +
>> +    "0 2000 1 2000"
> 
> Why 1? What is the peek brightness in this mode?

255, but the pattern only controls the timing. For max brightness
the last set brightness is used and the max brightness can be changed
while breathing by writing the brightness attribute.

This is just like how blinking works in the LED subsystem,
for both sw and hw blinking the brightness can also be changed
while blinking. Breathing on this hw really is just a variant
mode of blinking.

Regards,

Hans





> 
>> +
>> +    ^
>> +    |
>> +    Max-|     ---
>> +    |    /   \
>> +    |   /     \
>> +    |  /       \     /
>> +    | /         \   /
>> +    Min-|-           ---
>> +    |
>> +    0------2------4--> time (sec)
> v);
> 

Re: [PATCH 3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

[Jacek Anaszewski]

Hi Hans,

On 4/16/23 22:31, Hans de Goede wrote:
> Hi Jacek,
> 
> On 4/16/23 17:17, Jacek Anaszewski wrote:
>> Hi Hans,
>>
[...]
>>> +===========================================================
>>> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
>>> +===========================================================
>>> +
>>> +/sys/class/leds/<led>/hw_pattern
>>> +--------------------------------
>>> +
>>> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
>>> +
>>> +The only supported pattern is hardware breathing mode::
>>> +
>>> +    "0 2000 1 2000"
>>
>> Why 1? What is the peek brightness in this mode?
> 
> 255, but the pattern only controls the timing. For max brightness
> the last set brightness is used and the max brightness can be changed
> while breathing by writing the brightness attribute.
> 
> This is just like how blinking works in the LED subsystem,
> for both sw and hw blinking the brightness can also be changed
> while blinking. Breathing on this hw really is just a variant
> mode of blinking.

Thanks for the explanation. So it would be nice to have it
also mentioned explicitly here.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

Hi Pavel,

Thank you for reviewing this.

On 4/14/23 14:44, Pavel Machek wrote:
> On Thu 2023-04-13 17:18:04, Hans de Goede wrote:
>> From: Yauhen Kharuzhy <jekhor@gmail.com>
>>
>> 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.
> 
> leds->LEDs.
> 
>> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
>> new file mode 100644
>> index 000000000000..06447804d050
>> --- /dev/null
>> +++ b/drivers/leds/leds-cht-wcove.c
>> @@ -0,0 +1,368 @@
>> +// 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>
>> + * Copyright 2023 Hans de Goede <hansg@kernel.org>
>> + *
>> + * Based on Lenovo Yoga Book Android kernel sources
>> + */
> 
> "sources." Should copyrights from Android be copied here?
>
>> +static const char * const cht_wc_leds_names[CHT_WC_LED_COUNT] = {
>> +	"cht-wc::" LED_FUNCTION_CHARGING,
>> +	"cht-wc::" LED_FUNCTION_INDICATOR,
>> +};
> 
> No need for "cht-wc".

I should put something there right, iow not just
"::" LED_FUNCTION_CHARGING" if it should not be
cht-wc should I put "platform::" there then, or
maybe "pmic::" ?

> Mention color.

This PMIC is used in multiple designs at
least both white and red LEDs are used in
2 designs I know about. AFAIK the norm is
to leave out the color if it is different
per design? 


> At least charging LED is
> going to be common - document in Documentation/leds/well-known-leds.txt. 

That already has:

* Power management

Good: "platform:*:charging" (allwinner sun50i)

So you want me to extend the (allwinner sun50i) bit with ", cht-wc" ?

Ack to all the other comments, will fix for v2.

Regards,

Hans



> 
>> +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->regmap, led->regs->ctrl,
>> +					 led->regs->on_off_mask, led->regs->off_val);
>> +		if (ret)
>> +			dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
>> +
>> +		/* Disable HW blinking */
>> +		ret = regmap_update_bits(led->regmap, led->regs->fsm,
>> +					 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);
> 
> This overwrites previous error. Not sure if it is big deal.
> 
>> +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;
> 
> That's really slow blink; I'd do something faster by default.
> 
>> +	/*
>> +	 * LED1 might be in hw-controlled mode when this driver gets loaded; and
>> +	 * since the PMIC is always powered by the battery any changes made are
>> +	 * permanent. Save LED1 regs to restore them on remove() or shutdown().
>> +	 */
> 
> Fun :-).
> 
> Thank you.
> 
> Best regards,
> 								Pavel

Re: [PATCH 3/5] leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs API

[Hans de Goede]

Hi,

On 4/17/23 22:00, Jacek Anaszewski wrote:
> Hi Hans,
> 
> On 4/16/23 22:31, Hans de Goede wrote:
>> Hi Jacek,
>>
>> On 4/16/23 17:17, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
> [...]
>>>> +===========================================================
>>>> +Kernel driver for Intel Cherry Trail Whiskey Cove PMIC LEDs
>>>> +===========================================================
>>>> +
>>>> +/sys/class/leds/<led>/hw_pattern
>>>> +--------------------------------
>>>> +
>>>> +Specify a hardware pattern for the Whiskey Cove PMIC LEDs.
>>>> +
>>>> +The only supported pattern is hardware breathing mode::
>>>> +
>>>> +    "0 2000 1 2000"
>>>
>>> Why 1? What is the peek brightness in this mode?
>>
>> 255, but the pattern only controls the timing. For max brightness
>> the last set brightness is used and the max brightness can be changed
>> while breathing by writing the brightness attribute.
>>
>> This is just like how blinking works in the LED subsystem,
>> for both sw and hw blinking the brightness can also be changed
>> while blinking. Breathing on this hw really is just a variant
>> mode of blinking.
> 
> Thanks for the explanation. So it would be nice to have it
> also mentioned explicitly here.

Ack, I'll add something like the above explanation to the
docs for v2.

Regards,

Hans

Re: [PATCH 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

Hi,

On 4/14/23 14:44, Pavel Machek wrote:
> On Thu 2023-04-13 17:18:04, Hans de Goede wrote:
>> From: Yauhen Kharuzhy <jekhor@gmail.com>
>>
>> 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.
> 
> leds->LEDs.
> 
>> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
>> new file mode 100644
>> index 000000000000..06447804d050
>> --- /dev/null
>> +++ b/drivers/leds/leds-cht-wcove.c
>> @@ -0,0 +1,368 @@
>> +// 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>
>> + * Copyright 2023 Hans de Goede <hansg@kernel.org>
>> + *
>> + * Based on Lenovo Yoga Book Android kernel sources
>> + */
> 
> "sources." Should copyrights from Android be copied here?

Quick follow up on this, the original file has no copyright statements,
so there is nothing to copy. Also it uses a custom sysfs API, so
this new LED driver was pretty much done from scratch. The only thing
which comes from the Android source is hw-interface info like
register addresses and what various bits in the registers mean.

For v2 I've updated this comment to:

 * Register info comes from the Lenovo Yoga Book Android kernel sources:
 * YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c.

Regards,

Hans

Re: [PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede]

Hi Yauhen,

On 4/16/23 15:04, Hans de Goede wrote:
> Hi Yauhen,
> 
> On 4/15/23 22:32, Yauhen Kharuzhy wrote:
>> On Thu, Apr 13, 2023 at 05:18:03PM +0200, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Here is a patch series to add support for the LED controller on
>>> Intel Cherry Trail Whiskey Cove PMICs.
>>>
>>> This is based on the original patch for this from Yauhen Kharuzhy,
>>> with additional work on top by me.
>>>
>>> This addresses the review remarks on the v2 posting from Yauhen:
>>> - Since the PMIC is connected to the battery any changes we make to
>>>   the LED settings are permanent, even surviving reboot / poweroff.
>>>   Save LED1 register settings on probe() and if auto-/hw-control was
>>>   enabled on probe() restore the settings on remove() and shutdown().
>>> - Add support for the pattern trigger to select breathing mode
>>>
>>> This makes the charging LED on devices with these PMICs properly
>>> reflect the charging status (this relies on sw control on most
>>> devices) and this also allows control of the LED behind the pen
>>> (digitizer on) symbol on the keyboard half of the Lenovo Yoga Book
>>> 1 models.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> Hans de Goede (4):
>>>   leds: cht-wcove: Add suspend/resume handling
>>>   leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs
>>>     API
>>>   leds: cht-wcove: Set default trigger for charging LED
>>>   leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set
>>>
>>> Yauhen Kharuzhy (1):
>>>   leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver
>>>
>>>  Documentation/leds/index.rst          |   1 +
>>>  Documentation/leds/leds-cht-wcove.rst |  29 ++
>>>  drivers/leds/Kconfig                  |  11 +
>>>  drivers/leds/Makefile                 |   1 +
>>>  drivers/leds/leds-cht-wcove.c         | 466 ++++++++++++++++++++++++++
>>>  5 files changed, 508 insertions(+)
>>>  create mode 100644 Documentation/leds/leds-cht-wcove.rst
>>>  create mode 100644 drivers/leds/leds-cht-wcove.c
>>
>> Hi Hans,
>>
>> Thanks for reviving this patch!
> 
> You're welcome.
> 
>> I haven't tested it on linux-next yet but on v6.2.11 (with few patches for
>> Yoabook) I catched following trace. I will investigate it later but maybe you
>> can take a look also?
> 
> Right, this is an unrelated pre-existing kernel bug when using
> led_trigger_blink().
> 
> I already hit that myself and I have a fix for it, see this series:
> https://lore.kernel.org/linux-leds/20230412215855.593541-1-hdegoede@redhat.com/

Lee is asking for testers of this series, if you have time it would be
good if you can give this series a try in combination with this new
leds-cht-wcove driver.

Assuming the other series fixed the oops/backtrace for you can you
please reply with your Tested-by to that series:

https://lore.kernel.org/linux-leds/20230412215855.593541-1-hdegoede@redhat.com/

(or even better give your Tested-by for both series)

Regards,

Hans

Re: [PATCH 0/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Yauhen Kharuzhy]

Hi,

OK, I will test the latest version near the weekend and will reply
with Tested-by if all will be fine.

вт, 25 апр. 2023 г. в 16:37, Hans de Goede <hdegoede@redhat.com>:
>
> Hi Yauhen,
>
> On 4/16/23 15:04, Hans de Goede wrote:
> > Hi Yauhen,
> >
> > On 4/15/23 22:32, Yauhen Kharuzhy wrote:
> >> On Thu, Apr 13, 2023 at 05:18:03PM +0200, Hans de Goede wrote:
> >>> Hi All,
> >>>
> >>> Here is a patch series to add support for the LED controller on
> >>> Intel Cherry Trail Whiskey Cove PMICs.
> >>>
> >>> This is based on the original patch for this from Yauhen Kharuzhy,
> >>> with additional work on top by me.
> >>>
> >>> This addresses the review remarks on the v2 posting from Yauhen:
> >>> - Since the PMIC is connected to the battery any changes we make to
> >>>   the LED settings are permanent, even surviving reboot / poweroff.
> >>>   Save LED1 register settings on probe() and if auto-/hw-control was
> >>>   enabled on probe() restore the settings on remove() and shutdown().
> >>> - Add support for the pattern trigger to select breathing mode
> >>>
> >>> This makes the charging LED on devices with these PMICs properly
> >>> reflect the charging status (this relies on sw control on most
> >>> devices) and this also allows control of the LED behind the pen
> >>> (digitizer on) symbol on the keyboard half of the Lenovo Yoga Book
> >>> 1 models.
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>
> >>> Hans de Goede (4):
> >>>   leds: cht-wcove: Add suspend/resume handling
> >>>   leds: cht-wcove: Add support for breathing mode use hw_pattern sysfs
> >>>     API
> >>>   leds: cht-wcove: Set default trigger for charging LED
> >>>   leds: cht-wcove: Use breathing when LED_INIT_DEFAULT_TRIGGER is set
> >>>
> >>> Yauhen Kharuzhy (1):
> >>>   leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver
> >>>
> >>>  Documentation/leds/index.rst          |   1 +
> >>>  Documentation/leds/leds-cht-wcove.rst |  29 ++
> >>>  drivers/leds/Kconfig                  |  11 +
> >>>  drivers/leds/Makefile                 |   1 +
> >>>  drivers/leds/leds-cht-wcove.c         | 466 ++++++++++++++++++++++++++
> >>>  5 files changed, 508 insertions(+)
> >>>  create mode 100644 Documentation/leds/leds-cht-wcove.rst
> >>>  create mode 100644 drivers/leds/leds-cht-wcove.c
> >>
> >> Hi Hans,
> >>
> >> Thanks for reviving this patch!
> >
> > You're welcome.
> >
> >> I haven't tested it on linux-next yet but on v6.2.11 (with few patches for
> >> Yoabook) I catched following trace. I will investigate it later but maybe you
> >> can take a look also?
> >
> > Right, this is an unrelated pre-existing kernel bug when using
> > led_trigger_blink().
> >
> > I already hit that myself and I have a fix for it, see this series:
> > https://lore.kernel.org/linux-leds/20230412215855.593541-1-hdegoede@redhat.com/
>
> Lee is asking for testers of this series, if you have time it would be
> good if you can give this series a try in combination with this new
> leds-cht-wcove driver.
>
> Assuming the other series fixed the oops/backtrace for you can you
> please reply with your Tested-by to that series:
>
> https://lore.kernel.org/linux-leds/20230412215855.593541-1-hdegoede@redhat.com/
>
> (or even better give your Tested-by for both series)
>
> Regards,
>
> Hans
>


-- 
Yauhen Kharuzhy