Skip to contentSkip to conversation

Intel Cherry Trail Whiskey Cove LEDs support

PATCH Superseded Yauhen Kharuzhy 5 participants 12 messages Feb 9, 2019 – Feb 12, 2019

Subsystems (3)
Multifunction devices (MFD) LED Intel PMIC multifunction device drivers

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Yauhen Kharuzhy

author
#0 lore raw

cover Intel Cherry Trail Whiskey Cove LEDs support

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.

author
#1 lore raw

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

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

This driver was tested with Lenovo Yoga Book notebook.

Signed-off-by Yauhen Kharuzhy <[email protected]>
---
 drivers/leds/Kconfig          |  11 ++
 drivers/leds/Makefile         |   1 +
 drivers/leds/leds-cht-wcove.c | 278 ++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+)
 create mode 100644 drivers/leds/leds-cht-wcove.c
drivers/leds/Kconfig +11 -0
index a72f97fca57b..8f50f38af57e 100644
@@ -106,6 +106,17 @@ config LEDS_BCM6358
 	  This option enables support for LEDs connected to the BCM6358
 	  LED HW controller accessed via MMIO registers.
 
+config LEDS_CHT_WCOVE
+	tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC"
+	depends on LEDS_CLASS
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This option enables support for charger and general purpose LEDs
+	  connected to the Intel Cherrytrail Whiskey Cove PMIC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-cht-wcove.
+
 config LEDS_CPCAP
 	tristate "LED Support for Motorola CPCAP"
 	depends on LEDS_CLASS
drivers/leds/Makefile +1 -0
index 4c1b0054f379..1c1995d3441c 100644
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
+obj-$(CONFIG_LEDS_CHT_WCOVE)		+= leds-cht-wcove.o
 obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
 obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
 obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
drivers/leds/leds-cht-wcove.c new file +278 -0

278 lines not on this page · raw message

Signature · 1 line
2.20.1
#2 lore raw

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

Hi!

Quoted 6 lines
drivers/leds/leds-cht-wcove.c new file +6 -0
+static struct cht_wc_led cht_wc_leds[] = {
+	{
+		.name = "pmic::charge",
+		.ctrl_reg = CHT_WC_LED1_CTRL,
+		.fsm_reg = CHT_WC_LED1_FSM,
+		.pwm_reg = CHT_WC_LED1_PWM,

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

Quoted 10 lines
drivers/leds/leds-cht-wcove.c new file +10 -0
+		.enable_mask = CHT_WC_LED1_ON,
+	},
+	{
+		.name = "pmic::gpled",
+		.ctrl_reg = CHT_WC_LED2_CTRL,
+		.fsm_reg = CHT_WC_LED2_FSM,
+		.pwm_reg = CHT_WC_LED2_PWM,
+		.enable_mask = CHT_WC_LED2_ON,
+	},
+};

Unfortunately, these LED names will not be too useful for userspace. What about "platform::charging" and "status-led::"?

Should we have some default triggers?
Pavel

-*- org -*-

It is somehow important to provide consistent interface to the userland. LED devices have one problem there, and that is naming of directories in /sys/class/leds. It would be nice if userland would just know right "name" for given LED function, but situation got more complex.

Anyway, if backwards compatibility is not an issue, new code should use one of the "good" names from this list, and you should extend the list where applicable.

Bad names are listed, too, in case you are writing application that wants to use particular feature, you should probe for good name first but then try the bad ones, too".

* Keyboards

Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)

Set of common keyboard LEDs, going back to PC AT or so.

Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

Frontlight/backlight of main keyboard.

Bad: "button-backlight" (Motorola Droid 4)

Some phones have touch buttons below screen; it is different from main keyboard. And this is their backlight.

* Sound subsystem

Good: "platform:*:mute"
Good: "platform:*:micmute"

LEDs on notebook body, indicating that sound input / output is muted.

* System notification

Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)

Phones usually have multi-color status LED.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#3 lore raw

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

Quoted 13 lines · Pavel Machek

Hi!

Quoted 11 lines
drivers/leds/leds-cht-wcove.c new file +11 -0
+static struct cht_wc_led cht_wc_leds[] = {
+	{
+		.name = "pmic::charge",
+		.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 = "pmic::gpled",
+		.ctrl_reg = CHT_WC_LED2_CTRL,
Quoted 8 lines

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

Quoted 5 lines
drivers/leds/leds-cht-wcove.c new file +5 -0
+		.fsm_reg = CHT_WC_LED2_FSM,
+		.pwm_reg = CHT_WC_LED2_PWM,
+		.enable_mask = CHT_WC_LED2_ON,
+	},
+};

Unfortunately, these LED names will not be too useful for userspace. What about "platform::charging" and "status-led::"?

That sounds reasonable.

Quoted 2 lines

Should we have some default triggers?

I don't know which triggers may be suitable for this. There are no generic "is charging now" triggers in kernel. I can set default trigger of charging led to 'bq27542-0-charging-blink-full-solid' as example but it will valid for platforms with bq27542 battery fuel gauge, of course.

For status led, 'default-on' should be reasonable.

Quoted 1 line

Pavel

Signature · 1 line
Yauhen Kharuzhy
#4 lore raw

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

In reply to Yauhen Kharuzhy

Hi Yauhen,

Thank you for the patch.

Please find few comments in the code.

Quoted 54 lines · Yauhen Kharuzhy

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

This driver was tested with Lenovo Yoga Book notebook.

Signed-off-by Yauhen Kharuzhy <[email protected]>
---
  drivers/leds/Kconfig          |  11 ++
  drivers/leds/Makefile         |   1 +
  drivers/leds/leds-cht-wcove.c | 278 ++++++++++++++++++++++++++++++++++
  3 files changed, 290 insertions(+)

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

drivers/leds/Kconfig +11 -0
index a72f97fca57b..8f50f38af57e 100644
@@ -106,6 +106,17 @@ config LEDS_BCM6358
  	  This option enables support for LEDs connected to the BCM6358
  	  LED HW controller accessed via MMIO registers.
 
+config LEDS_CHT_WCOVE
+	tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC"
+	depends on LEDS_CLASS
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This option enables support for charger and general purpose LEDs
+	  connected to the Intel Cherrytrail Whiskey Cove PMIC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-cht-wcove.
+
  config LEDS_CPCAP
  	tristate "LED Support for Motorola CPCAP"
  	depends on LEDS_CLASS
drivers/leds/Makefile +1 -0
index 4c1b0054f379..1c1995d3441c 100644
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
+obj-$(CONFIG_LEDS_CHT_WCOVE)		+= leds-cht-wcove.o
  obj-$(CONFIG_LEDS_CPCAP)		+= leds-cpcap.o
  obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
  obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
drivers/leds/leds-cht-wcove.c new file +0 -0
new file mode 100644
index 000000000000..82ed0845bf72

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

Quoted 8 lines
drivers/leds/leds-cht-wcove.c new file +8 -0
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC
+ *
+ * Copyright 2019 Yauhen Kharuzhy <[email protected]>
+ *
+ * Based on Lenovo Yoga Book Android kernel sources
+ */

Please use uniform "//" comment style above.

Quoted 58 lines
drivers/leds/leds-cht-wcove.c new file +58 -0
+#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;
+	u16	ctrl_reg;
+	u8	enable_mask;
+	u16	fsm_reg;
+	u16	pwm_reg;
+};
+
+static struct cht_wc_led cht_wc_leds[] = {
+	{
+		.name = "pmic::charge",
+		.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 = "pmic::gpled",
+		.ctrl_reg = CHT_WC_LED2_CTRL,
+		.fsm_reg = CHT_WC_LED2_FSM,

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

Quoted 10 lines
drivers/leds/leds-cht-wcove.c new file +10 -0
+		.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;

You need mutex protection here. Think of the cases where brightness is set from sysfs and the call from trigger jumps in the middle.

Quoted 21 lines
drivers/leds/leds-cht-wcove.c new file +21 -0
+	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)

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

Quoted 10 lines
drivers/leds/leds-cht-wcove.c new file +10 -0
+			dev_err(cdev->dev, "Failed to turn on: %d\n", ret);
+	}
+	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;

Ditto.

Quoted 58 lines
drivers/leds/leds-cht-wcove.c new file +58 -0
+	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);
+		return LED_OFF;
+	}
+
+	val &= led->enable_mask;
+
+	if (!val)
+		return LED_OFF;
+
+	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);
+		return LED_ON;
+	}
+
+	return val;
+}
+
+/* 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

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

Quoted 10 lines
drivers/leds/leds-cht-wcove.c new file +10 -0
+		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 needed as well.

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

Quoted 3 lines
drivers/leds/leds-cht-wcove.c new file +3 -0
+	if (!*delay_on && !*delay_off) {
+		/* Return current settings */
+		ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &ctrl);

Why read? This op sets blinking. Please refer to the blink_set semantics description from include/linux/leds.h:

/*
* Activate hardware accelerated blink, delays are in milliseconds
* and if both are zero then a sensible default should be chosen.
* The call should adjust the timings in that case and if it can't
* match the values specified exactly.
* Deactivate blinking again when the brightness is set to LED_OFF
* via the brightness_set() callback.
*/
int (*blink_set)(struct led_classdev *led_cdev,
unsigned long *delay_on, unsigned long *delay_off);

Please compare how other LED class drivers do that.

Quoted 86 lines
drivers/leds/leds-cht-wcove.c new file +86 -0
+
+		if (ret < 0) {
+			dev_err(cdev->dev,
+				"Failed to read LED CTRL reg: %d\n", ret);
+			return ret;
+		}
+
+		*delay_off = *delay_on = cht_wc_leds_get_period(ctrl) / 2;
+
+		return 0;
+	}
+
+	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;
+		return -EINVAL;
+	}
+
+	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;
+
+	return 0;
+}
+
+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;
+		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;
+
+		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,

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

Quoted 10 lines
drivers/leds/leds-cht-wcove.c new file +10 -0
+	.id_table = cht_wc_leds_table,
+	.driver = {
+		.name = "cht_wcove_leds",
+	},
+};
+module_platform_driver(cht_wc_leds_driver);
+
+MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC LEDs driver");
+MODULE_AUTHOR("Yauhen Kharuzhy <[email protected]>");
+MODULE_LICENSE("GPL");

s/GPL/GPL v2/

Quoted 2 lines
drivers/leds/leds-cht-wcove.c new file +1 -0
+
Signature · 2 lines
Best regards,
Jacek Anaszewski
author
#5 lore raw

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

Quoted 7 lines · Jacek Anaszewski

Hi Yauhen,

Thank you for the patch.

Please find few comments in the code.

On 2/9/19 11:12 PM, Yauhen Kharuzhy wrote:

Quoted 16 lines
Quoted 6 lines
drivers/leds/leds-cht-wcove.c new file +0 -0
new file mode 100644
index 000000000000..82ed0845bf72

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

Quoted 8 lines
drivers/leds/leds-cht-wcove.c new file +8 -0
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC
+ *
+ * Copyright 2019 Yauhen Kharuzhy <[email protected]>
+ *
+ * Based on Lenovo Yoga Book Android kernel sources
+ */

Please use uniform "//" comment style above.

OK.

Quoted 9 lines

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

Quoted 6 lines
drivers/leds/leds-cht-wcove.c new file +6 -0
+
+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;

You need mutex protection here. Think of the cases where brightness is set from sysfs and the call from trigger jumps in the middle.

Agree.

Quoted 7 lines

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

Quoted 5 lines
drivers/leds/leds-cht-wcove.c new file +5 -0
+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;

Ditto.

OK

Quoted 9 lines

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

Quoted 7 lines
drivers/leds/leds-cht-wcove.c new file +7 -0
+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 needed as well.

OK

Quoted 20 lines

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

Quoted 4 lines
drivers/leds/leds-cht-wcove.c new file +3 -0
+	if (!*delay_on && !*delay_off) {
+		/* Return current settings */
+		ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &ctrl);

Why read? This op sets blinking. Please refer to the blink_set semantics description from include/linux/leds.h:

/*
* Activate hardware accelerated blink, delays are in milliseconds
* and if both are zero then a sensible default should be chosen.
* The call should adjust the timings in that case and if it can't
* match the values specified exactly.
* Deactivate blinking again when the brightness is set to LED_OFF
* via the brightness_set() callback.
*/
int (*blink_set)(struct led_classdev *led_cdev,
unsigned long *delay_on, unsigned long *delay_off);

Quoted 1 line

Please compare how other LED class drivers do that.

Hmm... This was attempt to set 'reasonable default' to current blink settings from hardware. But if this is too unpredictable behaviour for LED class, I will change it to constant values.

Quoted 5 lines

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

Quoted 3 lines
drivers/leds/leds-cht-wcove.c new file +3 -0
+MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC LEDs driver");
+MODULE_AUTHOR("Yauhen Kharuzhy <[email protected]>");
+MODULE_LICENSE("GPL");

s/GPL/GPL v2/

OK.

Signature · 1 line
Yauhen Kharuzhy
author
#6 lore raw

PATCH 2/2 mfd: Add leds MFD cell for intel_soc_pmic_chtwc

In reply to Yauhen Kharuzhy

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

Signed-off-by Yauhen Kharuzhy <[email protected]>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mfd/intel_soc_pmic_chtwc.c +2 -0
index 64a3aece9c5e..68b54173edd8 100644
@@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
 		.resources = cht_wc_ext_charger_resources,
 	},
 	{	.name = "cht_wcove_region", },
+
+	{	.name = "cht_wcove_leds", },
 };
 
 /*
Signature · 1 line
2.20.1
#7 lore raw

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

Quoted 13 lines · Yauhen Kharuzhy

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

Signed-off-by Yauhen Kharuzhy <[email protected]>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mfd/intel_soc_pmic_chtwc.c +0 -0
index 64a3aece9c5e..68b54173edd8 100644

drivers/mfd/intel_soc_pmic_chtwc.c static struct mfd_cell cht_wc_dev[] = { · line 63

Quoted 4 lines
drivers/mfd/intel_soc_pmic_chtwc.c +1 -0
@@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
 		.resources = cht_wc_ext_charger_resources,
 	},
 	{	.name = "cht_wcove_region", },
+

Nit: No need for the blank line, please remove it.

Quoted 4 lines
drivers/mfd/intel_soc_pmic_chtwc.c +1 -0
+	{	.name = "cht_wcove_leds", },
 };
 
 /*
Signature · 4 lines
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#8 lore raw

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

Quoted 10 lines · Lee Jones
Quoted 8 lines · Yauhen Kharuzhy

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

Signed-off-by Yauhen Kharuzhy <[email protected]>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 11 lines

drivers/mfd/intel_soc_pmic_chtwc.c static struct mfd_cell cht_wc_dev[] = { · line 63

Quoted 9 lines
drivers/mfd/intel_soc_pmic_chtwc.c +1 -0
index 64a3aece9c5e..68b54173edd8 100644
@@ -60,6 +60,8 @@ static struct mfd_cell cht_wc_dev[] = {
 		.resources = cht_wc_ext_charger_resources,
 	},
 	{	.name = "cht_wcove_region", },
+

Nit: No need for the blank line, please remove it.

Agree.

Quoted 3 lines
Quoted 3 lines
drivers/mfd/intel_soc_pmic_chtwc.c +1 -0
+	{	.name = "cht_wcove_leds", },
 };
Signature · 1 line
Yauhen Kharuzhy
#9 lore raw
In reply to Yauhen Kharuzhy
Quoted 8 lines · 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.

I'm not sure you have a correct Cc list here. LED maintainers are

M:      Jacek Anaszewski <[email protected]>
M:      Pavel Machek <[email protected]>

Also would be nice to cc to Hans de Goede.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · 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.

Quoted 5 lines

I'm not sure you have a correct Cc list here. LED maintainers are

M:      Jacek Anaszewski <[email protected]>
M:      Pavel Machek <[email protected]>

Also would be nice to cc to Hans de Goede.

I follow the list. I beileve Jacek does, too.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#11 lore raw
Quoted 11 lines · Pavel Machek
Quoted 10 lines · Andy Shevchenko
Quoted 8 lines · 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.

Quoted 7 lines
Quoted 5 lines

I'm not sure you have a correct Cc list here. LED maintainers are

M:      Jacek Anaszewski <[email protected]>
M:      Pavel Machek <[email protected]>

Also would be nice to cc to Hans de Goede.

I follow the list. I beileve Jacek does, too.

Right, there is no issue with that.

Signature · 2 lines
Best regards,
Jacek Anaszewski