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

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

[Kate Hsuan]

The v3 patch includes:
1. Move the LEDB current setting to a function and also assume the value
of the current setting is based on tps68470 datasheet.
2. Alphabetical order.
3. Return -ENOMEM when memory allocation returns an error.


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                  | 185 ++++++++++++++++++
 drivers/platform/x86/intel/int3472/tps68470.c |   5 +-
 include/linux/mfd/tps68470.h                  |   5 +
 5 files changed, 206 insertions(+), 2 deletions(-)
 create mode 100644 drivers/leds/leds-tps68470.c

-- 
2.39.2

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

[Kate Hsuan]

Add MFD cell for tps68470-led.

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 5b8d1a9620a5..82ef022f8916 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" },
@@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		cells[1].name = "tps68470-regulator";
 		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[2].name = "tps68470-led";
+		cells[3].name = "tps68470-gpio";
 
 		for (i = 0; i < board_data->n_gpiod_lookups; i++)
 			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
-- 
2.39.2

[PATCH v3 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)

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
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.2

[PATCH v3 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 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 | 185 +++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/leds/leds-tps68470.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2
+	  indicator LEDs and 2 flash LEDs.
+
+	  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..515a69953e73 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
 obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
+obj-$(CONFIG_LEDS_TPS68470)		+= leds-tps68470.o
 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
diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
new file mode 100644
index 000000000000..35aeb5db89c8
--- /dev/null
+++ b/drivers/leds/leds-tps68470.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LED driver for TPS68470 PMIC
+ *
+ * Copyright (C) 2023 Red Hat
+ *
+ * Authors:
+ *	Kate Hsuan <hpa@redhat.com>
+ */
+
+#include <linux/leds.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+
+#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;
+
+	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)
+		return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n");
+
+	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;
+}
+
+
+static int tps68470_ledb_current_init(struct platform_device *pdev,
+				      struct tps68470_device *tps68470)
+{
+	int ret = 0;
+	unsigned int curr;
+
+	/* configure LEDB current if the properties can be got */
+	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
+		if (curr > CTRLB_16MA) {
+			dev_err(&pdev->dev,
+				"Invalid LEDB current value: %d\n",
+				curr);
+			return -EINVAL;
+		}
+		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL,
+					 TPS68470_ILEDCTL_CTRLB, curr);
+	}
+	return ret;
+}
+
+static int tps68470_leds_probe(struct platform_device *pdev)
+{
+	int i = 0;
+	int ret = 0;
+	struct tps68470_device *tps68470;
+	struct tps68470_led *led;
+	struct led_classdev *lcdev;
+
+	tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device),
+				GFP_KERNEL);
+	if (!tps68470)
+		return -ENOMEM;
+
+	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;
+		}
+
+		if (i == TPS68470_ILED_B) {
+			ret = tps68470_ledb_current_init(pdev, tps68470);
+			if (ret)
+				goto err_exit;
+		}
+	}
+
+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");
-- 
2.39.2

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

[Hans de Goede]

Hi,

On 3/21/23 16:37, Kate Hsuan wrote:
> Add MFD cell for tps68470-led.
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

Thanks, patch looks good to me:

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

Regards,

Hans


> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 5b8d1a9620a5..82ef022f8916 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" },
> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		cells[1].name = "tps68470-regulator";
>  		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[2].name = "tps68470-led";
> +		cells[3].name = "tps68470-gpio";
>  
>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
>  			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);

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

[Hans de Goede]

Hi,

On 3/21/23 16:37, 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>

Thanks, patch looks good to me:

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

Regards,

Hans


> ---
>  drivers/leds/Kconfig         |  12 +++
>  drivers/leds/Makefile        |   1 +
>  drivers/leds/leds-tps68470.c | 185 +++++++++++++++++++++++++++++++++++
>  3 files changed, 198 insertions(+)
>  create mode 100644 drivers/leds/leds-tps68470.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2
> +	  indicator LEDs and 2 flash LEDs.
> +
> +	  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..515a69953e73 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
>  obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
>  obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
>  obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
> +obj-$(CONFIG_LEDS_TPS68470)		+= leds-tps68470.o
>  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
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> new file mode 100644
> index 000000000000..35aeb5db89c8
> --- /dev/null
> +++ b/drivers/leds/leds-tps68470.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LED driver for TPS68470 PMIC
> + *
> + * Copyright (C) 2023 Red Hat
> + *
> + * Authors:
> + *	Kate Hsuan <hpa@redhat.com>
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +
> +#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;
> +
> +	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)
> +		return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n");
> +
> +	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;
> +}
> +
> +
> +static int tps68470_ledb_current_init(struct platform_device *pdev,
> +				      struct tps68470_device *tps68470)
> +{
> +	int ret = 0;
> +	unsigned int curr;
> +
> +	/* configure LEDB current if the properties can be got */
> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
> +		if (curr > CTRLB_16MA) {
> +			dev_err(&pdev->dev,
> +				"Invalid LEDB current value: %d\n",
> +				curr);
> +			return -EINVAL;
> +		}
> +		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL,
> +					 TPS68470_ILEDCTL_CTRLB, curr);
> +	}
> +	return ret;
> +}
> +
> +static int tps68470_leds_probe(struct platform_device *pdev)
> +{
> +	int i = 0;
> +	int ret = 0;
> +	struct tps68470_device *tps68470;
> +	struct tps68470_led *led;
> +	struct led_classdev *lcdev;
> +
> +	tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device),
> +				GFP_KERNEL);
> +	if (!tps68470)
> +		return -ENOMEM;
> +
> +	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;
> +		}
> +
> +		if (i == TPS68470_ILED_B) {
> +			ret = tps68470_ledb_current_init(pdev, tps68470);
> +			if (ret)
> +				goto err_exit;
> +		}
> +	}
> +
> +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");

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

[Dan Scally]

Hi Kate

On 21/03/2023 15:37, 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>
> ---

This looks good to me now: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>   drivers/leds/Kconfig         |  12 +++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-tps68470.c | 185 +++++++++++++++++++++++++++++++++++
>   3 files changed, 198 insertions(+)
>   create mode 100644 drivers/leds/leds-tps68470.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9dbce09eabac..ab9073b2cfef 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 provides two LED controllers, with the ability to drive 2
> +	  indicator LEDs and 2 flash LEDs.
> +
> +	  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..515a69953e73 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
>   obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
>   obj-$(CONFIG_LEDS_TLC591XX)		+= leds-tlc591xx.o
>   obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
> +obj-$(CONFIG_LEDS_TPS68470)		+= leds-tps68470.o
>   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
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> new file mode 100644
> index 000000000000..35aeb5db89c8
> --- /dev/null
> +++ b/drivers/leds/leds-tps68470.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LED driver for TPS68470 PMIC
> + *
> + * Copyright (C) 2023 Red Hat
> + *
> + * Authors:
> + *	Kate Hsuan <hpa@redhat.com>
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +
> +#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;
> +
> +	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)
> +		return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n");
> +
> +	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;
> +}
> +
> +
> +static int tps68470_ledb_current_init(struct platform_device *pdev,
> +				      struct tps68470_device *tps68470)
> +{
> +	int ret = 0;
> +	unsigned int curr;
> +
> +	/* configure LEDB current if the properties can be got */
> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
> +		if (curr > CTRLB_16MA) {
> +			dev_err(&pdev->dev,
> +				"Invalid LEDB current value: %d\n",
> +				curr);
> +			return -EINVAL;
> +		}
> +		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_ILEDCTL,
> +					 TPS68470_ILEDCTL_CTRLB, curr);
> +	}
> +	return ret;
> +}
> +
> +static int tps68470_leds_probe(struct platform_device *pdev)
> +{
> +	int i = 0;
> +	int ret = 0;
> +	struct tps68470_device *tps68470;
> +	struct tps68470_led *led;
> +	struct led_classdev *lcdev;
> +
> +	tps68470 = devm_kzalloc(&pdev->dev, sizeof(struct tps68470_device),
> +				GFP_KERNEL);
> +	if (!tps68470)
> +		return -ENOMEM;
> +
> +	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;
> +		}
> +
> +		if (i == TPS68470_ILED_B) {
> +			ret = tps68470_ledb_current_init(pdev, tps68470);
> +			if (ret)
> +				goto err_exit;
> +		}
> +	}
> +
> +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");

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

Hi!

> 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).

If the LED can have four different currents, should it have 4
brightness levels?

> +++ 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 provides two LED controllers, with the ability to drive 2
> +	  indicator LEDs and 2 flash LEDs.
> +
> +	  To compile this driver as a module, choose M and it will be
> +	  called leds-tps68470

. at end of line.

> +static const char *tps68470_led_names[] = {
> +	[TPS68470_ILED_A] = "tps68470-iled_a",
> +	[TPS68470_ILED_B] = "tps68470-iled_b",

No. See Documentation/leds/well-known-leds.txt . We want the LEDs to
be named after their function.

> +static int tps68470_ledb_current_init(struct platform_device *pdev,
> +				      struct tps68470_device *tps68470)
> +{
> +	int ret = 0;
> +	unsigned int curr;
> +
> +	/* configure LEDB current if the properties can be got */

english?

> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
> +		if (curr > CTRLB_16MA) {

We'll need device tree binding documentation, right?

Best regards,
								Pavel

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Hans de Goede]

Hi Pavel,

On 3/23/23 12:15, Pavel Machek wrote:
> Hi!
> 
>> 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).
> 
> If the LED can have four different currents, should it have 4
> brightness levels?

No this was already discussed with an earlier version. This is in
indicator LED output. The current setting is a one time boot configure
thing after which the indicator LED is either on or off.

> 
>> +++ 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 provides two LED controllers, with the ability to drive 2
>> +	  indicator LEDs and 2 flash LEDs.
>> +
>> +	  To compile this driver as a module, choose M and it will be
>> +	  called leds-tps68470
> 
> . at end of line.
> 
>> +static const char *tps68470_led_names[] = {
>> +	[TPS68470_ILED_A] = "tps68470-iled_a",
>> +	[TPS68470_ILED_B] = "tps68470-iled_b",
> 
> No. See Documentation/leds/well-known-leds.txt . We want the LEDs to
> be named after their function.
> 
>> +static int tps68470_ledb_current_init(struct platform_device *pdev,
>> +				      struct tps68470_device *tps68470)
>> +{
>> +	int ret = 0;
>> +	unsigned int curr;
>> +
>> +	/* configure LEDB current if the properties can be got */
> 
> english?
> 
>> +	if (!device_property_read_u32(&pdev->dev, "ti,ledb-current", &curr)) {
>> +		if (curr > CTRLB_16MA) {
> 
> We'll need device tree binding documentation, right?

For now this PMIC is only used for the camera in some x86/acpi designs,
so no we don't need dt binding documentation (the dt binding maintainers
have explicitly asked to not add dt binding documentation for things
not actually used with dt).

Or I guess we should simply add this to the platform_data which
one of Daniel's later patches introduces and drop the initializing
of the LEDB current setting from the initial driver addition.

I think that that (moving this to the later added platform_data)
would be best. Since now after Daniel's patches we have a mix
of platform_data + the 1 device-property for this.

Daniel, what do you think about moving the LEDB current setting to your
"[PATCH 2/8] leds: tps68470: Init LED registers using platform data"
patch ?

Regards,

Hans

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

On Thu 2023-03-23 12:24:05, Hans de Goede wrote:
> Hi Pavel,
> 
> On 3/23/23 12:15, Pavel Machek wrote:
> > Hi!
> > 
> >> 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).
> > 
> > If the LED can have four different currents, should it have 4
> > brightness levels?
> 
> No this was already discussed with an earlier version. This is in
> indicator LED output. The current setting is a one time boot configure
> thing after which the indicator LED is either on or off.

Current levels are exponential in that driver. That will result in
rather nice four level. Surely LED does not care if you set it during
boot or later?

BR,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Hans de Goede]

Hi,

On 3/23/23 12:26, Pavel Machek wrote:
> On Thu 2023-03-23 12:24:05, Hans de Goede wrote:
>> Hi Pavel,
>>
>> On 3/23/23 12:15, Pavel Machek wrote:
>>> Hi!
>>>
>>>> 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).
>>>
>>> If the LED can have four different currents, should it have 4
>>> brightness levels?
>>
>> No this was already discussed with an earlier version. This is in
>> indicator LED output. The current setting is a one time boot configure
>> thing after which the indicator LED is either on or off.
> 
> Current levels are exponential in that driver. That will result in
> rather nice four level. Surely LED does not care if you set it during
> boot or later?

Well for one there is no guarantee the LED can continuously handle
the maximum configurable LED current and as you rightly point out
elsewhere in the thread we don't want to be blowing up hw.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 3/23/23 12:29, Hans de Goede wrote:
> Hi,
> 
> On 3/23/23 12:26, Pavel Machek wrote:
>> On Thu 2023-03-23 12:24:05, Hans de Goede wrote:
>>> Hi Pavel,
>>>
>>> On 3/23/23 12:15, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>> 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).
>>>>
>>>> If the LED can have four different currents, should it have 4
>>>> brightness levels?
>>>
>>> No this was already discussed with an earlier version. This is in
>>> indicator LED output. The current setting is a one time boot configure
>>> thing after which the indicator LED is either on or off.
>>
>> Current levels are exponential in that driver. That will result in
>> rather nice four level. Surely LED does not care if you set it during
>> boot or later?
> 
> Well for one there is no guarantee the LED can continuously handle
> the maximum configurable LED current and as you rightly point out
> elsewhere in the thread we don't want to be blowing up hw.

Also this LED output as mentioned is especially intended for use
with on/off triggers and those don't use/set different brightness
levels. So it really is best to set the LED current once to
the current which we want when the LED is on to indicate whatever
it is intended to be indicating.

Regards,

Hans

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Thu 2023-03-23 12:29:29, Hans de Goede wrote:
> Hi,
> 
> On 3/23/23 12:26, Pavel Machek wrote:
> > On Thu 2023-03-23 12:24:05, Hans de Goede wrote:
> >> Hi Pavel,
> >>
> >> On 3/23/23 12:15, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>> 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).
> >>>
> >>> If the LED can have four different currents, should it have 4
> >>> brightness levels?
> >>
> >> No this was already discussed with an earlier version. This is in
> >> indicator LED output. The current setting is a one time boot configure
> >> thing after which the indicator LED is either on or off.
> > 
> > Current levels are exponential in that driver. That will result in
> > rather nice four level. Surely LED does not care if you set it during
> > boot or later?
> 
> Well for one there is no guarantee the LED can continuously handle
> the maximum configurable LED current and as you rightly point out
> elsewhere in the thread we don't want to be blowing up hw.

hw can support 16mA -> you expose 0, 2mA, 4mA, 8mA, 16mA levels.

hw can support 4mA -> you expose 0, 2mA, 4mA.

Triggers will work with these.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Lee Jones]

On Tue, 21 Mar 2023, Kate Hsuan wrote:

> Add MFD cell for tps68470-led.
>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 5b8d1a9620a5..82ef022f8916 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" },
> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		cells[1].name = "tps68470-regulator";
>  		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[2].name = "tps68470-led";
> +		cells[3].name = "tps68470-gpio";

The question is, why is the MFD API being used out side of drivers/mfd?

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

--
Lee Jones [李琼斯]

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

[Hans de Goede]

Hi,

On 3/23/23 13:23, Lee Jones wrote:
> On Tue, 21 Mar 2023, Kate Hsuan wrote:
> 
>> Add MFD cell for tps68470-led.
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>> ---
>>  drivers/platform/x86/intel/int3472/tps68470.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
>> index 5b8d1a9620a5..82ef022f8916 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" },
>> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>>  		cells[1].name = "tps68470-regulator";
>>  		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[2].name = "tps68470-led";
>> +		cells[3].name = "tps68470-gpio";
> 
> The question is, why is the MFD API being used out side of drivers/mfd?

Because Intel made a big mess about how they describe camera sensors + the matching clks / regulators / GPIOs and the optional PMIC in ACPI.

The drivers/platform/x86/intel/int3472/ code untangles this mess and in some cases it instantiates MFD cells (with a whole bunch of derived platform_data per cell) for a TPS68470 PMIC.

And sometimes while binding to an INT3472 ACPI device-node it does not instantiate any MFD cells at all since the INT3472 ACPI device-node does not always describe such a PMIC.

Oh and also depending on of the ACPI tables are targetting ChromeOS or Windows a different set of MFD cells needs to be instantiated. On ChromeOS most of the PMIC poking is done through ACPI through a ChomeOS specific custom ACPI OpRegion, so there there are only cells for GPIO and a driver providing the OpRegion are created.

So lots of ugly x86 platform specific handling, ACPI parsing, etc. which is why this landed under drivers/platform/x86/ . IIRC you were even involved in the original merge since there once was a much simpler MFD driver under driver/mfd which only supported the ChromeOS setup.

(but my memory may be deceiving me here).

Regards,

Hans





> 
>>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
>>  			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
>> --
>> 2.39.2
>>
> 
> --
> Lee Jones [李琼斯]
> 

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

[Hans de Goede]

Hi,

On 3/23/23 12:36, Pavel Machek wrote:
> On Thu 2023-03-23 12:29:29, Hans de Goede wrote:
>> Hi,
>>
>> On 3/23/23 12:26, Pavel Machek wrote:
>>> On Thu 2023-03-23 12:24:05, Hans de Goede wrote:
>>>> Hi Pavel,
>>>>
>>>> On 3/23/23 12:15, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>> 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).
>>>>>
>>>>> If the LED can have four different currents, should it have 4
>>>>> brightness levels?
>>>>
>>>> No this was already discussed with an earlier version. This is in
>>>> indicator LED output. The current setting is a one time boot configure
>>>> thing after which the indicator LED is either on or off.
>>>
>>> Current levels are exponential in that driver. That will result in
>>> rather nice four level. Surely LED does not care if you set it during
>>> boot or later?
>>
>> Well for one there is no guarantee the LED can continuously handle
>> the maximum configurable LED current and as you rightly point out
>> elsewhere in the thread we don't want to be blowing up hw.
> 
> hw can support 16mA -> you expose 0, 2mA, 4mA, 8mA, 16mA levels.
> 
> hw can support 4mA -> you expose 0, 2mA, 4mA.

This is just not how this hw is intended to be used. If you look
at the registers there are on/off bits for LEDA and LEDB in a
single register and then there is a current-limit only for LEDB,
where as LEDA has a fixed current.

So clearly the intention here is on/off use with a fixed current
and dimming something like a privacy-LED on a laptop camera
makes no sense and will never be used since we disable userspace
access of the LED when it is used as a privacy-LED.

Regards,

Hans

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

[Lee Jones]

On Thu, 23 Mar 2023, Hans de Goede wrote:

> Hi,
>
> On 3/23/23 13:23, Lee Jones wrote:
> > On Tue, 21 Mar 2023, Kate Hsuan wrote:
> >
> >> Add MFD cell for tps68470-led.
> >>
> >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> >> ---
> >>  drivers/platform/x86/intel/int3472/tps68470.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> >> index 5b8d1a9620a5..82ef022f8916 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" },
> >> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
> >>  		cells[1].name = "tps68470-regulator";
> >>  		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[2].name = "tps68470-led";
> >> +		cells[3].name = "tps68470-gpio";
> >
> > The question is, why is the MFD API being used out side of drivers/mfd?
>
> Because Intel made a big mess about how they describe camera sensors + the matching clks / regulators / GPIOs and the optional PMIC in ACPI.
>
> The drivers/platform/x86/intel/int3472/ code untangles this mess and in some cases it instantiates MFD cells (with a whole bunch of derived platform_data per cell) for a TPS68470 PMIC.
>
> And sometimes while binding to an INT3472 ACPI device-node it does not instantiate any MFD cells at all since the INT3472 ACPI device-node does not always describe such a PMIC.
>
> Oh and also depending on of the ACPI tables are targetting ChromeOS or Windows a different set of MFD cells needs to be instantiated. On ChromeOS most of the PMIC poking is done through ACPI through a ChomeOS specific custom ACPI OpRegion, so there there are only cells for GPIO and a driver providing the OpRegion are created.
>
> So lots of ugly x86 platform specific handling, ACPI parsing, etc. which is why this landed under drivers/platform/x86/ . IIRC you were even involved in the original merge since there once was a much simpler MFD driver under driver/mfd which only supported the ChromeOS setup.
>
> (but my memory may be deceiving me here).

Right, I guess we've both slept since then!

My normal request is that MFD handling should be in drivers/mfd.
Anything else can be farmed out to the various functional subsystems and
drivers/platform.

--
Lee Jones [李琼斯]