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@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. 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
