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

[Lee Jones <lee@kernel.org>]

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.

-- 
Lee Jones [李琼斯]