* [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support
@ 2019-02-09 22:12 Yauhen Kharuzhy
2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Yauhen Kharuzhy @ 2019-02-09 22:12 UTC (permalink / raw)
To: linux-kernel, linux-leds; +Cc: Andy Shevchenko, Lee Jones
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs 2019-02-09 22:12 [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Yauhen Kharuzhy @ 2019-02-09 22:12 ` Yauhen Kharuzhy 2019-02-11 13:17 ` Pavel Machek 2019-02-11 21:36 ` Jacek Anaszewski 2019-02-09 22:12 ` [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc Yauhen Kharuzhy 2019-02-11 11:59 ` [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Andy Shevchenko 2 siblings, 2 replies; 12+ messages in thread From: Yauhen Kharuzhy @ 2019-02-09 22:12 UTC (permalink / raw) To: linux-kernel, linux-leds; +Cc: Andy Shevchenko, Lee Jones, Yauhen Kharuzhy Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC. Charger and general-purpose leds are supported. Hardware blinking is implemented, breathing is not. This driver was tested with Lenovo Yoga Book notebook. Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com> --- drivers/leds/Kconfig | 11 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-cht-wcove.c | 278 ++++++++++++++++++++++++++++++++++ 3 files changed, 290 insertions(+) create mode 100644 drivers/leds/leds-cht-wcove.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index a72f97fca57b..8f50f38af57e 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -106,6 +106,17 @@ config LEDS_BCM6358 This option enables support for LEDs connected to the BCM6358 LED HW controller accessed via MMIO registers. +config LEDS_CHT_WCOVE + tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC" + depends on LEDS_CLASS + depends on INTEL_SOC_PMIC_CHTWC + help + This option enables support for charger and general purpose LEDs + connected to the Intel Cherrytrail Whiskey Cove PMIC. + + To compile this driver as a module, choose M here: the module + will be called leds-cht-wcove. + config LEDS_CPCAP tristate "LED Support for Motorola CPCAP" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 4c1b0054f379..1c1995d3441c 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o +obj-$(CONFIG_LEDS_CHT_WCOVE) += leds-cht-wcove.o obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c new file mode 100644 index 000000000000..82ed0845bf72 --- /dev/null +++ b/drivers/leds/leds-cht-wcove.c @@ -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 <jekhor@gmail.com> + * + * Based on Lenovo Yoga Book Android kernel sources + */ +#include <linux/kernel.h> +#include <linux/mfd/intel_soc_pmic.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define CHT_WC_LED1_CTRL 0x5e1f +#define CHT_WC_LED1_FSM 0x5e20 +#define CHT_WC_LED1_PWM 0x5e21 + +#define CHT_WC_LED2_CTRL 0x4fdf +#define CHT_WC_LED2_FSM 0x4fe0 +#define CHT_WC_LED2_PWM 0x4fe1 + +/* HW or SW control of charging led */ +#define CHT_WC_LED1_SWCTL BIT(0) +#define CHT_WC_LED1_ON BIT(1) + +#define CHT_WC_LED2_ON BIT(0) +#define CHT_WC_LED_I_MA2_5 (2 << 2) +/* LED current limit */ +#define CHT_WC_LED_I_MASK GENMASK(3, 2) + +#define CHT_WC_LED_F_1_4_HZ (0 << 4) +#define CHT_WC_LED_F_1_2_HZ (1 << 4) +#define CHT_WC_LED_F_1_HZ (2 << 4) +#define CHT_WC_LED_F_2_HZ (3 << 4) +#define CHT_WC_LED_F_MASK 0x30 + +#define CHT_WC_LED_EFF_ON BIT(1) +#define CHT_WC_LED_EFF_BLINKING BIT(2) +#define CHT_WC_LED_EFF_BREATHING BIT(3) +#define CHT_WC_LED_EFF_MASK 0x06 + +struct cht_wc_led { + struct led_classdev cdev; + struct intel_soc_pmic *pmic; + const char *name; + 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, + .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; + + if (!value) { + ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg, + led->enable_mask, 0); + if (ret) + dev_err(cdev->dev, "Failed to turn off: %d\n", ret); + + ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg, + CHT_WC_LED_EFF_MASK, + CHT_WC_LED_EFF_ON); + if (ret < 0) + dev_err(cdev->dev, + "Failed to update LED FSM reg: %d\n", ret); + } else { + ret = regmap_write(led->pmic->regmap, led->pwm_reg, value); + if (ret) + dev_err(cdev->dev, + "Failed to set brightness: %d\n", ret); + + ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg, + led->enable_mask, led->enable_mask); + if (ret) + dev_err(cdev->dev, "Failed to turn on: %d\n", ret); + } + 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; + + 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 + 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; + + if (!*delay_on && !*delay_off) { + /* Return current settings */ + ret = regmap_read(led->pmic->regmap, led->ctrl_reg, &ctrl); + + 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, + .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 <jekhor@gmail.com>"); +MODULE_LICENSE("GPL"); + -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs 2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy @ 2019-02-11 13:17 ` Pavel Machek 2019-02-12 20:25 ` Yauhen Kharuzhy 2019-02-11 21:36 ` Jacek Anaszewski 1 sibling, 1 reply; 12+ messages in thread From: Pavel Machek @ 2019-02-11 13:17 UTC (permalink / raw) To: Yauhen Kharuzhy; +Cc: linux-kernel, linux-leds, Andy Shevchenko, Lee Jones [-- Attachment #1: Type: text/plain, Size: 2223 bytes --] Hi! > +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, > + .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. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs 2019-02-11 13:17 ` Pavel Machek @ 2019-02-12 20:25 ` Yauhen Kharuzhy 0 siblings, 0 replies; 12+ messages in thread From: Yauhen Kharuzhy @ 2019-02-12 20:25 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-kernel, linux-leds, Lee Jones On Mon, Feb 11, 2019 at 02:17:26PM +0100, Pavel Machek wrote: > Hi! > > > +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, > > + .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. > > 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. > Pavel -- Yauhen Kharuzhy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs 2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy 2019-02-11 13:17 ` Pavel Machek @ 2019-02-11 21:36 ` Jacek Anaszewski 2019-02-12 19:50 ` Yauhen Kharuzhy 1 sibling, 1 reply; 12+ messages in thread From: Jacek Anaszewski @ 2019-02-11 21:36 UTC (permalink / raw) To: Yauhen Kharuzhy, linux-kernel, linux-leds; +Cc: Andy Shevchenko, Lee Jones Hi Yauhen, Thank you for the patch. Please find few comments in the code. On 2/9/19 11:12 PM, Yauhen Kharuzhy wrote: > Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove > PMIC. Charger and general-purpose leds are supported. Hardware blinking > is implemented, breathing is not. > > This driver was tested with Lenovo Yoga Book notebook. > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com> > --- > 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 > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index a72f97fca57b..8f50f38af57e 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -106,6 +106,17 @@ config LEDS_BCM6358 > This option enables support for LEDs connected to the BCM6358 > LED HW controller accessed via MMIO registers. > > +config LEDS_CHT_WCOVE > + tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC" > + depends on LEDS_CLASS > + depends on INTEL_SOC_PMIC_CHTWC > + help > + This option enables support for charger and general purpose LEDs > + connected to the Intel Cherrytrail Whiskey Cove PMIC. > + > + To compile this driver as a module, choose M here: the module > + will be called leds-cht-wcove. > + > config LEDS_CPCAP > tristate "LED Support for Motorola CPCAP" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 4c1b0054f379..1c1995d3441c 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > +obj-$(CONFIG_LEDS_CHT_WCOVE) += leds-cht-wcove.o > obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o > obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c > new file mode 100644 > index 000000000000..82ed0845bf72 > --- /dev/null > +++ b/drivers/leds/leds-cht-wcove.c > @@ -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 <jekhor@gmail.com> > + * > + * Based on Lenovo Yoga Book Android kernel sources > + */ Please use uniform "//" comment style above. > +#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, > + .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. > + if (!value) { > + ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg, > + led->enable_mask, 0); > + if (ret) > + dev_err(cdev->dev, "Failed to turn off: %d\n", ret); > + > + ret = regmap_update_bits(led->pmic->regmap, led->fsm_reg, > + CHT_WC_LED_EFF_MASK, > + CHT_WC_LED_EFF_ON); > + if (ret < 0) > + dev_err(cdev->dev, > + "Failed to update LED FSM reg: %d\n", ret); > + } else { > + ret = regmap_write(led->pmic->regmap, led->pwm_reg, value); > + if (ret) > + dev_err(cdev->dev, > + "Failed to set brightness: %d\n", ret); > + > + ret = regmap_update_bits(led->pmic->regmap, led->ctrl_reg, > + led->enable_mask, led->enable_mask); > + if (ret) > + dev_err(cdev->dev, "Failed to turn on: %d\n", ret); > + } > + 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. > + 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 > + 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. > + 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. > + > + 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, > + .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 <jekhor@gmail.com>"); > +MODULE_LICENSE("GPL"); s/GPL/GPL v2/ > + > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs 2019-02-11 21:36 ` Jacek Anaszewski @ 2019-02-12 19:50 ` Yauhen Kharuzhy 0 siblings, 0 replies; 12+ messages in thread From: Yauhen Kharuzhy @ 2019-02-12 19:50 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: linux-kernel, linux-leds, Lee Jones On Mon, Feb 11, 2019 at 10:36:30PM +0100, Jacek Anaszewski wrote: > Hi Yauhen, > > Thank you for the patch. > > Please find few comments in the code. > > On 2/9/19 11:12 PM, Yauhen Kharuzhy wrote: > > diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c > > new file mode 100644 > > index 000000000000..82ed0845bf72 > > --- /dev/null > > +++ b/drivers/leds/leds-cht-wcove.c > > @@ -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 <jekhor@gmail.com> > > + * > > + * Based on Lenovo Yoga Book Android kernel sources > > + */ > > Please use uniform "//" comment style above. OK. > > + > > +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. > > +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 > > +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 > > > + 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. 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. > > +MODULE_DESCRIPTION("Intel Cherrytrail Whiskey Cove PMIC LEDs driver"); > > +MODULE_AUTHOR("Yauhen Kharuzhy <jekhor@gmail.com>"); > > +MODULE_LICENSE("GPL"); > > s/GPL/GPL v2/ OK. -- Yauhen Kharuzhy ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc 2019-02-09 22:12 [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Yauhen Kharuzhy 2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy @ 2019-02-09 22:12 ` Yauhen Kharuzhy 2019-02-12 8:14 ` Lee Jones 2019-02-11 11:59 ` [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Andy Shevchenko 2 siblings, 1 reply; 12+ messages in thread From: Yauhen Kharuzhy @ 2019-02-09 22:12 UTC (permalink / raw) To: linux-kernel, linux-leds; +Cc: Andy Shevchenko, Lee Jones, Yauhen Kharuzhy Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC mfd device driver. Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com> --- drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c index 64a3aece9c5e..68b54173edd8 100644 --- a/drivers/mfd/intel_soc_pmic_chtwc.c +++ b/drivers/mfd/intel_soc_pmic_chtwc.c @@ -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", }, }; /* -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc 2019-02-09 22:12 ` [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc Yauhen Kharuzhy @ 2019-02-12 8:14 ` Lee Jones 2019-02-12 19:52 ` Yauhen Kharuzhy 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2019-02-12 8:14 UTC (permalink / raw) To: Yauhen Kharuzhy; +Cc: linux-kernel, linux-leds, Andy Shevchenko On Sun, 10 Feb 2019, Yauhen Kharuzhy wrote: > Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC > mfd device driver. > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com> > --- > drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c > index 64a3aece9c5e..68b54173edd8 100644 > --- a/drivers/mfd/intel_soc_pmic_chtwc.c > +++ b/drivers/mfd/intel_soc_pmic_chtwc.c > @@ -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. > + { .name = "cht_wcove_leds", }, > }; > > /* -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc 2019-02-12 8:14 ` Lee Jones @ 2019-02-12 19:52 ` Yauhen Kharuzhy 0 siblings, 0 replies; 12+ messages in thread From: Yauhen Kharuzhy @ 2019-02-12 19:52 UTC (permalink / raw) To: Lee Jones; +Cc: linux-kernel, linux-leds On Tue, Feb 12, 2019 at 08:14:09AM +0000, Lee Jones wrote: > On Sun, 10 Feb 2019, Yauhen Kharuzhy wrote: > > > Add MFD cell for LEDs driver to the Intel Cherry Trail Whiskey Cove PMIC > > mfd device driver. > > > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com> > > --- > > drivers/mfd/intel_soc_pmic_chtwc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c > > index 64a3aece9c5e..68b54173edd8 100644 > > --- a/drivers/mfd/intel_soc_pmic_chtwc.c > > +++ b/drivers/mfd/intel_soc_pmic_chtwc.c > > @@ -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. > > > + { .name = "cht_wcove_leds", }, > > }; -- Yauhen Kharuzhy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support 2019-02-09 22:12 [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Yauhen Kharuzhy 2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy 2019-02-09 22:12 ` [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc Yauhen Kharuzhy @ 2019-02-11 11:59 ` Andy Shevchenko 2019-02-11 13:05 ` Pavel Machek 2 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2019-02-11 11:59 UTC (permalink / raw) To: Yauhen Kharuzhy; +Cc: linux-kernel, linux-leds, Lee Jones On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote: > This patch series introduces new driver for controlling LEDs connected > to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger > status led). Only simple 'always on' and blinking modes are supported > for now, no breathing. > > Driver was tested only with Lenovo Yoga Book notebook, and I don't have > any documentation for the PMIC, so proposals and testing are welcome. > I'm not sure you have a correct Cc list here. LED maintainers are M: Jacek Anaszewski <jacek.anaszewski@gmail.com> M: Pavel Machek <pavel@ucw.cz> Also would be nice to cc to Hans de Goede. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support 2019-02-11 11:59 ` [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Andy Shevchenko @ 2019-02-11 13:05 ` Pavel Machek 2019-02-11 21:20 ` Jacek Anaszewski 0 siblings, 1 reply; 12+ messages in thread From: Pavel Machek @ 2019-02-11 13:05 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Yauhen Kharuzhy, linux-kernel, linux-leds, Lee Jones [-- Attachment #1: Type: text/plain, Size: 950 bytes --] On Mon 2019-02-11 13:59:51, Andy Shevchenko wrote: > On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote: > > This patch series introduces new driver for controlling LEDs connected > > to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger > > status led). Only simple 'always on' and blinking modes are supported > > for now, no breathing. > > > > Driver was tested only with Lenovo Yoga Book notebook, and I don't have > > any documentation for the PMIC, so proposals and testing are welcome. > > > > I'm not sure you have a correct Cc list here. LED maintainers are > M: Jacek Anaszewski <jacek.anaszewski@gmail.com> > M: Pavel Machek <pavel@ucw.cz> > > Also would be nice to cc to Hans de Goede. I follow the list. I beileve Jacek does, too. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support 2019-02-11 13:05 ` Pavel Machek @ 2019-02-11 21:20 ` Jacek Anaszewski 0 siblings, 0 replies; 12+ messages in thread From: Jacek Anaszewski @ 2019-02-11 21:20 UTC (permalink / raw) To: Pavel Machek, Andy Shevchenko Cc: Yauhen Kharuzhy, linux-kernel, linux-leds, Lee Jones On 2/11/19 2:05 PM, Pavel Machek wrote: > On Mon 2019-02-11 13:59:51, Andy Shevchenko wrote: >> On Sun, Feb 10, 2019 at 01:12:12AM +0300, Yauhen Kharuzhy wrote: >>> This patch series introduces new driver for controlling LEDs connected >>> to Intel Cherry Trail Whiskey Cove PMIC (general-purpose LED and charger >>> status led). Only simple 'always on' and blinking modes are supported >>> for now, no breathing. >>> >>> Driver was tested only with Lenovo Yoga Book notebook, and I don't have >>> any documentation for the PMIC, so proposals and testing are welcome. >>> >> >> I'm not sure you have a correct Cc list here. LED maintainers are >> M: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> M: Pavel Machek <pavel@ucw.cz> >> >> 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. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-02-12 20:25 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-09 22:12 [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Yauhen Kharuzhy 2019-02-09 22:12 ` [PATCH 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC LEDs Yauhen Kharuzhy 2019-02-11 13:17 ` Pavel Machek 2019-02-12 20:25 ` Yauhen Kharuzhy 2019-02-11 21:36 ` Jacek Anaszewski 2019-02-12 19:50 ` Yauhen Kharuzhy 2019-02-09 22:12 ` [PATCH 2/2] mfd: Add leds MFD cell for intel_soc_pmic_chtwc Yauhen Kharuzhy 2019-02-12 8:14 ` Lee Jones 2019-02-12 19:52 ` Yauhen Kharuzhy 2019-02-11 11:59 ` [PATCH 0/2] Intel Cherry Trail Whiskey Cove LEDs support Andy Shevchenko 2019-02-11 13:05 ` Pavel Machek 2019-02-11 21:20 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).