Hi Lee, On 4/24/23 16:15, Lee Jones wrote: > On Thu, 20 Apr 2023, Hans de Goede wrote: > >> From: Yauhen Kharuzhy <jekhor@xxxxxxxxx> >> >> 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@xxxxxxxxx >> Signed-off-by: Yauhen Kharuzhy <jekhor@xxxxxxxxx> >> Co-developed-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> 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@xxxxxxxxx> >> + * Copyright 2023 Hans de Goede <hansg@xxxxxxxxxx> >> + * >> + * 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@xxxxxxxxx>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.39.2 >> >
