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@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. 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. -- Lee Jones [李琼斯]
