[RESEND PATCH 0/3] leds: tps68470: LED driver for TPS68470

[RESEND PATCH 0/3] leds: tps68470: LED driver for TPS68470

[Kate Hsuan]

This work allows user to control the indicator and flash LEDs though
sysfs in user space. TPS68470 is a part of INT3470 and provides GPIO,
LED, and power controls.

To allow the user to control these two LEDs, two masks for the
ILEDCTL, called TPS68470_ILEDCTL_ENA and TPS68470_ILEDCTL_ENA and a two
bits mask, called TPS68470_ILEDCTL_CTRLB are defined and are used to
enable/disable the LEDs and set the power status, respectively.

The LED driver called leds-tps68470 provides the sysfs interface and
the register configuration implementation. For the indicator LED,
only TPS68470_ILEDCTL_ENA needs to be set to turn on/off the LED. For the
flash LED, TPS68470 provides 4 levels of power settings. After enabling
the TPS68470_ILEDCTL_ENB, the power level should also be set.
The strategy is

Brightness  |  Power
=======================
LED_OFF     |   -
LED_ON      |   4mA
LED_HALF    |   8mA
LED_FULL    |   16mA

Moreover, the user and application can set the brightness through sysfs,
/sys/class/leds/tps68470-ileda and /sys/class/leds/tps68470-iledb

Kate Hsuan (3):
  platform: x86: int3472: Add MFD cell for tps68470 LED
  include: mfd: tps68470: Add masks for LEDA and LEDB
  leds: tps68470: Add LED control for tps68470

 drivers/leds/Kconfig                          |  12 ++
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-tps68470.c                  | 170 ++++++++++++++++++
 drivers/platform/x86/intel/int3472/tps68470.c |   3 +-
 include/linux/mfd/tps68470.h                  |   5 +
 5 files changed, 190 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-tps68470.c

-- 
2.39.0

[RESEND PATCH 1/3] platform: x86: int3472: Add MFD cell for tps68470 LED

[Kate Hsuan]

Add MFD cell for tps68470-led.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 5b8d1a9620a5..9dceb6507a01 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -17,7 +17,7 @@
 #define DESIGNED_FOR_CHROMEOS		1
 #define DESIGNED_FOR_WINDOWS		2
 
-#define TPS68470_WIN_MFD_CELL_COUNT	3
+#define TPS68470_WIN_MFD_CELL_COUNT	4
 
 static const struct mfd_cell tps68470_cros[] = {
 	{ .name = "tps68470-gpio" },
@@ -194,6 +194,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
 		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
 		cells[2].name = "tps68470-gpio";
+		cells[3].name = "tps68470-led";
 
 		for (i = 0; i < board_data->n_gpiod_lookups; i++)
 			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
-- 
2.39.0

[RESEND PATCH 2/3] include: mfd: tps68470: Add masks for LEDA and LEDB

[Kate Hsuan]

Add flags for both LEDA(TPS68470_ILEDCTL_ENA), LEDB
(TPS68470_ILEDCTL_ENB), and current control mask for LEDB
(TPS68470_ILEDCTL_CTRLB)

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 include/linux/mfd/tps68470.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
index 7807fa329db0..2d2abb25b944 100644
--- a/include/linux/mfd/tps68470.h
+++ b/include/linux/mfd/tps68470.h
@@ -34,6 +34,7 @@
 #define TPS68470_REG_SGPO		0x22
 #define TPS68470_REG_GPDI		0x26
 #define TPS68470_REG_GPDO		0x27
+#define TPS68470_REG_ILEDCTL		0x28
 #define TPS68470_REG_VCMVAL		0x3C
 #define TPS68470_REG_VAUX1VAL		0x3D
 #define TPS68470_REG_VAUX2VAL		0x3E
@@ -94,4 +95,8 @@
 #define TPS68470_GPIO_MODE_OUT_CMOS	2
 #define TPS68470_GPIO_MODE_OUT_ODRAIN	3
 
+#define TPS68470_ILEDCTL_ENA		BIT(2)
+#define TPS68470_ILEDCTL_ENB		BIT(6)
+#define TPS68470_ILEDCTL_CTRLB		GENMASK(5, 4)
+
 #endif /* __LINUX_MFD_TPS68470_H */
-- 
2.39.0

[RESEND PATCH 3/3] leds: tps68470: Add LED control for tps68470

[Kate Hsuan]

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 4 levels of power status for LEDB, so after setting
TPS68470_ILEDCTL_ENB, the current status field (TPS68470_ILEDCTL_CTRLB)
should also be set according to the brightness value from user space.
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 | 170 +++++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 drivers/leds/leds-tps68470.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..453404cb1329 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -846,6 +846,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.
+
+	  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 4fd2f92cd198..0a2ec01e27d9 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -86,6 +86,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..6243e7a4a718
--- /dev/null
+++ b/drivers/leds/leds-tps68470.c
@@ -0,0 +1,170 @@
+// 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>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/leds.h>
+
+struct tps68470_led_data {
+	struct regmap *tps68470_regmap;
+	unsigned int brightness_a;
+	unsigned int brightness_b;
+	struct led_classdev leda_cdev;
+	struct led_classdev ledb_cdev;
+};
+
+enum ctrlb_current {
+	CTRLB_2MA	= 0,
+	CTRLB_4MA	= 1,
+	CTRLB_8MA	= 2,
+	CTRLB_16MA	= 3,
+};
+
+static int set_ledb_current(struct regmap *regmap,
+			    unsigned int *data_brightness,
+			    enum led_brightness brightness)
+{
+	unsigned int ledb_current;
+
+	switch (brightness) {
+	case LED_HALF:
+		ledb_current = CTRLB_8MA;
+		break;
+	case LED_FULL:
+		ledb_current = CTRLB_16MA;
+		break;
+	case LED_ON:
+		ledb_current = CTRLB_4MA;
+		break;
+	case LED_OFF:
+		ledb_current = CTRLB_2MA;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*data_brightness = brightness;
+	return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL,
+				  TPS68470_ILEDCTL_CTRLB, ledb_current);
+}
+
+static int tps68470_brightness_set(struct led_classdev *led_cdev,
+				   enum led_brightness brightness)
+{
+	struct tps68470_led_data *data;
+	struct regmap *regmap;
+	unsigned int mask;
+	unsigned int value;
+	int ret;
+
+	if (!strncmp(led_cdev->name, "tps68470-ileda", 14)) {
+		data = container_of(led_cdev, struct tps68470_led_data, leda_cdev);
+		regmap = data->tps68470_regmap;
+		data->brightness_a = brightness ? TPS68470_ILEDCTL_ENA : 0;
+		mask = TPS68470_ILEDCTL_ENA;
+		value = data->brightness_a;
+	} else if (!strncmp(led_cdev->name, "tps68470-iledb", 14)) {
+		data = container_of(led_cdev, struct tps68470_led_data, ledb_cdev);
+		regmap = data->tps68470_regmap;
+		mask = TPS68470_ILEDCTL_ENB;
+		value = brightness ? TPS68470_ILEDCTL_ENB : 0;
+		/* Set current state for ledb */
+		ret = set_ledb_current(regmap, &data->brightness_b, brightness);
+		if (ret)
+			goto err_exit;
+	} else
+		return -EINVAL;
+
+	ret = regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, mask, value);
+
+err_exit:
+	return ret;
+}
+
+static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
+{
+	struct tps68470_led_data *data = container_of(led_cdev,
+						      struct tps68470_led_data,
+						      ledb_cdev);
+
+	if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
+		return data->brightness_a;
+	else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
+		return data->brightness_b;
+
+	return -EINVAL;
+}
+
+static int tps68470_led_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct tps68470_led_data *tps68470_led;
+
+	tps68470_led = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_led_data),
+				    GFP_KERNEL);
+	if (!tps68470_led)
+		return -ENOMEM;
+
+	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
+	tps68470_led->leda_cdev.name = "tps68470-ileda";
+	tps68470_led->leda_cdev.max_brightness = 1;
+	tps68470_led->leda_cdev.brightness_set_blocking = tps68470_brightness_set;
+	tps68470_led->leda_cdev.brightness_get = tps68470_brightness_get;
+	tps68470_led->leda_cdev.dev = &pdev->dev;
+	tps68470_led->brightness_a = 0;
+	ret = led_classdev_register(&pdev->dev, &tps68470_led->leda_cdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register LEDA: %d\n", ret);
+		return ret;
+	}
+
+	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
+	tps68470_led->ledb_cdev.name = "tps68470-iledb";
+	tps68470_led->ledb_cdev.max_brightness = 255;
+	tps68470_led->ledb_cdev.brightness_set_blocking = tps68470_brightness_set;
+	tps68470_led->ledb_cdev.brightness_get = tps68470_brightness_get;
+	tps68470_led->ledb_cdev.dev = &pdev->dev;
+	tps68470_led->brightness_b = 0;
+	ret = led_classdev_register(&pdev->dev, &tps68470_led->ledb_cdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register LEDB: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, tps68470_led);
+
+	return ret;
+}
+
+static int tps68470_led_remove(struct platform_device *pdev)
+{
+	struct tps68470_led_data *data = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&data->leda_cdev);
+	led_classdev_unregister(&data->ledb_cdev);
+
+	return 0;
+}
+
+static struct platform_driver tps68470_led_driver = {
+	.driver = {
+		   .name = "tps68470-led",
+	},
+	.probe = tps68470_led_probe,
+	.remove = tps68470_led_remove,
+};
+module_platform_driver(tps68470_led_driver);
+
+MODULE_ALIAS("platform:tps68470-led");
+MODULE_DESCRIPTION("LED driver for TPS68470 PMIC");
+MODULE_LICENSE("GPL v2");
-- 
2.39.0

Re: [RESEND PATCH 1/3] platform: x86: int3472: Add MFD cell for tps68470 LED

[Hans de Goede]

Hi Kate,

On 2/13/23 13:45, Kate Hsuan wrote:
> Add MFD cell for tps68470-led.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 5b8d1a9620a5..9dceb6507a01 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -17,7 +17,7 @@
>  #define DESIGNED_FOR_CHROMEOS		1
>  #define DESIGNED_FOR_WINDOWS		2
>  
> -#define TPS68470_WIN_MFD_CELL_COUNT	3
> +#define TPS68470_WIN_MFD_CELL_COUNT	4
>  
>  static const struct mfd_cell tps68470_cros[] = {
>  	{ .name = "tps68470-gpio" },
> @@ -194,6 +194,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
>  		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
>  		cells[2].name = "tps68470-gpio";
> +		cells[3].name = "tps68470-led";

As the comment a few lines above states, the gpio cell must be last because the GPIO
driver ends up calling acpi_dev_clear_dependencies() which will cause the ACPI code
to instantiate the i2c_client-s for the camera sensors and we want all the cells
to be instantiated before this happens, so this needs to become:

		cells[2].name = "tps68470-led";
  		cells[3].name = "tps68470-gpio";

And for the sensor to actually be able to use the LEDA output as privacy LED,
we also need to add a led lookup-table entry linking the sensor and the LEDA
LED-class-device. I'll write a patch for this and submit that upstream soonish.

For v2 of this patch-set, please include my patch with the lookup as a 4th patch.

Regards,

Hans






>  
>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
>  			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);

Re: [RESEND PATCH 2/3] include: mfd: tps68470: Add masks for LEDA and LEDB

[Hans de Goede]

Hi,

On 2/13/23 13:45, Kate Hsuan wrote:
> Add flags for both LEDA(TPS68470_ILEDCTL_ENA), LEDB
> (TPS68470_ILEDCTL_ENB), and current control mask for LEDB
> (TPS68470_ILEDCTL_CTRLB)
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  include/linux/mfd/tps68470.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
> index 7807fa329db0..2d2abb25b944 100644
> --- a/include/linux/mfd/tps68470.h
> +++ b/include/linux/mfd/tps68470.h
> @@ -34,6 +34,7 @@
>  #define TPS68470_REG_SGPO		0x22
>  #define TPS68470_REG_GPDI		0x26
>  #define TPS68470_REG_GPDO		0x27
> +#define TPS68470_REG_ILEDCTL		0x28
>  #define TPS68470_REG_VCMVAL		0x3C
>  #define TPS68470_REG_VAUX1VAL		0x3D
>  #define TPS68470_REG_VAUX2VAL		0x3E
> @@ -94,4 +95,8 @@
>  #define TPS68470_GPIO_MODE_OUT_CMOS	2
>  #define TPS68470_GPIO_MODE_OUT_ODRAIN	3
>  
> +#define TPS68470_ILEDCTL_ENA		BIT(2)
> +#define TPS68470_ILEDCTL_ENB		BIT(6)
> +#define TPS68470_ILEDCTL_CTRLB		GENMASK(5, 4)
> +
>  #endif /* __LINUX_MFD_TPS68470_H */

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

[Hans de Goede]

Hi Kate,

On 2/13/23 13:45, 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 4 levels of power status for LEDB, so after setting
> TPS68470_ILEDCTL_ENB, the current status field (TPS68470_ILEDCTL_CTRLB)
> should also be set according to the brightness value from user space.
> 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 | 170 +++++++++++++++++++++++++++++++++++
>  3 files changed, 183 insertions(+)
>  create mode 100644 drivers/leds/leds-tps68470.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 499d0f215a8b..453404cb1329 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -846,6 +846,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.
> +
> +	  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 4fd2f92cd198..0a2ec01e27d9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -86,6 +86,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..6243e7a4a718
> --- /dev/null
> +++ b/drivers/leds/leds-tps68470.c
> @@ -0,0 +1,170 @@
> +// 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>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/leds.h>
> +
> +struct tps68470_led_data {
> +	struct regmap *tps68470_regmap;
> +	unsigned int brightness_a;
> +	unsigned int brightness_b;
> +	struct led_classdev leda_cdev;
> +	struct led_classdev ledb_cdev;
> +};
> +
> +enum ctrlb_current {
> +	CTRLB_2MA	= 0,
> +	CTRLB_4MA	= 1,
> +	CTRLB_8MA	= 2,
> +	CTRLB_16MA	= 3,
> +};
> +
> +static int set_ledb_current(struct regmap *regmap,
> +			    unsigned int *data_brightness,
> +			    enum led_brightness brightness)
> +{
> +	unsigned int ledb_current;
> +
> +	switch (brightness) {
> +	case LED_HALF:
> +		ledb_current = CTRLB_8MA;
> +		break;
> +	case LED_FULL:
> +		ledb_current = CTRLB_16MA;
> +		break;

LED_FULL is 255, now a days the LED class supports custom
brightness ranges, so you could just set max_brightness to 4
and then use brightness 1-4 to map to the 4 values.

> +	case LED_ON:
> +		ledb_current = CTRLB_4MA;
> +		break;
> +	case LED_OFF:
> +		ledb_current = CTRLB_2MA;
> +		break;

This makes no sense, when brightness == LED_OFF you
disable the LEDB output, so the configured current does
not matter.

But I believe that mapping the current to brightness is
not how this should be done / modeled. With the separate
on/off toggle this is clearly not intended to allow dimming
a LED. This is configurable to allow adjusting for different
notification LED types, but the intention is still for LEDB
to be used as a simple on/off notification LED.

(The lack of e.g. actual PWM / fine grained current control
vs doubling the current each step to me is another clear
indication the current is intended to be set once and not
for dimming purposes)

So IMHO both LED pins should have a max_brightness of 1 and
the sysfs API (or internal kernel users) should only be able
to turn them on/off (sysfs brightness 1/0)

The setting of the current should be done through a device
property. Lets call this "ti,ledb-current" with allowed
values of 2, 4, 8, 16. You can then try to get a configured
current for the pin using device-properties from probe()
and only set the current once if the property is their, e.g.
something like this:

probe ()
{
	u32 curr;

	ret = device_property_read_u32(dev, "ti,ledb-current", &curr);
	if (ret == 0) {
		switch (curr) {
		case  2: current = CTRLB_2MA; break;
		case  4: current = CTRLB_4MA; break;
		case  8: current = CTRLB_8MA; break;
		case 16: current = CTRLB_16MA; break;
		default:
			dev_err(dev, "Invalid LEDB curr value: %d\n", curr);
			return -EINVAL;
		}

		regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_CTRLB, curr);
	}


> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*data_brightness = brightness;
> +	return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL,
> +				  TPS68470_ILEDCTL_CTRLB, ledb_current);
> +}
> +
> +static int tps68470_brightness_set(struct led_classdev *led_cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct tps68470_led_data *data;
> +	struct regmap *regmap;
> +	unsigned int mask;
> +	unsigned int value;
> +	int ret;
> +
> +	if (!strncmp(led_cdev->name, "tps68470-ileda", 14)) {
> +		data = container_of(led_cdev, struct tps68470_led_data, leda_cdev);
> +		regmap = data->tps68470_regmap;
> +		data->brightness_a = brightness ? TPS68470_ILEDCTL_ENA : 0;
> +		mask = TPS68470_ILEDCTL_ENA;
> +		value = data->brightness_a;
> +	} else if (!strncmp(led_cdev->name, "tps68470-iledb", 14)) {
> +		data = container_of(led_cdev, struct tps68470_led_data, ledb_cdev);
> +		regmap = data->tps68470_regmap;
> +		mask = TPS68470_ILEDCTL_ENB;
> +		value = brightness ? TPS68470_ILEDCTL_ENB : 0;
> +		/* Set current state for ledb */
> +		ret = set_ledb_current(regmap, &data->brightness_b, brightness);
> +		if (ret)
> +			goto err_exit;
> +	} else
> +		return -EINVAL;
> +
> +	ret = regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, mask, value);
> +
> +err_exit:
> +	return ret;
> +}


> +
> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> +{
> +	struct tps68470_led_data *data = container_of(led_cdev,
> +						      struct tps68470_led_data,
> +						      ledb_cdev);

This container_of only works for led_b not for led_a.

> +
> +	if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
> +		return data->brightness_a;
> +	else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
> +		return data->brightness_b;
> +
> +	return -EINVAL;
> +}

Instead of this strcmp magic, please just use 2 separate
brightness_get functions (thus also solving the container_of
problem above). And please also do the same for brightness_set.

Regards,

Hans


> +
> +static int tps68470_led_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +	struct tps68470_led_data *tps68470_led;
> +
> +	tps68470_led = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_led_data),
> +				    GFP_KERNEL);
> +	if (!tps68470_led)
> +		return -ENOMEM;
> +
> +	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
> +	tps68470_led->leda_cdev.name = "tps68470-ileda";
> +	tps68470_led->leda_cdev.max_brightness = 1;
> +	tps68470_led->leda_cdev.brightness_set_blocking = tps68470_brightness_set;
> +	tps68470_led->leda_cdev.brightness_get = tps68470_brightness_get;
> +	tps68470_led->leda_cdev.dev = &pdev->dev;
> +	tps68470_led->brightness_a = 0;
> +	ret = led_classdev_register(&pdev->dev, &tps68470_led->leda_cdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register LEDA: %d\n", ret);
> +		return ret;
> +	}
> +
> +	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
> +	tps68470_led->ledb_cdev.name = "tps68470-iledb";
> +	tps68470_led->ledb_cdev.max_brightness = 255;
> +	tps68470_led->ledb_cdev.brightness_set_blocking = tps68470_brightness_set;
> +	tps68470_led->ledb_cdev.brightness_get = tps68470_brightness_get;
> +	tps68470_led->ledb_cdev.dev = &pdev->dev;
> +	tps68470_led->brightness_b = 0;
> +	ret = led_classdev_register(&pdev->dev, &tps68470_led->ledb_cdev);
> +	if (ret < 0) {

		You are forgetting to unregister the other LED here.

But instead of adding an unregister here, please just switch to
devm_led_classdev_register() for both LEDs and then... (continued below)

> +		dev_err(&pdev->dev, "Failed to register LEDB: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, tps68470_led);
> +
> +	return ret;
> +}
> +
> +static int tps68470_led_remove(struct platform_device *pdev)
> +{
> +	struct tps68470_led_data *data = platform_get_drvdata(pdev);
> +
> +	led_classdev_unregister(&data->leda_cdev);
> +	led_classdev_unregister(&data->ledb_cdev);
> +
> +	return 0;
> +}

You can remove the tps68470_led_remove() function since the
devm framework now takes care of unregistering on probe-errors
and on driver unbinding (aka remove).

> +
> +static struct platform_driver tps68470_led_driver = {
> +	.driver = {
> +		   .name = "tps68470-led",
> +	},
> +	.probe = tps68470_led_probe,
> +	.remove = tps68470_led_remove,
> +};
> +module_platform_driver(tps68470_led_driver);
> +
> +MODULE_ALIAS("platform:tps68470-led");
> +MODULE_DESCRIPTION("LED driver for TPS68470 PMIC");
> +MODULE_LICENSE("GPL v2");

Regards,

Hans

Re: [RESEND PATCH 2/3] include: mfd: tps68470: Add masks for LEDA and LEDB

[Dan Scally]

Hi Kate - thanks for the set

On 13/02/2023 12:45, Kate Hsuan wrote:
> Add flags for both LEDA(TPS68470_ILEDCTL_ENA), LEDB
> (TPS68470_ILEDCTL_ENB), and current control mask for LEDB
> (TPS68470_ILEDCTL_CTRLB)
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   include/linux/mfd/tps68470.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
> index 7807fa329db0..2d2abb25b944 100644
> --- a/include/linux/mfd/tps68470.h
> +++ b/include/linux/mfd/tps68470.h
> @@ -34,6 +34,7 @@
>   #define TPS68470_REG_SGPO		0x22
>   #define TPS68470_REG_GPDI		0x26
>   #define TPS68470_REG_GPDO		0x27
> +#define TPS68470_REG_ILEDCTL		0x28
>   #define TPS68470_REG_VCMVAL		0x3C
>   #define TPS68470_REG_VAUX1VAL		0x3D
>   #define TPS68470_REG_VAUX2VAL		0x3E
> @@ -94,4 +95,8 @@
>   #define TPS68470_GPIO_MODE_OUT_CMOS	2
>   #define TPS68470_GPIO_MODE_OUT_ODRAIN	3
>   
> +#define TPS68470_ILEDCTL_ENA		BIT(2)
> +#define TPS68470_ILEDCTL_ENB		BIT(6)
> +#define TPS68470_ILEDCTL_CTRLB		GENMASK(5, 4)
> +
>   #endif /* __LINUX_MFD_TPS68470_H */

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

[Dan Scally]

On 22/02/2023 16:53, Hans de Goede wrote:
> Hi Kate,
> 
> On 2/13/23 13:45, 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 4 levels of power status for LEDB, so after setting
>> TPS68470_ILEDCTL_ENB, the current status field (TPS68470_ILEDCTL_CTRLB)
>> should also be set according to the brightness value from user space.
>> 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 | 170 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 183 insertions(+)
>>   create mode 100644 drivers/leds/leds-tps68470.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 499d0f215a8b..453404cb1329 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -846,6 +846,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.
>> +
>> +	  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 4fd2f92cd198..0a2ec01e27d9 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -86,6 +86,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..6243e7a4a718
>> --- /dev/null
>> +++ b/drivers/leds/leds-tps68470.c
>> @@ -0,0 +1,170 @@
>> +// 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>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/leds.h>
>> +
>> +struct tps68470_led_data {
>> +	struct regmap *tps68470_regmap;
>> +	unsigned int brightness_a;
>> +	unsigned int brightness_b;
>> +	struct led_classdev leda_cdev;
>> +	struct led_classdev ledb_cdev;
>> +};
>> +
>> +enum ctrlb_current {
>> +	CTRLB_2MA	= 0,
>> +	CTRLB_4MA	= 1,
>> +	CTRLB_8MA	= 2,
>> +	CTRLB_16MA	= 3,
>> +};
>> +
>> +static int set_ledb_current(struct regmap *regmap,
>> +			    unsigned int *data_brightness,
>> +			    enum led_brightness brightness)
>> +{
>> +	unsigned int ledb_current;
>> +
>> +	switch (brightness) {
>> +	case LED_HALF:
>> +		ledb_current = CTRLB_8MA;
>> +		break;
>> +	case LED_FULL:
>> +		ledb_current = CTRLB_16MA;
>> +		break;
> 
> LED_FULL is 255, now a days the LED class supports custom
> brightness ranges, so you could just set max_brightness to 4
> and then use brightness 1-4 to map to the 4 values.
> 
>> +	case LED_ON:
>> +		ledb_current = CTRLB_4MA;
>> +		break;
>> +	case LED_OFF:
>> +		ledb_current = CTRLB_2MA;
>> +		break;
> 
> This makes no sense, when brightness == LED_OFF you
> disable the LEDB output, so the configured current does
> not matter.
> 
> But I believe that mapping the current to brightness is
> not how this should be done / modeled. With the separate
> on/off toggle this is clearly not intended to allow dimming
> a LED. This is configurable to allow adjusting for different
> notification LED types, but the intention is still for LEDB
> to be used as a simple on/off notification LED.
> 
> (The lack of e.g. actual PWM / fine grained current control
> vs doubling the current each step to me is another clear
> indication the current is intended to be set once and not
> for dimming purposes)
> 
> So IMHO both LED pins should have a max_brightness of 1 and
> the sysfs API (or internal kernel users) should only be able
> to turn them on/off (sysfs brightness 1/0)
> 
> The setting of the current should be done through a device
> property. Lets call this "ti,ledb-current" with allowed
> values of 2, 4, 8, 16. You can then try to get a configured
> current for the pin using device-properties from probe()
> and only set the current once if the property is their, e.g.
> something like this:
> 
> probe ()
> {
> 	u32 curr;
> 
> 	ret = device_property_read_u32(dev, "ti,ledb-current", &curr);
> 	if (ret == 0) {
> 		switch (curr) {
> 		case  2: current = CTRLB_2MA; break;
> 		case  4: current = CTRLB_4MA; break;
> 		case  8: current = CTRLB_8MA; break;
> 		case 16: current = CTRLB_16MA; break;
> 		default:
> 			dev_err(dev, "Invalid LEDB curr value: %d\n", curr);
> 			return -EINVAL;
> 		}
> 
> 		regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_CTRLB, curr);
> 	}
> 
> 

fwiw I agree with Hans on the above

>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	*data_brightness = brightness;
>> +	return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL,
>> +				  TPS68470_ILEDCTL_CTRLB, ledb_current);
>> +}
>> +
>> +static int tps68470_brightness_set(struct led_classdev *led_cdev,
>> +				   enum led_brightness brightness)
>> +{
>> +	struct tps68470_led_data *data;
>> +	struct regmap *regmap;
>> +	unsigned int mask;
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	if (!strncmp(led_cdev->name, "tps68470-ileda", 14)) {
>> +		data = container_of(led_cdev, struct tps68470_led_data, leda_cdev);
>> +		regmap = data->tps68470_regmap;
>> +		data->brightness_a = brightness ? TPS68470_ILEDCTL_ENA : 0;
>> +		mask = TPS68470_ILEDCTL_ENA;
>> +		value = data->brightness_a;
>> +	} else if (!strncmp(led_cdev->name, "tps68470-iledb", 14)) {
>> +		data = container_of(led_cdev, struct tps68470_led_data, ledb_cdev);
>> +		regmap = data->tps68470_regmap;
>> +		mask = TPS68470_ILEDCTL_ENB;
>> +		value = brightness ? TPS68470_ILEDCTL_ENB : 0;
>> +		/* Set current state for ledb */
>> +		ret = set_ledb_current(regmap, &data->brightness_b, brightness);
>> +		if (ret)
>> +			goto err_exit;
>> +	} else
>> +		return -EINVAL;
>> +
>> +	ret = regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, mask, value);
>> +
>> +err_exit:
>> +	return ret;
>> +}
> 
> 
>> +
>> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
>> +{
>> +	struct tps68470_led_data *data = container_of(led_cdev,
>> +						      struct tps68470_led_data,
>> +						      ledb_cdev);
> 
> This container_of only works for led_b not for led_a.
> 
>> +
>> +	if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
>> +		return data->brightness_a;
>> +	else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
>> +		return data->brightness_b;
>> +
>> +	return -EINVAL;
>> +}
> 
> Instead of this strcmp magic, please just use 2 separate
> brightness_get functions (thus also solving the container_of
> problem above). And please also do the same for brightness_set.

I don't mind the single function so much but I don't particularly like the strcmp. I'm actually 
working on this at the moment too trying (but so far mostly failing) to get the WLED that drives the 
Surface Go's IR LED working properly (I can drive it...for a maximum of 13 seconds); I had modeled 
the problem as an array of structs for the LEDs and reference them with IDs:

#define lcdev_to_led(lcdev) \
	container_of(lcdev, 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_WLED,
	TPS68470_NUM_LEDS
};

static const char *tps68470_led_names[] = {
	[TPS68470_ILED_A] = "tps68470-iled_a",
	[TPS68470_ILED_B] = "tps68470-iled_b",
	[TPS68470_WLED] = "tps68470-wled",
};

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];
};

int tps68470_led_brightness_set(...)
{
	struct tps68470_led *led = lcdev_to_led(lcdev);
	struct tps68470_device *tps68470 = led_to_tps68470(led, led->index);

	switch (led->led_id) {
	case TPS68470_ILED_A:
		return regmap_update_bits(...);
	case TPS68470_ILED_B:
		...

	}
}

int tps68470_leds_probe(...)
{
	struct tps68470_led *led;

	...

	for (i = 0; i < TPS68470_NUM_LEDS; i++) {
		led = &tps68470->leds[i];

		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_set_blocking = tps68470_led_brightness_set;

		ret = devm_led_classdev_register(tps68470->dev, lcdev);
		if (ret)
			return dev_err_probe(tps68470->dev, ret,
					"error registering led\n");
	}
}

Personally I think that's better than having 3 functions to do it.

Regardless of how it ends up being done; I think you need the LED_FUNCTION_INDICATOR part in 
lcdev->name to match the "devicename:color:function" that the LED subsystem seems to want.

> 
> Regards,
> 
> Hans
> 
> 
>> +
>> +static int tps68470_led_probe(struct platform_device *pdev)
>> +{
>> +	int ret = 0;
>> +	struct tps68470_led_data *tps68470_led;
>> +
>> +	tps68470_led = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_led_data),
>> +				    GFP_KERNEL);
>> +	if (!tps68470_led)
>> +		return -ENOMEM;
>> +
>> +	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
>> +	tps68470_led->leda_cdev.name = "tps68470-ileda";
>> +	tps68470_led->leda_cdev.max_brightness = 1;
>> +	tps68470_led->leda_cdev.brightness_set_blocking = tps68470_brightness_set;
>> +	tps68470_led->leda_cdev.brightness_get = tps68470_brightness_get;
>> +	tps68470_led->leda_cdev.dev = &pdev->dev;
>> +	tps68470_led->brightness_a = 0;
>> +	ret = led_classdev_register(&pdev->dev, &tps68470_led->leda_cdev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Failed to register LEDA: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	tps68470_led->tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
>> +	tps68470_led->ledb_cdev.name = "tps68470-iledb";
>> +	tps68470_led->ledb_cdev.max_brightness = 255;
>> +	tps68470_led->ledb_cdev.brightness_set_blocking = tps68470_brightness_set;
>> +	tps68470_led->ledb_cdev.brightness_get = tps68470_brightness_get;
>> +	tps68470_led->ledb_cdev.dev = &pdev->dev;
>> +	tps68470_led->brightness_b = 0;
>> +	ret = led_classdev_register(&pdev->dev, &tps68470_led->ledb_cdev);
>> +	if (ret < 0) {
> 
> 		You are forgetting to unregister the other LED here.
> 
> But instead of adding an unregister here, please just switch to
> devm_led_classdev_register() for both LEDs and then... (continued below)
> 
>> +		dev_err(&pdev->dev, "Failed to register LEDB: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, tps68470_led);
>> +
>> +	return ret;
>> +}
>> +
>> +static int tps68470_led_remove(struct platform_device *pdev)
>> +{
>> +	struct tps68470_led_data *data = platform_get_drvdata(pdev);
>> +
>> +	led_classdev_unregister(&data->leda_cdev);
>> +	led_classdev_unregister(&data->ledb_cdev);
>> +
>> +	return 0;
>> +}
> 
> You can remove the tps68470_led_remove() function since the
> devm framework now takes care of unregistering on probe-errors
> and on driver unbinding (aka remove).
> 
>> +
>> +static struct platform_driver tps68470_led_driver = {
>> +	.driver = {
>> +		   .name = "tps68470-led",
>> +	},
>> +	.probe = tps68470_led_probe,
>> +	.remove = tps68470_led_remove,
>> +};
>> +module_platform_driver(tps68470_led_driver);
>> +
>> +MODULE_ALIAS("platform:tps68470-led");
>> +MODULE_DESCRIPTION("LED driver for TPS68470 PMIC");
>> +MODULE_LICENSE("GPL v2");
> 
> Regards,
> 
> Hans
> 

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

[Hans de Goede]

Hi,

On 2/24/23 16:14, Dan Scally wrote:
> On 22/02/2023 16:53, Hans de Goede wrote:

<snip>

>>> +
>>> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
>>> +{
>>> +    struct tps68470_led_data *data = container_of(led_cdev,
>>> +                              struct tps68470_led_data,
>>> +                              ledb_cdev);
>>
>> This container_of only works for led_b not for led_a.
>>
>>> +
>>> +    if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
>>> +        return data->brightness_a;
>>> +    else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
>>> +        return data->brightness_b;
>>> +
>>> +    return -EINVAL;
>>> +}
>>
>> Instead of this strcmp magic, please just use 2 separate
>> brightness_get functions (thus also solving the container_of
>> problem above). And please also do the same for brightness_set.
> 
> I don't mind the single function so much but I don't particularly like the strcmp. I'm actually working on this at the moment too trying (but so far mostly failing) to get the WLED that drives the Surface Go's IR LED working properly (I can drive it...for a maximum of 13 seconds); I had modeled the problem as an array of structs for the LEDs and reference them with IDs:
> 
> #define lcdev_to_led(lcdev) \
>     container_of(lcdev, 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_WLED,
>     TPS68470_NUM_LEDS
> };
> 
> static const char *tps68470_led_names[] = {
>     [TPS68470_ILED_A] = "tps68470-iled_a",
>     [TPS68470_ILED_B] = "tps68470-iled_b",
>     [TPS68470_WLED] = "tps68470-wled",
> };
> 
> 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];
> };
> 
> int tps68470_led_brightness_set(...)
> {
>     struct tps68470_led *led = lcdev_to_led(lcdev);
>     struct tps68470_device *tps68470 = led_to_tps68470(led, led->index);

I assume led->index should be led->led_id here ?

> 
>     switch (led->led_id) {
>     case TPS68470_ILED_A:
>         return regmap_update_bits(...);
>     case TPS68470_ILED_B:
>         ...
> 
>     }

But since the indices into the register are not simple a function of
led->led_id, you still need this switch-case here and then a separate
implementation for each LED.

At which point IMHO just having a single set / get function per LED
is much simpler then adding the complications with the struct wrapping
struct led_class_dev to add an index to it.

Anyways there is an easy solution here: Kate you get to choose between
1 set + get function per LED or Dan's solution, but please drop
the strcmp() calls since neither Dan nor I like those.

<snip>

> Regardless of how it ends up being done; I think you need the LED_FUNCTION_INDICATOR part in lcdev->name to match the "devicename:color:function" that the LED subsystem seems to want.

Agreed, Kate please switch to this, e.g.:

tps68470_led->leda_cdev.name = "tps68470::" LED_FUNCTION_INDICATOR;

LED names should always be in the format of "devicename:color:function"
I missed you were not using that before. And since we don't know the
color we just leave it empty (this is allowed).

Note LED_FUNCTION_INDICATOR is defined in include/dt-bindings/leds/common.h .

Regards,

Hans

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

[Kate Hsuan]

On Wed, Mar 1, 2023 at 8:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/24/23 16:14, Dan Scally wrote:
> > On 22/02/2023 16:53, Hans de Goede wrote:
>
> <snip>
>
> >>> +
> >>> +static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> >>> +{
> >>> +    struct tps68470_led_data *data = container_of(led_cdev,
> >>> +                              struct tps68470_led_data,
> >>> +                              ledb_cdev);
> >>
> >> This container_of only works for led_b not for led_a.
> >>
> >>> +
> >>> +    if (!strncmp(led_cdev->name, "tps68470-ileda", 14))
> >>> +        return data->brightness_a;
> >>> +    else if (!strncmp(led_cdev->name, "tps68470-iledb", 14))
> >>> +        return data->brightness_b;
> >>> +
> >>> +    return -EINVAL;
> >>> +}
> >>
> >> Instead of this strcmp magic, please just use 2 separate
> >> brightness_get functions (thus also solving the container_of
> >> problem above). And please also do the same for brightness_set.
> >
> > I don't mind the single function so much but I don't particularly like the strcmp. I'm actually working on this at the moment too trying (but so far mostly failing) to get the WLED that drives the Surface Go's IR LED working properly (I can drive it...for a maximum of 13 seconds); I had modeled the problem as an array of structs for the LEDs and reference them with IDs:
> >
> > #define lcdev_to_led(lcdev) \
> >     container_of(lcdev, 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_WLED,
> >     TPS68470_NUM_LEDS
> > };
> >
> > static const char *tps68470_led_names[] = {
> >     [TPS68470_ILED_A] = "tps68470-iled_a",
> >     [TPS68470_ILED_B] = "tps68470-iled_b",
> >     [TPS68470_WLED] = "tps68470-wled",
> > };
> >
> > 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];
> > };
> >
> > int tps68470_led_brightness_set(...)
> > {
> >     struct tps68470_led *led = lcdev_to_led(lcdev);
> >     struct tps68470_device *tps68470 = led_to_tps68470(led, led->index);
>
> I assume led->index should be led->led_id here ?
>
> >
> >     switch (led->led_id) {
> >     case TPS68470_ILED_A:
> >         return regmap_update_bits(...);
> >     case TPS68470_ILED_B:
> >         ...
> >
> >     }
>
> But since the indices into the register are not simple a function of
> led->led_id, you still need this switch-case here and then a separate
> implementation for each LED.
>
> At which point IMHO just having a single set / get function per LED
> is much simpler then adding the complications with the struct wrapping
> struct led_class_dev to add an index to it.
>
> Anyways there is an easy solution here: Kate you get to choose between
> 1 set + get function per LED or Dan's solution, but please drop
> the strcmp() calls since neither Dan nor I like those.

Thank you for the comments.
Okay, let me think between both solutions and I'll remove strcmp() for sure.

>
> <snip>
>
> > Regardless of how it ends up being done; I think you need the LED_FUNCTION_INDICATOR part in lcdev->name to match the "devicename:color:function" that the LED subsystem seems to want.
>
> Agreed, Kate please switch to this, e.g.:
>
> tps68470_led->leda_cdev.name = "tps68470::" LED_FUNCTION_INDICATOR;
>
> LED names should always be in the format of "devicename:color:function"
> I missed you were not using that before. And since we don't know the
> color we just leave it empty (this is allowed).

OK. I have checked the document for the naming format. I'll correct this.

>
> Note LED_FUNCTION_INDICATOR is defined in include/dt-bindings/leds/common.h .

Thank you for noting this.

>
> Regards,
>
> Hans
>
>
>
>


-- 
BR,
Kate