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

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

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.




> 
>> + */
>> +
>> +#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@gmail.com>");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.39.2
>>
>