| Message ID | 20230420123741.57160-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver | expand |
On Thu, 20 Apr 2023, 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. > > 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> > --- > Changes in v2: > - Update comment about YB1 kernel source usage for register info > - Replace "cht-wc::" LED name prefix with "platform::" > - Add leds-cht-wcove to list of drivers using "platform::charging" in > Documentation/leds/well-known-leds.txt > - Bail from cht_wc_leds_brightness_set() on first error > - Make default blink 1Hz, like sw-blink default blink > --- > Documentation/leds/well-known-leds.txt | 2 +- > drivers/leds/Kconfig | 11 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-cht-wcove.c | 373 +++++++++++++++++++++++++ > 4 files changed, 386 insertions(+), 1 deletion(-) > create mode 100644 drivers/leds/leds-cht-wcove.c Generally nice. Couple of small nits. > diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt > index 2160382c86be..7640debee6c0 100644 > --- a/Documentation/leds/well-known-leds.txt > +++ b/Documentation/leds/well-known-leds.txt > @@ -65,7 +65,7 @@ Phones usually have multi-color status LED. > > * Power management > > -Good: "platform:*:charging" (allwinner sun50i) > +Good: "platform:*:charging" (allwinner sun50i, leds-cht-wcove) > > * Screen > > 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..908965e25552 > --- /dev/null > +++ b/drivers/leds/leds-cht-wcove.c > @@ -0,0 +1,373 @@ > +// 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> > + * > + * Register info comes from the Lenovo Yoga Book Android kernel sources: > + * YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c. How does one browse to this? > + */ > + > +#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> So close! > +#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 */ If you're documenting a single define, care to put it on the same line? > +#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] = { > + "platform::" LED_FUNCTION_CHARGING, > + "platform::" 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 < 0) { > + dev_err(cdev->dev, "Failed to turn off: %d\n", ret); > + goto out; > + } > + > + /* 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 < 0) { > + dev_err(cdev->dev, "Failed to set brightness: %d\n", ret); > + goto out; > + } > + > + ret = regmap_update_bits(led->regmap, led->regs->ctrl, > + led->regs->on_off_mask, led->regs->on_val); > + if (ret < 0) > + dev_err(cdev->dev, "Failed to turn on: %d\n", ret); > + } > +out: > + 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; include/linux/leds.h: /* This is obsolete/useless. We now support variable maximum brightness. */ enum led_brightness { LED_OFF = 0, LED_ON = 1, LED_HALF = 127, LED_FULL = 255, }; > + 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); > + > + /* blink with 1 Hz as default if nothing specified */ Nit: "Blink" > + if (!*delay_on && !*delay_off) > + *delay_on = *delay_off = 500; > + > + 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); platform_*() > + 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. */ Either use full-stops, or don't. Please be consistent. > + 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, This is new to me. What does remove_new do? Just returns void? > + .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.2 >
Jacek, On Thu, 20 Apr 2023, 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> Is this what you were after? > Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/ > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2 > - Improve/extend Documentation/leds/leds-cht-wcove.rst a bit > --- > Documentation/leds/index.rst | 1 + > Documentation/leds/leds-cht-wcove.rst | 38 ++++++++++++++++++++++++ > drivers/leds/leds-cht-wcove.c | 42 ++++++++++++++++++++++++--- > 3 files changed, 77 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..5ec7cb60c4aa > --- /dev/null > +++ b/Documentation/leds/leds-cht-wcove.rst > @@ -0,0 +1,38 @@ > +.. 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. > + > +The set 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. > diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c > index 166c6f010492..23e97b08f6ea 100644 > --- a/drivers/leds/leds-cht-wcove.c > +++ b/drivers/leds/leds-cht-wcove.c > @@ -218,9 +218,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; > @@ -247,7 +248,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); > > @@ -266,6 +267,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) > { > @@ -322,6 +354,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.2 >
Hi Lee, On 4/24/23 16:15, Lee Jones wrote: > On Thu, 20 Apr 2023, 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. >> >> 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> >> --- >> Changes in v2: >> - Update comment about YB1 kernel source usage for register info >> - Replace "cht-wc::" LED name prefix with "platform::" >> - Add leds-cht-wcove to list of drivers using "platform::charging" in >> Documentation/leds/well-known-leds.txt >> - Bail from cht_wc_leds_brightness_set() on first error >> - Make default blink 1Hz, like sw-blink default blink >> --- >> Documentation/leds/well-known-leds.txt | 2 +- >> drivers/leds/Kconfig | 11 + >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-cht-wcove.c | 373 +++++++++++++++++++++++++ >> 4 files changed, 386 insertions(+), 1 deletion(-) >> create mode 100644 drivers/leds/leds-cht-wcove.c > > Generally nice. Couple of small nits. > >> diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt >> index 2160382c86be..7640debee6c0 100644 >> --- a/Documentation/leds/well-known-leds.txt >> +++ b/Documentation/leds/well-known-leds.txt >> @@ -65,7 +65,7 @@ Phones usually have multi-color status LED. >> >> * Power management >> >> -Good: "platform:*:charging" (allwinner sun50i) >> +Good: "platform:*:charging" (allwinner sun50i, leds-cht-wcove) >> >> * Screen >> >> 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..908965e25552 >> --- /dev/null >> +++ b/drivers/leds/leds-cht-wcove.c >> @@ -0,0 +1,373 @@ >> +// 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> >> + * >> + * Register info comes from the Lenovo Yoga Book Android kernel sources: >> + * YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c. > > How does one browse to this? There is a tarbal with kernel sources available for download from the support page for the Android version of the Yoga Book (yb1-x90f / yb1-x90l). This is the file path within that tarbal. I add a deep-link to the tarbal here, but I'm afraid that will not be a stable link. Or I guess I could omit the filename too? I added the filename because even if you have the tarbal the file is still sort of non trivial to find. > >> + */ >> + >> +#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> > > So close! I assume you point to the includes being almost alphabetically sorted ? Ack, will fix for v3. > >> +#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 */ > > If you're documenting a single define, care to put it on the same line? Ack, will fix for v3. > >> +#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] = { >> + "platform::" LED_FUNCTION_CHARGING, >> + "platform::" 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 < 0) { >> + dev_err(cdev->dev, "Failed to turn off: %d\n", ret); >> + goto out; >> + } >> + >> + /* 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 < 0) { >> + dev_err(cdev->dev, "Failed to set brightness: %d\n", ret); >> + goto out; >> + } >> + >> + ret = regmap_update_bits(led->regmap, led->regs->ctrl, >> + led->regs->on_off_mask, led->regs->on_val); >> + if (ret < 0) >> + dev_err(cdev->dev, "Failed to turn on: %d\n", ret); >> + } >> +out: >> + 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; > > > include/linux/leds.h: > > /* This is obsolete/useless. We now support variable maximum brightness. */ > enum led_brightness { > LED_OFF = 0, > LED_ON = 1, > LED_HALF = 127, > LED_FULL = 255, > }; I know but LED_OFF is still somewhat useful, it makes it clear that wat is being returned is a brightness value where as "ret = 0" reads like returning success. With that said if you prefer 0/1 over LED_OFF / LED_ON I'm happy to replace them all ? > >> + 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); >> + >> + /* blink with 1 Hz as default if nothing specified */ > > Nit: "Blink" Ack. >> + if (!*delay_on && !*delay_off) >> + *delay_on = *delay_off = 500; >> + >> + 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); > > platform_*() This is getting the parent's driver data so platform_get_drvdata() can not be used here. >> + 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. */ > > Either use full-stops, or don't. Please be consistent. I added the full-stop here because there is a ',' in the comment, I'll drop it. > >> + 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, > > This is new to me. What does remove_new do? > > Just returns void? Yes all the platform_device remove callbacks are being moved over to this which indeed returns void. Regards, Hans > >> + .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.2 >> >
On 4/24/23 16:16, Lee Jones wrote: > Jacek, > > On Thu, 20 Apr 2023, 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> > > Is this what you were after? I remember I recommended to Hans using pattern trigger for hw patterns somewhere in 2019, so yes. >> Link: https://lore.kernel.org/all/6beed61c-1fc6-6525-e873-a8978f5fbffb@gmail.com/ >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2 >> - Improve/extend Documentation/leds/leds-cht-wcove.rst a bit
On Mon, 24 Apr 2023, Hans de Goede wrote: > Hi Lee, > > On 4/24/23 16:15, Lee Jones wrote: > > On Thu, 20 Apr 2023, 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. > >> > >> 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> > >> --- > >> Changes in v2: > >> - Update comment about YB1 kernel source usage for register info > >> - Replace "cht-wc::" LED name prefix with "platform::" > >> - Add leds-cht-wcove to list of drivers using "platform::charging" in > >> Documentation/leds/well-known-leds.txt > >> - Bail from cht_wc_leds_brightness_set() on first error > >> - Make default blink 1Hz, like sw-blink default blink > >> --- > >> Documentation/leds/well-known-leds.txt | 2 +- > >> drivers/leds/Kconfig | 11 + > >> drivers/leds/Makefile | 1 + > >> drivers/leds/leds-cht-wcove.c | 373 +++++++++++++++++++++++++ > >> 4 files changed, 386 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/leds/leds-cht-wcove.c > > > > Generally nice. Couple of small nits. > > > >> diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt > >> index 2160382c86be..7640debee6c0 100644 > >> --- a/Documentation/leds/well-known-leds.txt > >> +++ b/Documentation/leds/well-known-leds.txt > >> @@ -65,7 +65,7 @@ Phones usually have multi-color status LED. > >> > >> * Power management > >> > >> -Good: "platform:*:charging" (allwinner sun50i) > >> +Good: "platform:*:charging" (allwinner sun50i, leds-cht-wcove) > >> > >> * Screen > >> > >> 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..908965e25552 > >> --- /dev/null > >> +++ b/drivers/leds/leds-cht-wcove.c > >> @@ -0,0 +1,373 @@ > >> +// 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> > >> + * > >> + * Register info comes from the Lenovo Yoga Book Android kernel sources: > >> + * YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c. > > > > How does one browse to this? > > There is a tarbal with kernel sources available for download from > the support page for the Android version of the Yoga Book (yb1-x90f / yb1-x90l). > > This is the file path within that tarbal. I add a deep-link > to the tarbal here, but I'm afraid that will not be a stable link. > > Or I guess I could omit the filename too? I added the filename because > even if you have the tarbal the file is still sort of non trivial to find. That's not the issue I have with it. How about: <tarball>/YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c Or: file:///YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c [...] > >> +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; > > > > > > include/linux/leds.h: > > > > /* This is obsolete/useless. We now support variable maximum brightness. */ > > enum led_brightness { > > LED_OFF = 0, > > LED_ON = 1, > > LED_HALF = 127, > > LED_FULL = 255, > > }; > > I know but LED_OFF is still somewhat useful, it makes it > clear that wat is being returned is a brightness value > where as "ret = 0" reads like returning success. > > With that said if you prefer 0/1 over LED_OFF / LED_ON > I'm happy to replace them all ? This is probably for Pavel to answer. Ideally it'll either be: "still useful and thus not deprecated" Or: "deprecated and therefore must not be used" I'm less happy with a deprecated but still okay to use limbo-land. [...] > >> +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); > > > > platform_*() > > This is getting the parent's driver data so platform_get_drvdata() > can not be used here. Fair point. > >> + 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. */ > > > > Either use full-stops, or don't. Please be consistent. > > I added the full-stop here because there is a ',' in the comment, I'll drop it. Please apply this review comment widely, not just for this one line. [...] > >> +static struct platform_driver cht_wc_leds_driver = { > >> + .probe = cht_wc_leds_probe, > >> + .remove_new = cht_wc_leds_remove, > > > > This is new to me. What does remove_new do? > > > > Just returns void? > > Yes all the platform_device remove callbacks are being moved over to this which > indeed returns void. Thanks.
Hi, On 4/27/23 18:57, Lee Jones wrote: > On Mon, 24 Apr 2023, Hans de Goede wrote: > >> Hi Lee, >> >> On 4/24/23 16:15, Lee Jones wrote: >>> On Thu, 20 Apr 2023, 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. >>>> >>>> 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> >>>> --- >>>> Changes in v2: >>>> - Update comment about YB1 kernel source usage for register info >>>> - Replace "cht-wc::" LED name prefix with "platform::" >>>> - Add leds-cht-wcove to list of drivers using "platform::charging" in >>>> Documentation/leds/well-known-leds.txt >>>> - Bail from cht_wc_leds_brightness_set() on first error >>>> - Make default blink 1Hz, like sw-blink default blink >>>> --- >>>> Documentation/leds/well-known-leds.txt | 2 +- >>>> drivers/leds/Kconfig | 11 + >>>> drivers/leds/Makefile | 1 + >>>> drivers/leds/leds-cht-wcove.c | 373 +++++++++++++++++++++++++ >>>> 4 files changed, 386 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/leds/leds-cht-wcove.c >>> >>> Generally nice. Couple of small nits. >>> >>>> diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt >>>> index 2160382c86be..7640debee6c0 100644 >>>> --- a/Documentation/leds/well-known-leds.txt >>>> +++ b/Documentation/leds/well-known-leds.txt >>>> @@ -65,7 +65,7 @@ Phones usually have multi-color status LED. >>>> >>>> * Power management >>>> >>>> -Good: "platform:*:charging" (allwinner sun50i) >>>> +Good: "platform:*:charging" (allwinner sun50i, leds-cht-wcove) >>>> >>>> * Screen >>>> >>>> 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..908965e25552 >>>> --- /dev/null >>>> +++ b/drivers/leds/leds-cht-wcove.c >>>> @@ -0,0 +1,373 @@ >>>> +// 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> >>>> + * >>>> + * Register info comes from the Lenovo Yoga Book Android kernel sources: >>>> + * YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c. >>> >>> How does one browse to this? >> >> There is a tarbal with kernel sources available for download from >> the support page for the Android version of the Yoga Book (yb1-x90f / yb1-x90l). >> >> This is the file path within that tarbal. I add a deep-link >> to the tarbal here, but I'm afraid that will not be a stable link. >> >> Or I guess I could omit the filename too? I added the filename because >> even if you have the tarbal the file is still sort of non trivial to find. > > That's not the issue I have with it. > > How about: > > <tarball>/YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c > > Or: > > file:///YB1_source_code/kernel/cht/drivers/misc/charger_gp_led.c > > [...] > >>>> +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; >>> >>> >>> include/linux/leds.h: >>> >>> /* This is obsolete/useless. We now support variable maximum brightness. */ >>> enum led_brightness { >>> LED_OFF = 0, >>> LED_ON = 1, >>> LED_HALF = 127, >>> LED_FULL = 255, >>> }; >> >> I know but LED_OFF is still somewhat useful, it makes it >> clear that wat is being returned is a brightness value >> where as "ret = 0" reads like returning success. >> >> With that said if you prefer 0/1 over LED_OFF / LED_ON >> I'm happy to replace them all ? > > This is probably for Pavel to answer. > > Ideally it'll either be: > > "still useful and thus not deprecated" > > Or: > > "deprecated and therefore must not be used" > > I'm less happy with a deprecated but still okay to use limbo-land. I understand. I'll just replace the use of LED_OFF with 0 for the v3 I'm preparing, as well as address all your other review remarks. Regards, Hans
Hi All, Here is v2 of the patch series to add support for the LED controller on Intel Cherry Trail Whiskey Cove PMICs. Changes in v2 (of my version of this): - Address Pavel's small remarks on patch 1/5, see patch 1/5's commit message for details - Improve/extend pattern docs in Documentation/leds/leds-cht-wcove.rst 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 | 38 ++ Documentation/leds/well-known-leds.txt | 2 +- drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/leds-cht-wcove.c | 471 +++++++++++++++++++++++++ 6 files changed, 523 insertions(+), 1 deletion(-) create mode 100644 Documentation/leds/leds-cht-wcove.rst create mode 100644 drivers/leds/leds-cht-wcove.c