Re: [PATCH v2 3/4] leds: tps68470: Add LED control for tps68470

[Dan Scally <dan.scally@ideasonboard.com>]

Hi Kate - thanks for the v2

On 10/03/2023 09:56, Kate Hsuan wrote:
> There are two LED controllers, LEDA indicator LED and LEDB flash LED for
> tps68470. LEDA can be enabled by setting TPS68470_ILEDCTL_ENA. Moreover,
> tps68470 provides four levels of power status for LEDB. If the
> properties called "ti,ledb-current" can be found, the current will be
> set according to the property values. These two LEDs can be controlled
> through the LED class of sysfs (tps68470-leda and tps68470-ledb).
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>   drivers/leds/Kconfig         |  12 +++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-tps68470.c | 182 +++++++++++++++++++++++++++++++++++
>   3 files changed, 195 insertions(+)
>   create mode 100644 drivers/leds/leds-tps68470.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9dbce09eabac..fd26036b3c61 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -827,6 +827,18 @@ config LEDS_TPS6105X
>   	  It is a single boost converter primarily for white LEDs and
>   	  audio amplifiers.
>   
> +config LEDS_TPS68470
> +	tristate "LED support for TI TPS68470"
> +	depends on LEDS_CLASS
> +	depends on INTEL_SKL_INT3472
> +	help
> +	  This driver supports TPS68470 PMIC with LED chip.
> +	  It provide two LED controllers, including an indicator LED
> +	  and a flash LED.

s/provide/provides. Also maybe "It provides two LED controllers, with the ability to drive 2 
indicator LEDs and 2 flash LEDs". I actually got the WLED part working now finally so I'll send 
patches on top of this series if that's ok?

> +
> +	  To compile this driver as a module, choose M and it will be
> +	  called leds-tps68470
> +
>   config LEDS_IP30
>   	tristate "LED support for SGI Octane machines"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d30395d11fd8..b284bc0daa98 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+= leds-turris-omnia.o
>   obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
>   obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
>   obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
> +obj-$(CONFIG_LEDS_TPS68470)		+= leds-tps68470.o
>   
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> new file mode 100644
> index 000000000000..98bb56153690
> --- /dev/null
> +++ b/drivers/leds/leds-tps68470.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LED driver for TPS68470 PMIC
> + *
> + * Copyright (C) 2023 Red Hat
> + *
> + * Authors:
> + *	Kate Hsuan <hpa@redhat.com>
> + */
> +
> +#include <linux/gpio/driver.h>

Not needed I think?

> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/leds.h>

Alphabetical order?
> +
> +#define lcdev_to_led(led_cdev) \
> +	container_of(led_cdev, struct tps68470_led, lcdev)
> +
> +#define led_to_tps68470(led, index) \
> +	container_of(led, struct tps68470_device, leds[index])
> +
> +enum tps68470_led_ids {
> +	TPS68470_ILED_A,
> +	TPS68470_ILED_B,
> +	TPS68470_NUM_LEDS
> +};
> +
> +static const char *tps68470_led_names[] = {
> +	[TPS68470_ILED_A] = "tps68470-iled_a",
> +	[TPS68470_ILED_B] = "tps68470-iled_b",
> +};
> +
> +struct tps68470_led {
> +	unsigned int led_id;
> +	struct led_classdev lcdev;
> +};
> +
> +struct tps68470_device {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct tps68470_led leds[TPS68470_NUM_LEDS];
> +};
> +
> +enum ctrlb_current {
> +	CTRLB_2MA	= 0,
> +	CTRLB_4MA	= 1,
> +	CTRLB_8MA	= 2,
> +	CTRLB_16MA	= 3,
> +};
> +
> +static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brightness brightness)
> +{
> +	struct tps68470_led *led = lcdev_to_led(led_cdev);
> +	struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id);
> +	struct regmap *regmap = tps68470->regmap
This would work fine as is...but I would maybe add something like

	if (state > LED_ON)
		return -EINVAL;

So that brightness values of > 1 aren't just silently accepted...or does the LED core already 
prevent that? If so it's fine.

> +
> +	switch (led->led_id) {
> +	case TPS68470_ILED_A:
> +		return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENA,
> +					  brightness ? TPS68470_ILEDCTL_ENA : 0);
> +	case TPS68470_ILED_B:
> +		return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB,
> +					  brightness ? TPS68470_ILEDCTL_ENB : 0);
> +	}
> +	return -EINVAL;
> +}
> +
> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> +{
> +	struct tps68470_led *led = lcdev_to_led(led_cdev);
> +	struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id);
> +	struct regmap *regmap = tps68470->regmap;
> +	int ret = 0;
> +	int value = 0;
> +
> +	ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
> +	if (ret)
> +		goto error;

Just return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n") here imo.
> +
> +	switch (led->led_id) {
> +	case TPS68470_ILED_A:
> +		value = value & TPS68470_ILEDCTL_ENA;
> +		break;
> +	case TPS68470_ILED_B:
> +		value = value & TPS68470_ILEDCTL_ENB;
> +		break;
> +	}
> +
> +	return value ? LED_ON : LED_OFF;
> +
> +error:
> +	dev_err(led_cdev->dev, "Failed on reading register\n");
> +	return -EINVAL;
> +}
> +
> +static int tps68470_leds_probe(struct platform_device *pdev)
> +{
> +	int i = 0;
> +	int ret = 0;
> +	unsigned int curr;
> +	struct tps68470_device *tps68470;
> +	struct tps68470_led *led;
> +	struct led_classdev *lcdev;
> +
> +	tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device),
> +				GFP_KERNEL);

No -ENOMEM check here?

> +	tps68470->dev = &pdev->dev;
> +	tps68470->regmap = dev_get_drvdata(pdev->dev.parent);
> +
> +	for (i = 0; i < TPS68470_NUM_LEDS; i++) {
> +		led = &tps68470->leds[i];
> +		lcdev = &led->lcdev;
> +
> +		led->led_id = i;
> +
> +		lcdev->name = devm_kasprintf(tps68470->dev, GFP_KERNEL, "%s::%s",
> +					     tps68470_led_names[i], LED_FUNCTION_INDICATOR);
> +		if (!lcdev->name)
> +			return -ENOMEM;
> +
> +		lcdev->max_brightness = 1;
> +		lcdev->brightness = 0;
> +		lcdev->brightness_set_blocking = tps68470_brightness_set;
> +		lcdev->brightness_get = tps68470_brightness_get;
> +		lcdev->dev = &pdev->dev;
> +
> +		ret = devm_led_classdev_register(tps68470->dev, lcdev);
> +		if (ret) {
> +			dev_err_probe(tps68470->dev, ret,
> +				      "error registering led\n");
> +			goto err_exit;
> +		}
> +	}
> +
> +	/* configure LEDB current if the properties can be got */
> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
> +		switch (curr) {
> +		case  2:
> +			curr = CTRLB_2MA;
> +			break;
> +		case  4:
> +			curr = CTRLB_4MA;
> +			break;
> +		case  8:
> +			curr = CTRLB_8MA;
> +			break;
> +		case 16:
> +			curr = CTRLB_16MA;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "Invalid LEDB curr value: %d\n", curr);
> +			return -EINVAL;

There's no jump to err_exit here...I think that this whole section should go above the registration 
of the LEDS...and probably also into its own function.

> +		}
> +		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL,
> +					 TPS68470_ILEDCTL_CTRLB, curr);
> +	}
> +
> +err_exit:
> +	if (ret) {
> +		for (i = 0; i < TPS68470_NUM_LEDS; i++) {
> +			if (tps68470->leds[i].lcdev.name)
> +				devm_led_classdev_unregister(&pdev->dev,
> +							     &tps68470->leds[i].lcdev);
> +		}
> +	}
> +
> +	return ret;
> +}
> +static struct platform_driver tps68470_led_driver = {
> +	.driver = {
> +		   .name = "tps68470-led",
> +	},
> +	.probe = tps68470_leds_probe,
> +};
> +
> +module_platform_driver(tps68470_led_driver);
> +
> +MODULE_ALIAS("platform:tps68470-led");
> +MODULE_DESCRIPTION("LED driver for TPS68470 PMIC");
> +MODULE_LICENSE("GPL v2");