Re: [PATCH v2 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede <hdegoede@redhat.com>]

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