[PATCH 0/8] Add WLED support to TPS68470 LED driver

[PATCH 0/8] Add WLED support to TPS68470 LED driver

[Daniel Scally]

This series relies on the recent "leds: tps68470: LED driver for TPS68470" set
from Kate Hsuan [1]

The TPS68470 provides two additional LED outputs on top of the indicator LEDs.
Add support for those to the driver. The configuration of the chip is drawn from
platform data which is expected to be passed to the driver. Additionally update
the int3472-tps68470 driver to register led lookups from platform data so that
the right LED is driven for each sensor, and finally define those lookups for
the Microsoft Surface Go line.

Kate, Hans, this is the changes I made on top of the tps68470-led series to
enable the IR LED on my Go2 (plus one additional patch to media). #5 could
probably just be squashed into the other series though. The last two patches
cover how I think the LED lookup should work - I unfortunately can't see an
automatic way to guarantee the right LED goes to the right sensor.

Thanks
Dan

[1] https://lore.kernel.org/platform-driver-x86/20230321153718.1355511-1-hpa@redhat.com/T/

Daniel Scally (8):
  platform/x86: int3472: Add platform data for LEDs
  platform/x86: int3472: Init LED registers using platform data
  platform/x86: int3472: Add TPS68470 LED Board Data
  platform/x86: int3472: Add tps68470-led as clock consumer
  leds: tps68470: Refactor tps68470_brightness_get()
  leds: tps68470: Support the WLED driver
  platform/x86: int3472: Support LED lookups in board data
  platform/x86: int3472: Define LED lookup data for MS Surface Go

 drivers/leds/leds-tps68470.c                  | 170 +++++++++++++++++-
 drivers/platform/x86/intel/int3472/tps68470.c |  31 +++-
 drivers/platform/x86/intel/int3472/tps68470.h |  10 ++
 .../x86/intel/int3472/tps68470_board_data.c   |  31 ++++
 include/linux/mfd/tps68470.h                  |  12 ++
 include/linux/platform_data/tps68470.h        |  11 ++
 6 files changed, 248 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH 1/8] platform/x86: int3472: Add platform data for LEDs

[Daniel Scally]

Some of the LEDs that can be provided by the TPS68470 PMIC come with
various configuration registers that must be set to appropriate values.
Add a platform data struct so that those data can be defined and
passed to the tps68470-led platform device.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c |  2 ++
 drivers/platform/x86/intel/int3472/tps68470.h |  2 ++
 include/linux/platform_data/tps68470.h        | 11 +++++++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 82ef022f8916..53b0459f278a 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -194,6 +194,8 @@ 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-led";
+		cells[2].platform_data = (void *)board_data->tps68470_led_pdata;
+		cells[2].pdata_size = sizeof(struct tps68470_led_platform_data);
 		cells[3].name = "tps68470-gpio";
 
 		for (i = 0; i < board_data->n_gpiod_lookups; i++)
diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
index 35915e701593..ce50687db6fb 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.h
+++ b/drivers/platform/x86/intel/int3472/tps68470.h
@@ -13,10 +13,12 @@
 
 struct gpiod_lookup_table;
 struct tps68470_regulator_platform_data;
+struct tps68470_led_platform_data;
 
 struct int3472_tps68470_board_data {
 	const char *dev_name;
 	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
+	const struct tps68470_led_platform_data *tps68470_led_pdata;
 	unsigned int n_gpiod_lookups;
 	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
 };
diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
index e605a2cab07f..5d55ad5c17ed 100644
--- a/include/linux/platform_data/tps68470.h
+++ b/include/linux/platform_data/tps68470.h
@@ -37,4 +37,15 @@ struct tps68470_clk_platform_data {
 	struct tps68470_clk_consumer consumers[];
 };
 
+struct tps68470_led_platform_data {
+	u8 iledctl_ctrlb;
+	u8 wledmaxf;
+	u8 wledto;
+	u8 wledc1;
+	u8 wledc2;
+	u8 wledctl_mode;
+	bool wledctl_disled1;
+	bool wledctl_disled2;
+};
+
 #endif
-- 
2.34.1

[PATCH 2/8] platform/x86: int3472: Init LED registers using platform data

[Daniel Scally]

Check platform data to discover the appropriate settings for the PMIC's
WLED registers and set them during probe.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/leds/leds-tps68470.c | 51 ++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
index 35aeb5db89c8..d2060fe4259d 100644
--- a/drivers/leds/leds-tps68470.c
+++ b/drivers/leds/leds-tps68470.c
@@ -11,6 +11,7 @@
 #include <linux/leds.h>
 #include <linux/mfd/tps68470.h>
 #include <linux/module.h>
+#include <linux/platform_data/tps68470.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
@@ -113,6 +114,52 @@ static int tps68470_ledb_current_init(struct platform_device *pdev,
 	return ret;
 }
 
+static int tps68470_leds_init(struct tps68470_device *tps68470)
+{
+	struct tps68470_led_platform_data *pdata = tps68470->dev->platform_data;
+	int ret;
+
+	if (!pdata)
+		return 0;
+
+	ret = regmap_write(tps68470->regmap, TPS68470_REG_ILEDCTL, pdata->iledctl_ctrlb);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set ILED CTRLB\n");
+
+	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDMAXF,
+			   pdata->wledmaxf & TPS68470_WLEDMAXF_MAX_CUR_MASK);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDMAXF\n");
+
+	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDTO, pdata->wledto);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDTO\n");
+
+	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDC1,
+			   pdata->wledc1 & TPS68470_WLEDC_ILED_MASK);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDC1\n");
+
+	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDC2,
+			   pdata->wledc2 & TPS68470_WLEDC_ILED_MASK);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDC2\n");
+
+	ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
+				 TPS68470_WLED_DISLED1,
+				 pdata->wledctl_disled1 ? TPS68470_WLED_DISLED1 : 0);
+	if (ret)
+		return dev_err_probe(tps68470->dev, ret, "failed to set DISLED1\n");
+
+	ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
+				 TPS68470_WLED_DISLED2,
+				 pdata->wledctl_disled2 ? TPS68470_WLED_DISLED2 : 0);
+	if (ret)
+		dev_err_probe(tps68470->dev, ret, "failed to set DISLED2\n");
+
+	return 0;
+}
+
 static int tps68470_leds_probe(struct platform_device *pdev)
 {
 	int i = 0;
@@ -160,6 +207,10 @@ static int tps68470_leds_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = tps68470_leds_init(tps68470);
+	if (ret)
+		goto err_exit;
+
 err_exit:
 	if (ret) {
 		for (i = 0; i < TPS68470_NUM_LEDS; i++) {
-- 
2.34.1

[PATCH 3/8] platform/x86: int3472: Add TPS68470 LED Board Data

[Daniel Scally]

Add the board data for the Surface Go platforms to configure the LEDs
provided by the TPS68470 PMIC.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../x86/intel/int3472/tps68470_board_data.c         | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index 322237e056f3..0d46a238b630 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -146,9 +146,21 @@ static struct gpiod_lookup_table surface_go_int347e_gpios = {
 	}
 };
 
+static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
+	.iledctl_ctrlb = 0x30,
+	.wledmaxf = 0x1f,
+	.wledto = 0x07,
+	.wledc1 = 0x1f,
+	.wledc2 = 0x1f,
+	.wledctl_mode = 0x00,
+	.wledctl_disled1 = true,
+	.wledctl_disled2 = false,
+};
+
 static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
 	.dev_name = "i2c-INT3472:05",
 	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
+	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
 	.n_gpiod_lookups = 2,
 	.tps68470_gpio_lookup_tables = {
 		&surface_go_int347a_gpios,
@@ -159,6 +171,7 @@ static const struct int3472_tps68470_board_data surface_go_tps68470_board_data =
 static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data = {
 	.dev_name = "i2c-INT3472:01",
 	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
+	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
 	.n_gpiod_lookups = 2,
 	.tps68470_gpio_lookup_tables = {
 		&surface_go_int347a_gpios,
-- 
2.34.1

[PATCH 4/8] platform/x86: int3472: Add tps68470-led as clock consumer

[Daniel Scally]

Some of the LEDs provided by the TPS68470 require the clock that it
provides to be active in order to function. Add the platform driver
for the leds as a consumer of the clock so that the led driver can
discover it during .probe()

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 53b0459f278a..818f2fc5bf2a 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -105,25 +105,30 @@ skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct acpi_device *consumer;
-	unsigned int n_consumers = 0;
+	unsigned int n_consumers = 1;
 	const char *sensor_name;
-	unsigned int i = 0;
+	const char *led_name;
+	unsigned int i = 1;
 
 	for_each_acpi_consumer_dev(adev, consumer)
 		n_consumers++;
 
-	if (!n_consumers) {
-		dev_err(dev, "INT3472 seems to have no dependents\n");
-		return -ENODEV;
-	}
-
 	*clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers),
 				  GFP_KERNEL);
 	if (!*clk_pdata)
 		return -ENOMEM;
 
 	(*clk_pdata)->n_consumers = n_consumers;
-	i = 0;
+
+	/*
+	 * The TPS68470 includes an LED driver which requires the clock be active
+	 * to function. Add the led platform device as a consumer of the clock.
+	 */
+	led_name = devm_kstrdup(dev, "tps68470-led", GFP_KERNEL);
+	if (!led_name)
+		return -ENOMEM;
+
+	(*clk_pdata)->consumers[0].consumer_dev_name = led_name;
 
 	for_each_acpi_consumer_dev(adev, consumer) {
 		sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,
-- 
2.34.1

[PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Daniel Scally]

We want to extend tps68470_brightness_get() to be usable with the
other LEDs supplied by the IC; refactor it to make that easier.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/leds/leds-tps68470.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
index d2060fe4259d..44df175d25de 100644
--- a/drivers/leds/leds-tps68470.c
+++ b/drivers/leds/leds-tps68470.c
@@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
 	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;
+		ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
+		if (ret)
+			return dev_err_probe(led_cdev->dev, ret,
+					     "failed to read LED status\n");
+
+		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
+					TPS68470_ILEDCTL_ENB;
 		break;
+	default:
+		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
 	}
 
 	return value ? LED_ON : LED_OFF;
 }
 
-
 static int tps68470_ledb_current_init(struct platform_device *pdev,
 				      struct tps68470_device *tps68470)
 {
-- 
2.34.1

[PATCH 6/8] leds: tps68470: Support the WLED driver

[Daniel Scally]

The TPS68470 PMIC provides a third LED driver in addition to the two
indicator LEDs. Add support for the WLED. To ensure the LED is active
for as long as the kernel instructs it to be we need to re-trigger it
periodically to avoid the IC's internal timeouts.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/leds/leds-tps68470.c | 102 ++++++++++++++++++++++++++++++++++-
 include/linux/mfd/tps68470.h |  12 +++++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
index 44df175d25de..abcd3494b1a8 100644
--- a/drivers/leds/leds-tps68470.c
+++ b/drivers/leds/leds-tps68470.c
@@ -8,6 +8,7 @@
  *	Kate Hsuan <hpa@redhat.com>
  */
 
+#include <linux/clk.h>
 #include <linux/leds.h>
 #include <linux/mfd/tps68470.h>
 #include <linux/module.h>
@@ -15,7 +16,10 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/workqueue.h>
 
+#define work_to_led(work) \
+	container_of(work, struct tps68470_led, keepalive_work)
 
 #define lcdev_to_led(led_cdev) \
 	container_of(led_cdev, struct tps68470_led, lcdev)
@@ -26,20 +30,25 @@
 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;
+	enum led_brightness state;
+	struct work_struct keepalive_work;
 };
 
 struct tps68470_device {
+	struct clk *clk;
 	struct device *dev;
 	struct regmap *regmap;
 	struct tps68470_led leds[TPS68470_NUM_LEDS];
@@ -52,11 +61,33 @@ enum ctrlb_current {
 	CTRLB_16MA	= 3,
 };
 
+/*
+ * The WLED can operate in different modes, including a Flash and Torch mode. In
+ * each mode there's a timeout which ranges from a matter of milliseconds to up
+ * to 13 seconds. We don't want that timeout to apply though because the LED
+ * should be lit until we say that it should no longer be lit, re-trigger the
+ * LED periodically to keep it alive.
+ */
+static void tps68470_wled_keepalive_work(struct work_struct *work)
+{
+	struct tps68470_device *tps68470;
+	struct tps68470_led *led;
+
+	led = work_to_led(work);
+	tps68470 = led_to_tps68470(led, led->led_id);
+
+	regmap_update_bits_async(tps68470->regmap, TPS68470_REG_WLEDCTL,
+				 TPS68470_WLED_CTL_MASK, TPS68470_WLED_CTL_MASK);
+	schedule_work(&led->keepalive_work);
+}
+
 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;
+	const char *errmsg;
+	int ret;
 
 	switch (led->led_id) {
 	case TPS68470_ILED_A:
@@ -65,8 +96,59 @@ static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brigh
 	case TPS68470_ILED_B:
 		return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB,
 					  brightness ? TPS68470_ILEDCTL_ENB : 0);
+	case TPS68470_WLED:
+		/*
+		 * LED core does not prevent re-setting brightness to its current
+		 * value; we need to do so here to avoid unbalanced calls to clk
+		 * enable/disable.
+		 */
+		if (led->state == brightness)
+			return 0;
+
+		if (brightness) {
+			schedule_work(&led->keepalive_work);
+
+			ret = clk_prepare_enable(tps68470->clk);
+			if (ret) {
+				errmsg = "failed to start clock\n";
+				goto err_cancel_work;
+			}
+		} else {
+			cancel_work_sync(&led->keepalive_work);
+			clk_disable_unprepare(tps68470->clk);
+		}
+
+		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
+					 TPS68470_WLED_EN_MASK,
+					 brightness ? TPS68470_WLED_EN_MASK :
+						      ~TPS68470_WLED_EN_MASK);
+		if (ret) {
+			errmsg = "failed to set WLED EN\n";
+			goto err_disable_clk;
+		}
+
+		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
+					 TPS68470_WLED_CTL_MASK,
+					 brightness ? TPS68470_WLED_CTL_MASK :
+						      ~TPS68470_WLED_CTL_MASK);
+		if (ret) {
+			errmsg = "failed to set WLED START\n";
+			goto err_disable_clk;
+		}
+
+		led->state = brightness;
+		break;
+	default:
+		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
 	}
-	return -EINVAL;
+
+	return ret;
+
+err_disable_clk:
+	clk_disable_unprepare(tps68470->clk);
+err_cancel_work:
+	cancel_work_sync(&led->keepalive_work);
+	return dev_err_probe(tps68470->dev, ret, errmsg);
 }
 
 static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
@@ -88,6 +170,14 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
 		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
 					TPS68470_ILEDCTL_ENB;
 		break;
+	case TPS68470_WLED:
+		ret = regmap_read(regmap, TPS68470_REG_WLEDCTL, &value);
+		if (ret)
+			return dev_err_probe(led_cdev->dev, ret,
+					     "failed to read LED status\n");
+
+		value &= TPS68470_WLED_CTL_MASK;
+		break;
 	default:
 		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
 	}
@@ -177,6 +267,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
 	tps68470->dev = &pdev->dev;
 	tps68470->regmap = dev_get_drvdata(pdev->dev.parent);
 
+	tps68470->clk = devm_clk_get(tps68470->dev, NULL);
+	if (IS_ERR(tps68470->clk))
+		return dev_err_probe(tps68470->dev, PTR_ERR(tps68470->clk),
+				     "failed to get clock\n");
+
 	for (i = 0; i < TPS68470_NUM_LEDS; i++) {
 		led = &tps68470->leds[i];
 		lcdev = &led->lcdev;
@@ -206,6 +301,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
 			if (ret)
 				goto err_exit;
 		}
+
+		if (led->led_id == TPS68470_WLED) {
+			INIT_WORK(&led->keepalive_work,
+				  tps68470_wled_keepalive_work);
+		}
 	}
 
 	ret = tps68470_leds_init(tps68470);
diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
index 2d2abb25b944..103ff730e028 100644
--- a/include/linux/mfd/tps68470.h
+++ b/include/linux/mfd/tps68470.h
@@ -35,6 +35,11 @@
 #define TPS68470_REG_GPDI		0x26
 #define TPS68470_REG_GPDO		0x27
 #define TPS68470_REG_ILEDCTL		0x28
+#define TPS68470_REG_WLEDMAXF		0x2F
+#define TPS68470_REG_WLEDTO		0x30
+#define TPS68470_REG_WLEDC1		0x34
+#define TPS68470_REG_WLEDC2		0x35
+#define TPS68470_REG_WLEDCTL		0x36
 #define TPS68470_REG_VCMVAL		0x3C
 #define TPS68470_REG_VAUX1VAL		0x3D
 #define TPS68470_REG_VAUX2VAL		0x3E
@@ -98,5 +103,12 @@
 #define TPS68470_ILEDCTL_ENA		BIT(2)
 #define TPS68470_ILEDCTL_ENB		BIT(6)
 #define TPS68470_ILEDCTL_CTRLB		GENMASK(5, 4)
+#define TPS68470_WLEDMAXF_MAX_CUR_MASK	GENMASK(4, 0)
+#define TPS68470_WLEDC_ILED_MASK	GENMASK(4, 0)
+#define TPS68470_WLED_MODE_MASK		GENMASK(1, 0)
+#define TPS68470_WLED_EN_MASK		BIT(2)
+#define TPS68470_WLED_DISLED1		BIT(3)
+#define TPS68470_WLED_DISLED2		BIT(4)
+#define TPS68470_WLED_CTL_MASK		BIT(5)
 
 #endif /* __LINUX_MFD_TPS68470_H */
-- 
2.34.1

[PATCH 7/8] platform/x86: int3472: Support LED lookups in board data

[Daniel Scally]

On platforms with the TPS68470 PMIC, we need to be able to define
which of the LEDs powered by the PMIC should be used by each of
the sensors that consume its resources. Add the ability to define
tables of LED lookup data to the board data file.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 8 ++++++++
 drivers/platform/x86/intel/int3472/tps68470.h | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 818f2fc5bf2a..07ac7b5b9082 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -206,6 +206,10 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		for (i = 0; i < board_data->n_gpiod_lookups; i++)
 			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
 
+		if (board_data->led_lookups)
+			for (i = 0; i < board_data->led_lookups->n_lookups; i++)
+				led_add_lookup(&board_data->led_lookups->lookup_table[i]);
+
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 					   cells, TPS68470_WIN_MFD_CELL_COUNT,
 					   NULL, 0, NULL);
@@ -214,6 +218,10 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		if (ret) {
 			for (i = 0; i < board_data->n_gpiod_lookups; i++)
 				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
+
+			if (board_data->led_lookups)
+				for (i = 0; i < board_data->led_lookups->n_lookups; i++)
+					led_remove_lookup(&board_data->led_lookups->lookup_table[i]);
 		}
 
 		break;
diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
index ce50687db6fb..c03884654898 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.h
+++ b/drivers/platform/x86/intel/int3472/tps68470.h
@@ -11,14 +11,22 @@
 #ifndef _INTEL_SKL_INT3472_TPS68470_H
 #define _INTEL_SKL_INT3472_TPS68470_H
 
+#include <linux/leds.h>
+
 struct gpiod_lookup_table;
 struct tps68470_regulator_platform_data;
 struct tps68470_led_platform_data;
 
+struct tps68470_led_lookups {
+	unsigned int n_lookups;
+	struct led_lookup_data lookup_table[];
+};
+
 struct int3472_tps68470_board_data {
 	const char *dev_name;
 	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
 	const struct tps68470_led_platform_data *tps68470_led_pdata;
+	struct tps68470_led_lookups *led_lookups;
 	unsigned int n_gpiod_lookups;
 	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
 };
-- 
2.34.1

[PATCH 8/8] platform/x86: int3472: Define LED lookup data for MS Surface Go

[Daniel Scally]

Add LED lookup data to tps68470_board_data.c for the Microsoft
Surface Go line of devices.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../x86/intel/int3472/tps68470_board_data.c    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index 0d46a238b630..e2c53319e112 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -157,10 +157,27 @@ static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
 	.wledctl_disled2 = false,
 };
 
+static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
+	.n_lookups = 2,
+	.lookup_table = {
+		{
+			.provider = "tps68470-iled_a::indicator",
+			.dev_id = "i2c-INT347A:00",
+			.con_id = "privacy-led",
+		},
+		{
+			.provider = "tps68470-wled::indicator",
+			.dev_id = "i2c-INT347E:00",
+			.con_id = "privacy-led",
+		},
+	},
+};
+
 static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
 	.dev_name = "i2c-INT3472:05",
 	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
 	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
+	.led_lookups = &surface_go_tps68470_led_lookups,
 	.n_gpiod_lookups = 2,
 	.tps68470_gpio_lookup_tables = {
 		&surface_go_int347a_gpios,
@@ -172,6 +189,7 @@ static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data
 	.dev_name = "i2c-INT3472:01",
 	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
 	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
+	.led_lookups = &surface_go_tps68470_led_lookups,
 	.n_gpiod_lookups = 2,
 	.tps68470_gpio_lookup_tables = {
 		&surface_go_int347a_gpios,
-- 
2.34.1

Re: [PATCH 1/8] platform/x86: int3472: Add platform data for LEDs

[Hans de Goede]

Hi Daniel,

Thank you for your work in this!

On 3/22/23 17:09, Daniel Scally wrote:
> Some of the LEDs that can be provided by the TPS68470 PMIC come with
> various configuration registers that must be set to appropriate values.
> Add a platform data struct so that those data can be defined and
> passed to the tps68470-led platform device.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c |  2 ++
>  drivers/platform/x86/intel/int3472/tps68470.h |  2 ++
>  include/linux/platform_data/tps68470.h        | 11 +++++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 82ef022f8916..53b0459f278a 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -194,6 +194,8 @@ 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-led";
> +		cells[2].platform_data = (void *)board_data->tps68470_led_pdata;
> +		cells[2].pdata_size = sizeof(struct tps68470_led_platform_data);
>  		cells[3].name = "tps68470-gpio";
>  
>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index 35915e701593..ce50687db6fb 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.h
> +++ b/drivers/platform/x86/intel/int3472/tps68470.h
> @@ -13,10 +13,12 @@
>  
>  struct gpiod_lookup_table;
>  struct tps68470_regulator_platform_data;
> +struct tps68470_led_platform_data;
>  
>  struct int3472_tps68470_board_data {
>  	const char *dev_name;
>  	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
> +	const struct tps68470_led_platform_data *tps68470_led_pdata;
>  	unsigned int n_gpiod_lookups;
>  	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
>  };


> diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
> index e605a2cab07f..5d55ad5c17ed 100644
> --- a/include/linux/platform_data/tps68470.h
> +++ b/include/linux/platform_data/tps68470.h
> @@ -37,4 +37,15 @@ struct tps68470_clk_platform_data {
>  	struct tps68470_clk_consumer consumers[];
>  };
>  
> +struct tps68470_led_platform_data {
> +	u8 iledctl_ctrlb;
> +	u8 wledmaxf;
> +	u8 wledto;
> +	u8 wledc1;
> +	u8 wledc2;
> +	u8 wledctl_mode;
> +	bool wledctl_disled1;
> +	bool wledctl_disled2;
> +};
> +
>  #endif


It seems to me that these include/linux/platform_data/tps68470.h
changes should be squashed to:

[PATCH 2/8] platform/x86: int3472: Init LED registers using platform data

Which BTW has the wrong subsys prefix, it should be named:

[PATCH 2/8] leds: tps68470: Init LED registers using platform data

or even better:

[PATCH 2/8] leds: tps68470: Init WLED registers using platform data


And then squash the rest of this patch into:

[PATCH 3/8] platform/x86: int3472: Add TPS68470 LED Board Data

please and drop this now empty patch from the set.

Regards,

Hans

Re: [PATCH 2/8] platform/x86: int3472: Init LED registers using platform data

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Check platform data to discover the appropriate settings for the PMIC's
> WLED registers and set them during probe.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

As mentioned already the subsys-prefix for this patch is wrong
and maybe also in the subject do s/LED/WLED/, so we get:

[PATCH 2/8] leds: tps68470: Init WLED registers using platform data

although since the pdata is shared for all tps68470 LEDs
I guess without the W it makes sense too.

With the Subject subsys prefix fixed this is:

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

Regards,

Hans



> ---
>  drivers/leds/leds-tps68470.c | 51 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> index 35aeb5db89c8..d2060fe4259d 100644
> --- a/drivers/leds/leds-tps68470.c
> +++ b/drivers/leds/leds-tps68470.c
> @@ -11,6 +11,7 @@
>  #include <linux/leds.h>
>  #include <linux/mfd/tps68470.h>
>  #include <linux/module.h>
> +#include <linux/platform_data/tps68470.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> @@ -113,6 +114,52 @@ static int tps68470_ledb_current_init(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static int tps68470_leds_init(struct tps68470_device *tps68470)
> +{
> +	struct tps68470_led_platform_data *pdata = tps68470->dev->platform_data;
> +	int ret;
> +
> +	if (!pdata)
> +		return 0;
> +
> +	ret = regmap_write(tps68470->regmap, TPS68470_REG_ILEDCTL, pdata->iledctl_ctrlb);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set ILED CTRLB\n");
> +
> +	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDMAXF,
> +			   pdata->wledmaxf & TPS68470_WLEDMAXF_MAX_CUR_MASK);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDMAXF\n");
> +
> +	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDTO, pdata->wledto);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDTO\n");
> +
> +	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDC1,
> +			   pdata->wledc1 & TPS68470_WLEDC_ILED_MASK);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDC1\n");
> +
> +	ret = regmap_write(tps68470->regmap, TPS68470_REG_WLEDC2,
> +			   pdata->wledc2 & TPS68470_WLEDC_ILED_MASK);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set WLEDC2\n");
> +
> +	ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> +				 TPS68470_WLED_DISLED1,
> +				 pdata->wledctl_disled1 ? TPS68470_WLED_DISLED1 : 0);
> +	if (ret)
> +		return dev_err_probe(tps68470->dev, ret, "failed to set DISLED1\n");
> +
> +	ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> +				 TPS68470_WLED_DISLED2,
> +				 pdata->wledctl_disled2 ? TPS68470_WLED_DISLED2 : 0);
> +	if (ret)
> +		dev_err_probe(tps68470->dev, ret, "failed to set DISLED2\n");
> +
> +	return 0;
> +}
> +
>  static int tps68470_leds_probe(struct platform_device *pdev)
>  {
>  	int i = 0;
> @@ -160,6 +207,10 @@ static int tps68470_leds_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = tps68470_leds_init(tps68470);
> +	if (ret)
> +		goto err_exit;
> +
>  err_exit:
>  	if (ret) {
>  		for (i = 0; i < TPS68470_NUM_LEDS; i++) {

Re: [PATCH 3/8] platform/x86: int3472: Add TPS68470 LED Board Data

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Add the board data for the Surface Go platforms to configure the LEDs
> provided by the TPS68470 PMIC.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

As mentioned in the review of 1/8 please squash the 
drivers/platorm/x86/intel/int3472/ of 1/8 into this, with
that fixed this is:

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

Regards,

Hans

> ---
>  ...ctps68470_board_data.c         | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index 322237e056f3..0d46a238b630 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -146,9 +146,21 @@ static struct gpiod_lookup_table surface_go_int347e_gpios = {
>  	}
>  };
>  
> +static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
> +	.iledctl_ctrlb = 0x30,
> +	.wledmaxf = 0x1f,
> +	.wledto = 0x07,
> +	.wledc1 = 0x1f,
> +	.wledc2 = 0x1f,
> +	.wledctl_mode = 0x00,
> +	.wledctl_disled1 = true,
> +	.wledctl_disled2 = false,
> +};
> +
>  static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
>  	.dev_name = "i2c-INT3472:05",
>  	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
> +	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
>  	.n_gpiod_lookups = 2,
>  	.tps68470_gpio_lookup_tables = {
>  		&surface_go_int347a_gpios,
> @@ -159,6 +171,7 @@ static const struct int3472_tps68470_board_data surface_go_tps68470_board_data =
>  static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data = {
>  	.dev_name = "i2c-INT3472:01",
>  	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
> +	.tps68470_led_pdata = &surface_go_tps68470_led_pdata,
>  	.n_gpiod_lookups = 2,
>  	.tps68470_gpio_lookup_tables = {
>  		&surface_go_int347a_gpios,

Re: [PATCH 4/8] platform/x86: int3472: Add tps68470-led as clock consumer

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Some of the LEDs provided by the TPS68470 require the clock that it
> provides to be active in order to function. Add the platform driver
> for the leds as a consumer of the clock so that the led driver can
> discover it during .probe()
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 21 ++++++++++++-------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 53b0459f278a..818f2fc5bf2a 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -105,25 +105,30 @@ skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
>  	struct acpi_device *consumer;
> -	unsigned int n_consumers = 0;
> +	unsigned int n_consumers = 1;
>  	const char *sensor_name;
> -	unsigned int i = 0;
> +	const char *led_name;
> +	unsigned int i = 1;

Nitpick: please just don't init i here at all.

>  
>  	for_each_acpi_consumer_dev(adev, consumer)
>  		n_consumers++;
>  
> -	if (!n_consumers) {
> -		dev_err(dev, "INT3472 seems to have no dependents\n");
> -		return -ENODEV;
> -	}
> -
>  	*clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers),
>  				  GFP_KERNEL);
>  	if (!*clk_pdata)
>  		return -ENOMEM;
>  
>  	(*clk_pdata)->n_consumers = n_consumers;
> -	i = 0;
> +
> +	/*
> +	 * The TPS68470 includes an LED driver which requires the clock be active
> +	 * to function. Add the led platform device as a consumer of the clock.
> +	 */
> +	led_name = devm_kstrdup(dev, "tps68470-led", GFP_KERNEL);
> +	if (!led_name)
> +		return -ENOMEM;
> +
> +	(*clk_pdata)->consumers[0].consumer_dev_name = led_name;
>  

And add a:

	i = 1;

here, to make it clear that after setting consumers[0] we now
start adding more consumers at index 1.

>  	for_each_acpi_consumer_dev(adev, consumer) {
>  		sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,

With that fixed:

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

Regards,

Hans

Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> We want to extend tps68470_brightness_get() to be usable with the
> other LEDs supplied by the IC; refactor it to make that easier.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/leds/leds-tps68470.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> index d2060fe4259d..44df175d25de 100644
> --- a/drivers/leds/leds-tps68470.c
> +++ b/drivers/leds/leds-tps68470.c
> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
>  	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;
> +		ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
> +		if (ret)
> +			return dev_err_probe(led_cdev->dev, ret,
> +					     "failed to read LED status\n");

I realize this is a pre-existing problem, but I don't think we should
be using dev_err_probe() in functions which are used outside the probe()
path?

So maybe fix this up while at it and make this:

		if (ret) {
			dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret);
			return ret;
		}

> +
> +		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
> +					TPS68470_ILEDCTL_ENB;
>  		break;
> +	default:
> +		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");

idem.

With those fixed:

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

Regards,

Hans



>  	}
>  
>  	return value ? LED_ON : LED_OFF;
>  }
>  
> -
>  static int tps68470_ledb_current_init(struct platform_device *pdev,
>  				      struct tps68470_device *tps68470)
>  {

Re: [PATCH 6/8] leds: tps68470: Support the WLED driver

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> The TPS68470 PMIC provides a third LED driver in addition to the two
> indicator LEDs. Add support for the WLED. To ensure the LED is active
> for as long as the kernel instructs it to be we need to re-trigger it
> periodically to avoid the IC's internal timeouts.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/leds/leds-tps68470.c | 102 ++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/tps68470.h |  12 +++++
>  2 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> index 44df175d25de..abcd3494b1a8 100644
> --- a/drivers/leds/leds-tps68470.c
> +++ b/drivers/leds/leds-tps68470.c
> @@ -8,6 +8,7 @@
>   *	Kate Hsuan <hpa@redhat.com>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/leds.h>
>  #include <linux/mfd/tps68470.h>
>  #include <linux/module.h>
> @@ -15,7 +16,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> +#include <linux/workqueue.h>
>  
> +#define work_to_led(work) \
> +	container_of(work, struct tps68470_led, keepalive_work)
>  
>  #define lcdev_to_led(led_cdev) \
>  	container_of(led_cdev, struct tps68470_led, lcdev)
> @@ -26,20 +30,25 @@
>  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;
> +	enum led_brightness state;
> +	struct work_struct keepalive_work;

The way this is used does not look very periodical, it seems to just run as fast as it can?

IMHO this should use a delayed-work item and then queue that to run at a certain interval.

>  };
>  
>  struct tps68470_device {
> +	struct clk *clk;
>  	struct device *dev;
>  	struct regmap *regmap;
>  	struct tps68470_led leds[TPS68470_NUM_LEDS];
> @@ -52,11 +61,33 @@ enum ctrlb_current {
>  	CTRLB_16MA	= 3,
>  };
>  
> +/*
> + * The WLED can operate in different modes, including a Flash and Torch mode. In
> + * each mode there's a timeout which ranges from a matter of milliseconds to up
> + * to 13 seconds. We don't want that timeout to apply though because the LED
> + * should be lit until we say that it should no longer be lit, re-trigger the
> + * LED periodically to keep it alive.
> + */
> +static void tps68470_wled_keepalive_work(struct work_struct *work)
> +{
> +	struct tps68470_device *tps68470;
> +	struct tps68470_led *led;
> +
> +	led = work_to_led(work);
> +	tps68470 = led_to_tps68470(led, led->led_id);
> +
> +	regmap_update_bits_async(tps68470->regmap, TPS68470_REG_WLEDCTL,
> +				 TPS68470_WLED_CTL_MASK, TPS68470_WLED_CTL_MASK);
> +	schedule_work(&led->keepalive_work);
> +}
> +
>  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;
> +	const char *errmsg;
> +	int ret;
>  
>  	switch (led->led_id) {
>  	case TPS68470_ILED_A:
> @@ -65,8 +96,59 @@ static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brigh
>  	case TPS68470_ILED_B:
>  		return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB,
>  					  brightness ? TPS68470_ILEDCTL_ENB : 0);
> +	case TPS68470_WLED:
> +		/*
> +		 * LED core does not prevent re-setting brightness to its current
> +		 * value; we need to do so here to avoid unbalanced calls to clk
> +		 * enable/disable.
> +		 */
> +		if (led->state == brightness)
> +			return 0;
> +
> +		if (brightness) {
> +			schedule_work(&led->keepalive_work);
> +
> +			ret = clk_prepare_enable(tps68470->clk);
> +			if (ret) {
> +				errmsg = "failed to start clock\n";
> +				goto err_cancel_work;
> +			}
> +		} else {
> +			cancel_work_sync(&led->keepalive_work);
> +			clk_disable_unprepare(tps68470->clk);
> +		}
> +
> +		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> +					 TPS68470_WLED_EN_MASK,
> +					 brightness ? TPS68470_WLED_EN_MASK :
> +						      ~TPS68470_WLED_EN_MASK);
> +		if (ret) {
> +			errmsg = "failed to set WLED EN\n";
> +			goto err_disable_clk;
> +		}
> +
> +		ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> +					 TPS68470_WLED_CTL_MASK,
> +					 brightness ? TPS68470_WLED_CTL_MASK :
> +						      ~TPS68470_WLED_CTL_MASK);
> +		if (ret) {
> +			errmsg = "failed to set WLED START\n";
> +			goto err_disable_clk;
> +		}
> +
> +		led->state = brightness;
> +		break;
> +	default:
> +		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
>  	}
> -	return -EINVAL;
> +
> +	return ret;
> +
> +err_disable_clk:
> +	clk_disable_unprepare(tps68470->clk);
> +err_cancel_work:
> +	cancel_work_sync(&led->keepalive_work);
> +	return dev_err_probe(tps68470->dev, ret, errmsg);
>  }
>  
>  static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> @@ -88,6 +170,14 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
>  		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
>  					TPS68470_ILEDCTL_ENB;
>  		break;
> +	case TPS68470_WLED:
> +		ret = regmap_read(regmap, TPS68470_REG_WLEDCTL, &value);
> +		if (ret)
> +			return dev_err_probe(led_cdev->dev, ret,
> +					     "failed to read LED status\n");
> +
> +		value &= TPS68470_WLED_CTL_MASK;
> +		break;
>  	default:
>  		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
>  	}
> @@ -177,6 +267,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
>  	tps68470->dev = &pdev->dev;
>  	tps68470->regmap = dev_get_drvdata(pdev->dev.parent);
>  
> +	tps68470->clk = devm_clk_get(tps68470->dev, NULL);
> +	if (IS_ERR(tps68470->clk))
> +		return dev_err_probe(tps68470->dev, PTR_ERR(tps68470->clk),
> +				     "failed to get clock\n");
> +
>  	for (i = 0; i < TPS68470_NUM_LEDS; i++) {
>  		led = &tps68470->leds[i];
>  		lcdev = &led->lcdev;
> @@ -206,6 +301,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
>  			if (ret)
>  				goto err_exit;
>  		}
> +
> +		if (led->led_id == TPS68470_WLED) {
> +			INIT_WORK(&led->keepalive_work,
> +				  tps68470_wled_keepalive_work);
> +		}
>  	}
>  
>  	ret = tps68470_leds_init(tps68470);
> diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
> index 2d2abb25b944..103ff730e028 100644
> --- a/include/linux/mfd/tps68470.h
> +++ b/include/linux/mfd/tps68470.h
> @@ -35,6 +35,11 @@
>  #define TPS68470_REG_GPDI		0x26
>  #define TPS68470_REG_GPDO		0x27
>  #define TPS68470_REG_ILEDCTL		0x28
> +#define TPS68470_REG_WLEDMAXF		0x2F
> +#define TPS68470_REG_WLEDTO		0x30
> +#define TPS68470_REG_WLEDC1		0x34
> +#define TPS68470_REG_WLEDC2		0x35
> +#define TPS68470_REG_WLEDCTL		0x36
>  #define TPS68470_REG_VCMVAL		0x3C
>  #define TPS68470_REG_VAUX1VAL		0x3D
>  #define TPS68470_REG_VAUX2VAL		0x3E
> @@ -98,5 +103,12 @@
>  #define TPS68470_ILEDCTL_ENA		BIT(2)
>  #define TPS68470_ILEDCTL_ENB		BIT(6)
>  #define TPS68470_ILEDCTL_CTRLB		GENMASK(5, 4)
> +#define TPS68470_WLEDMAXF_MAX_CUR_MASK	GENMASK(4, 0)
> +#define TPS68470_WLEDC_ILED_MASK	GENMASK(4, 0)
> +#define TPS68470_WLED_MODE_MASK		GENMASK(1, 0)
> +#define TPS68470_WLED_EN_MASK		BIT(2)
> +#define TPS68470_WLED_DISLED1		BIT(3)
> +#define TPS68470_WLED_DISLED2		BIT(4)
> +#define TPS68470_WLED_CTL_MASK		BIT(5)
>  
>  #endif /* __LINUX_MFD_TPS68470_H */


Otherwise this LGTM.

Regards,

Hans

Re: [PATCH 7/8] platform/x86: int3472: Support LED lookups in board data

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> On platforms with the TPS68470 PMIC, we need to be able to define
> which of the LEDs powered by the PMIC should be used by each of
> the sensors that consume its resources. Add the ability to define
> tables of LED lookup data to the board data file.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

Thanks, patch looks good to me:

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

Regards,

Hans



> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 8 ++++++++
>  drivers/platform/x86/intel/int3472/tps68470.h | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 818f2fc5bf2a..07ac7b5b9082 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -206,6 +206,10 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
>  			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
>  
> +		if (board_data->led_lookups)
> +			for (i = 0; i < board_data->led_lookups->n_lookups; i++)
> +				led_add_lookup(&board_data->led_lookups->lookup_table[i]);
> +
>  		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>  					   cells, TPS68470_WIN_MFD_CELL_COUNT,
>  					   NULL, 0, NULL);
> @@ -214,6 +218,10 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		if (ret) {
>  			for (i = 0; i < board_data->n_gpiod_lookups; i++)
>  				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
> +
> +			if (board_data->led_lookups)
> +				for (i = 0; i < board_data->led_lookups->n_lookups; i++)
> +					led_remove_lookup(&board_data->led_lookups->lookup_table[i]);
>  		}
>  
>  		break;
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index ce50687db6fb..c03884654898 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.h
> +++ b/drivers/platform/x86/intel/int3472/tps68470.h
> @@ -11,14 +11,22 @@
>  #ifndef _INTEL_SKL_INT3472_TPS68470_H
>  #define _INTEL_SKL_INT3472_TPS68470_H
>  
> +#include <linux/leds.h>
> +
>  struct gpiod_lookup_table;
>  struct tps68470_regulator_platform_data;
>  struct tps68470_led_platform_data;
>  
> +struct tps68470_led_lookups {
> +	unsigned int n_lookups;
> +	struct led_lookup_data lookup_table[];
> +};
> +
>  struct int3472_tps68470_board_data {
>  	const char *dev_name;
>  	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
>  	const struct tps68470_led_platform_data *tps68470_led_pdata;
> +	struct tps68470_led_lookups *led_lookups;
>  	unsigned int n_gpiod_lookups;
>  	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
>  };

Re: [PATCH 8/8] platform/x86: int3472: Define LED lookup data for MS Surface Go

[Hans de Goede]

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Add LED lookup data to tps68470_board_data.c for the Microsoft
> Surface Go line of devices.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  .../x86/intel/int3472/tps68470_board_data.c    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index 0d46a238b630..e2c53319e112 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -157,10 +157,27 @@ static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
>  	.wledctl_disled2 = false,
>  };
>  
> +static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
> +	.n_lookups = 2,
> +	.lookup_table = {
> +		{
> +			.provider = "tps68470-iled_a::indicator",
> +			.dev_id = "i2c-INT347A:00",
> +			.con_id = "privacy-led",
> +		},
> +		{
> +			.provider = "tps68470-wled::indicator",
> +			.dev_id = "i2c-INT347E:00",
> +			.con_id = "privacy-led",
> +		},

So this feels wrong to me in 2 ways:

1. It is abusing .con_id = "privacy-led" to enable the WLED

2. You are not activating the front privacy LED for the IR projector. I have noticed on IPU6 devices that the _DSM listing GPIOs for the discrete INT3472 device lists a privacy-LED GPIO for the IR sensor too, which I so far have been guessing activates the actual privacy-LED. As I'm typing this I'm wondering if instead this is doing the same hack as you are doing here ?  

Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ?

IMHO this should look like this (with either the media-core or the driver consuming "ir-led"):

static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
	.n_lookups = 3,
	.lookup_table = {
		{
			.provider = "tps68470-iled_a::indicator",
			.dev_id = "i2c-INT347A:00",
			.con_id = "privacy-led",
		},
		{
			/* Use regular front-sensor privacy LED for ir-sensor too */
			.provider = "INT33BE_00::privacy_led",
			.dev_id = "i2c-INT347E:00",
			.con_id = "privacy-led",
		},
		{
			.provider = "tps68470-wled::indicator",
			.dev_id = "i2c-INT347E:00",
			.con_id = "ir-led",
		},
	}

Regards,

Hans

Re: [PATCH 0/8] Add WLED support to TPS68470 LED driver

[Hans de Goede]

Hi Daniel,

On 3/22/23 17:09, Daniel Scally wrote:
> This series relies on the recent "leds: tps68470: LED driver for TPS68470" set
> from Kate Hsuan [1]
> 
> The TPS68470 provides two additional LED outputs on top of the indicator LEDs.
> Add support for those to the driver. The configuration of the chip is drawn from
> platform data which is expected to be passed to the driver. Additionally update
> the int3472-tps68470 driver to register led lookups from platform data so that
> the right LED is driven for each sensor, and finally define those lookups for
> the Microsoft Surface Go line.
> 
> Kate, Hans, this is the changes I made on top of the tps68470-led series to
> enable the IR LED on my Go2 (plus one additional patch to media). #5 could
> probably just be squashed into the other series though.

Ack for squashing 5 into Kate's patch.

> The last two patches
> cover how I think the LED lookup should work - I unfortunately can't see an
> automatic way to guarantee the right LED goes to the right sensor.

I did not get around to replying to your review of my lookup patch. I believe
adding this to the board-data as you have done here is fine.

I had a few small comments on your patches and I believe that Kate's patches
are ready for merging now.

So I believe that for the next version it would be best to merge the 2 series
into 1 big series. Putting all "leds: tps68470: ..." patches first. Then once
they are all reviewed Lee can merge the entire drivers/leds/ part of
the series and send me an IB branch to pull from before I merge
the drivers/platform/x86/intel/int3472/ parts.

Kate, is it ok with you if Daniel includes your patches (keeping you as
author of course) in the next version of this series ?

Regards,

Hans





> [1] https://lore.kernel.org/platform-driver-x86/20230321153718.1355511-1-hpa@redhat.com/T/
> 
> Daniel Scally (8):
>   platform/x86: int3472: Add platform data for LEDs
>   platform/x86: int3472: Init LED registers using platform data
>   platform/x86: int3472: Add TPS68470 LED Board Data
>   platform/x86: int3472: Add tps68470-led as clock consumer
>   leds: tps68470: Refactor tps68470_brightness_get()
>   leds: tps68470: Support the WLED driver
>   platform/x86: int3472: Support LED lookups in board data
>   platform/x86: int3472: Define LED lookup data for MS Surface Go
> 
>  drivers/leds/leds-tps68470.c                  | 170 +++++++++++++++++-
>  drivers/platform/x86/intel/int3472/tps68470.c |  31 +++-
>  drivers/platform/x86/intel/int3472/tps68470.h |  10 ++
>  .../x86/intel/int3472/tps68470_board_data.c   |  31 ++++
>  include/linux/mfd/tps68470.h                  |  12 ++
>  include/linux/platform_data/tps68470.h        |  11 ++
>  6 files changed, 248 insertions(+), 17 deletions(-)
> 

Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Dan Scally]

Morning Hans

On 22/03/2023 17:22, Hans de Goede wrote:
> Hi,
>
> On 3/22/23 17:09, Daniel Scally wrote:
>> We want to extend tps68470_brightness_get() to be usable with the
>> other LEDs supplied by the IC; refactor it to make that easier.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   drivers/leds/leds-tps68470.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
>> index d2060fe4259d..44df175d25de 100644
>> --- a/drivers/leds/leds-tps68470.c
>> +++ b/drivers/leds/leds-tps68470.c
>> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
>>   	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;
>> +		ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
>> +		if (ret)
>> +			return dev_err_probe(led_cdev->dev, ret,
>> +					     "failed to read LED status\n");
> I realize this is a pre-existing problem, but I don't think we should
> be using dev_err_probe() in functions which are used outside the probe()
> path?


I had thought that this was being encouraged because of the standard formatting, but actually now I 
re-read the comment's function it's just "OK to use in .probe() even if it can't return 
-EPROBE_DEFER". My bad; I'll fix it.

>
> So maybe fix this up while at it and make this:
>
> 		if (ret) {
> 			dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret);
> 			return ret;
> 		}
>
>> +
>> +		value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
>> +					TPS68470_ILEDCTL_ENB;
>>   		break;
>> +	default:
>> +		return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
> idem.


idem? Sorry, I'm not following here.

>
> With those fixed:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
>
>
>>   	}
>>   
>>   	return value ? LED_ON : LED_OFF;
>>   }
>>   
>> -
>>   static int tps68470_ledb_current_init(struct platform_device *pdev,
>>   				      struct tps68470_device *tps68470)
>>   {

Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Hans de Goede]

Hi,

On 3/23/23 08:43, Dan Scally wrote:
> Morning Hans
> 
> On 22/03/2023 17:22, Hans de Goede wrote:
>> Hi,
>>
>> On 3/22/23 17:09, Daniel Scally wrote:
>>> We want to extend tps68470_brightness_get() to be usable with the
>>> other LEDs supplied by the IC; refactor it to make that easier.
>>>
>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>> ---
>>>   drivers/leds/leds-tps68470.c | 17 +++++++++--------
>>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
>>> index d2060fe4259d..44df175d25de 100644
>>> --- a/drivers/leds/leds-tps68470.c
>>> +++ b/drivers/leds/leds-tps68470.c
>>> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
>>>       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;
>>> +        ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
>>> +        if (ret)
>>> +            return dev_err_probe(led_cdev->dev, ret,
>>> +                         "failed to read LED status\n");
>> I realize this is a pre-existing problem, but I don't think we should
>> be using dev_err_probe() in functions which are used outside the probe()
>> path?
> 
> 
> I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it.
> 
>>
>> So maybe fix this up while at it and make this:
>>
>>         if (ret) {
>>             dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret);
>>             return ret;
>>         }
>>
>>> +
>>> +        value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
>>> +                    TPS68470_ILEDCTL_ENB;
>>>           break;
>>> +    default:
>>> +        return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
>> idem.
> 
> 
> idem? Sorry, I'm not following here.

My bad I though this was something which most people understand / know:

https://en.wikipedia.org/wiki/Idem

So what I was trying to say is:

"the same (don't use dev_err_probe()) applies here".

Regards,

Hans





> 
>>
>> With those fixed:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>       }
>>>         return value ? LED_ON : LED_OFF;
>>>   }
>>>   -
>>>   static int tps68470_ledb_current_init(struct platform_device *pdev,
>>>                         struct tps68470_device *tps68470)
>>>   {
> 

Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Dan Scally]

On 23/03/2023 09:52, Hans de Goede wrote:
> Hi,
>
> On 3/23/23 08:43, Dan Scally wrote:
>> Morning Hans
>>
>> On 22/03/2023 17:22, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 3/22/23 17:09, Daniel Scally wrote:
>>>> We want to extend tps68470_brightness_get() to be usable with the
>>>> other LEDs supplied by the IC; refactor it to make that easier.
>>>>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> ---
>>>>    drivers/leds/leds-tps68470.c | 17 +++++++++--------
>>>>    1 file changed, 9 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
>>>> index d2060fe4259d..44df175d25de 100644
>>>> --- a/drivers/leds/leds-tps68470.c
>>>> +++ b/drivers/leds/leds-tps68470.c
>>>> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
>>>>        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;
>>>> +        ret =  regmap_read(regmap, TPS68470_REG_ILEDCTL, &value);
>>>> +        if (ret)
>>>> +            return dev_err_probe(led_cdev->dev, ret,
>>>> +                         "failed to read LED status\n");
>>> I realize this is a pre-existing problem, but I don't think we should
>>> be using dev_err_probe() in functions which are used outside the probe()
>>> path?
>>
>> I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it.
>>
>>> So maybe fix this up while at it and make this:
>>>
>>>          if (ret) {
>>>              dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret);
>>>              return ret;
>>>          }
>>>
>>>> +
>>>> +        value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
>>>> +                    TPS68470_ILEDCTL_ENB;
>>>>            break;
>>>> +    default:
>>>> +        return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
>>> idem.
>>
>> idem? Sorry, I'm not following here.
> My bad I though this was something which most people understand / know:
>
> https://en.wikipedia.org/wiki/Idem
>
> So what I was trying to say is:
>
> "the same (don't use dev_err_probe()) applies here".


Ah-ha! Thanks, hadn't heard that one before.

>
> Regards,
>
> Hans
>
>
>
>
>
>>> With those fixed:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>>        }
>>>>          return value ? LED_ON : LED_OFF;
>>>>    }
>>>>    -
>>>>    static int tps68470_ledb_current_init(struct platform_device *pdev,
>>>>                          struct tps68470_device *tps68470)
>>>>    {

Re: [PATCH 8/8] platform/x86: int3472: Define LED lookup data for MS Surface Go

[Dan Scally]

Hi Hans

On 22/03/2023 17:34, Hans de Goede wrote:
> Hi,
>
> On 3/22/23 17:09, Daniel Scally wrote:
>> Add LED lookup data to tps68470_board_data.c for the Microsoft
>> Surface Go line of devices.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   .../x86/intel/int3472/tps68470_board_data.c    | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> index 0d46a238b630..e2c53319e112 100644
>> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> @@ -157,10 +157,27 @@ static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
>>   	.wledctl_disled2 = false,
>>   };
>>   
>> +static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
>> +	.n_lookups = 2,
>> +	.lookup_table = {
>> +		{
>> +			.provider = "tps68470-iled_a::indicator",
>> +			.dev_id = "i2c-INT347A:00",
>> +			.con_id = "privacy-led",
>> +		},
>> +		{
>> +			.provider = "tps68470-wled::indicator",
>> +			.dev_id = "i2c-INT347E:00",
>> +			.con_id = "privacy-led",
>> +		},
> So this feels wrong to me in 2 ways:
>
> 1. It is abusing .con_id = "privacy-led" to enable the WLED
>
> 2. You are not activating the front privacy LED for the IR projector. I have noticed on IPU6 devices that the _DSM listing GPIOs for the discrete INT3472 device lists a privacy-LED GPIO for the IR sensor too, which I so far have been guessing activates the actual privacy-LED. As I'm typing this I'm wondering if instead this is doing the same hack as you are doing here ?


Oh interesting; on IPU3 devices with the discrete INT3472 the IR cameras don't seem to have an LED 
GPIO in _DSM so we're not sure how to turn them on yet. I also have a Pro7 which is an IPU4 device, 
but that has the same problem as on the IPU3 ones - there's no privacy-led GPIO defined in _DSM and 
the _ON method for the camera's _PR0 resource just prints "PR Not Supported"...so we don't know how 
to use those yet. So interesting that the IPU6 ones work differently.


>
> Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ?


That does make a certain amount of sense yes - My only thought would be that this would be difficult 
to replicate to platforms that use _only_ discrete versions of the INT3472, because each sensor 
depends on a separate INT3472, so there wouldn't be an obvious way to automatically assign the 
privacy LED for the user facing camera to the IR camera since we couldn't use the board data method 
below. It might be surmountable using the location information in DSDT to decide whether it's on the 
same face as another camera which DOES have a privacy LED maybe...

>
> IMHO this should look like this (with either the media-core or the driver consuming "ir-led"):


The general principle of moving the IR led away from being treated as a privacy LED is ok by me - 
I'll work on that.

>
> static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
> 	.n_lookups = 3,
> 	.lookup_table = {
> 		{
> 			.provider = "tps68470-iled_a::indicator",
> 			.dev_id = "i2c-INT347A:00",
> 			.con_id = "privacy-led",
> 		},
> 		{
> 			/* Use regular front-sensor privacy LED for ir-sensor too */
> 			.provider = "INT33BE_00::privacy_led",
> 			.dev_id = "i2c-INT347E:00",
> 			.con_id = "privacy-led",
> 		},
> 		{
> 			.provider = "tps68470-wled::indicator",
> 			.dev_id = "i2c-INT347E:00",
> 			.con_id = "ir-led",
> 		},
> 	}
>
> Regards,
>
> Hans
>
>

Re: [PATCH 8/8] platform/x86: int3472: Define LED lookup data for MS Surface Go

[Hans de Goede]

Hi,

On 3/23/23 11:31, Dan Scally wrote:
> Hi Hans
> 
> On 22/03/2023 17:34, Hans de Goede wrote:
>> Hi,
>>
>> On 3/22/23 17:09, Daniel Scally wrote:
>>> Add LED lookup data to tps68470_board_data.c for the Microsoft
>>> Surface Go line of devices.
>>>
>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>> ---
>>>   .../x86/intel/int3472/tps68470_board_data.c    | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>>> index 0d46a238b630..e2c53319e112 100644
>>> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>>> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>>> @@ -157,10 +157,27 @@ static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
>>>       .wledctl_disled2 = false,
>>>   };
>>>   +static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
>>> +    .n_lookups = 2,
>>> +    .lookup_table = {
>>> +        {
>>> +            .provider = "tps68470-iled_a::indicator",
>>> +            .dev_id = "i2c-INT347A:00",
>>> +            .con_id = "privacy-led",
>>> +        },
>>> +        {
>>> +            .provider = "tps68470-wled::indicator",
>>> +            .dev_id = "i2c-INT347E:00",
>>> +            .con_id = "privacy-led",
>>> +        },
>> So this feels wrong to me in 2 ways:
>>
>> 1. It is abusing .con_id = "privacy-led" to enable the WLED
>>
>> 2. You are not activating the front privacy LED for the IR projector. I have noticed on IPU6 devices that the _DSM listing GPIOs for the discrete INT3472 device lists a privacy-LED GPIO for the IR sensor too, which I so far have been guessing activates the actual privacy-LED. As I'm typing this I'm wondering if instead this is doing the same hack as you are doing here ?
> 
> 
> Oh interesting; on IPU3 devices with the discrete INT3472 the IR cameras don't seem to have an LED GPIO in _DSM so we're not sure how to turn them on yet. I also have a Pro7 which is an IPU4 device, but that has the same problem as on the IPU3 ones - there's no privacy-led GPIO defined in _DSM and the _ON method for the camera's _PR0 resource just prints "PR Not Supported"...so we don't know how to use those yet. So interesting that the IPU6 ones work differently.
> 
> 
>>
>> Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ?
> 
> 
> That does make a certain amount of sense yes - My only thought would be that this would be difficult to replicate to platforms that use _only_ discrete versions of the INT3472, because each sensor depends on a separate INT3472, so there wouldn't be an obvious way to automatically assign the privacy LED for the user facing camera to the IR camera since we couldn't use the board data method below. It might be surmountable using the location information in DSDT to decide whether it's on the same face as another camera which DOES have a privacy LED maybe...

Right, I realize turning on the front privacy LED when the front ir-sensor is on is not always going to be (easily) doable. But IMHO we should at least do it on platforms where we can do this.

>> IMHO this should look like this (with either the media-core or the driver consuming "ir-led"):
> 
> 
> The general principle of moving the IR led away from being treated as a privacy LED is ok by me - I'll work on that.

Thanks.

Regards,

Hans


> 
>>
>> static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
>>     .n_lookups = 3,
>>     .lookup_table = {
>>         {
>>             .provider = "tps68470-iled_a::indicator",
>>             .dev_id = "i2c-INT347A:00",
>>             .con_id = "privacy-led",
>>         },
>>         {
>>             /* Use regular front-sensor privacy LED for ir-sensor too */
>>             .provider = "INT33BE_00::privacy_led",
>>             .dev_id = "i2c-INT347E:00",
>>             .con_id = "privacy-led",
>>         },
>>         {
>>             .provider = "tps68470-wled::indicator",
>>             .dev_id = "i2c-INT347E:00",
>>             .con_id = "ir-led",
>>         },
>>     }
>>
>> Regards,
>>
>> Hans
>>
>>
> 

Re: [PATCH 6/8] leds: tps68470: Support the WLED driver

[Pavel Machek]

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

On Wed 2023-03-22 16:09:24, Daniel Scally wrote:
> The TPS68470 PMIC provides a third LED driver in addition to the two
> indicator LEDs. Add support for the WLED. To ensure the LED is active
> for as long as the kernel instructs it to be we need to re-trigger it
> periodically to avoid the IC's internal timeouts.

Wow. No!

If hardware does not wart you to burn the LED, it is not okay to just
work around that. These are not designed for continuous operation.

> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> index 44df175d25de..abcd3494b1a8 100644

Fun sha1 hash ;-).

> @@ -52,11 +61,33 @@ enum ctrlb_current {
>  	CTRLB_16MA	= 3,
>  };
>  
> +/*
> + * The WLED can operate in different modes, including a Flash and Torch mode. In
> + * each mode there's a timeout which ranges from a matter of milliseconds to up
> + * to 13 seconds. We don't want that timeout to apply though because the LED
> + * should be lit until we say that it should no longer be lit, re-trigger the
> + * LED periodically to keep it alive.
> + */

We don't want the LED to overheat. That takes precedence.

Find out what are the maximum limits for on time at various current
levels. LED framework should be used for torch mode, with current set
such that unlimited operation is safe. V4L2 should be used for flash
mode.

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 6/8] leds: tps68470: Support the WLED driver

[Dan Scally]

Hi Pavel, thanks for taking a look

On 23/03/2023 11:22, Pavel Machek wrote:
> On Wed 2023-03-22 16:09:24, Daniel Scally wrote:
>> The TPS68470 PMIC provides a third LED driver in addition to the two
>> indicator LEDs. Add support for the WLED. To ensure the LED is active
>> for as long as the kernel instructs it to be we need to re-trigger it
>> periodically to avoid the IC's internal timeouts.
> Wow. No!
>
> If hardware does not wart you to burn the LED, it is not okay to just
> work around that. These are not designed for continuous operation.
>
>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
>> index 44df175d25de..abcd3494b1a8 100644
> Fun sha1 hash ;-).


heh yeh

>
>> @@ -52,11 +61,33 @@ enum ctrlb_current {
>>   	CTRLB_16MA	= 3,
>>   };
>>   
>> +/*
>> + * The WLED can operate in different modes, including a Flash and Torch mode. In
>> + * each mode there's a timeout which ranges from a matter of milliseconds to up
>> + * to 13 seconds. We don't want that timeout to apply though because the LED
>> + * should be lit until we say that it should no longer be lit, re-trigger the
>> + * LED periodically to keep it alive.
>> + */
> We don't want the LED to overheat. That takes precedence.
>
> Find out what are the maximum limits for on time at various current
> levels. LED framework should be used for torch mode, with current set
> such that unlimited operation is safe. V4L2 should be used for flash
> mode.


I did it this way because this is how the IC operates on my device whilst it's booted to 
Windows...but I suppose given they don't expose the LED outside of their Hello auth thing they can 
guarantee it's not being lit for too long - I confess that hadn't occurred to me. Anyway; I'll 
update this to re-trigger if the IC is in torch mode within the timeout (which the datasheet 
explicitly says you can do in torch mode; the current is much more heavily limited in that mode) and 
in the flash mode to update the brightness setting to 0 once the timeout expires so it reflects the 
actual state of the LED. Does that sound ok?

>
> BR,
> 										Pavel

Re: [PATCH 6/8] leds: tps68470: Support the WLED driver

[Hans de Goede]

Hi,

On 3/23/23 17:25, Dan Scally wrote:
> Hi Pavel, thanks for taking a look
> 
> On 23/03/2023 11:22, Pavel Machek wrote:
>> On Wed 2023-03-22 16:09:24, Daniel Scally wrote:
>>> The TPS68470 PMIC provides a third LED driver in addition to the two
>>> indicator LEDs. Add support for the WLED. To ensure the LED is active
>>> for as long as the kernel instructs it to be we need to re-trigger it
>>> periodically to avoid the IC's internal timeouts.
>> Wow. No!
>>
>> If hardware does not wart you to burn the LED, it is not okay to just
>> work around that. These are not designed for continuous operation.
>>
>>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
>>> index 44df175d25de..abcd3494b1a8 100644
>> Fun sha1 hash ;-).
> 
> 
> heh yeh
> 
>>
>>> @@ -52,11 +61,33 @@ enum ctrlb_current {
>>>       CTRLB_16MA    = 3,
>>>   };
>>>   +/*
>>> + * The WLED can operate in different modes, including a Flash and Torch mode. In
>>> + * each mode there's a timeout which ranges from a matter of milliseconds to up
>>> + * to 13 seconds. We don't want that timeout to apply though because the LED
>>> + * should be lit until we say that it should no longer be lit, re-trigger the
>>> + * LED periodically to keep it alive.
>>> + */
>> We don't want the LED to overheat. That takes precedence.
>>
>> Find out what are the maximum limits for on time at various current
>> levels. LED framework should be used for torch mode, with current set
>> such that unlimited operation is safe. V4L2 should be used for flash
>> mode.
> 
> 
> I did it this way because this is how the IC operates on my device whilst it's booted to Windows...but I suppose given they don't expose the LED outside of their Hello auth thing they can guarantee it's not being lit for too long - I confess that hadn't occurred to me. Anyway; I'll update this to re-trigger if the IC is in torch mode within the timeout (which the datasheet explicitly says you can do in torch mode; the current is much more heavily limited in that mode) and in the flash mode to update the brightness setting to 0 once the timeout expires so it reflects the actual state of the LED. Does that sound ok?

That sounds reasonable to me.

Regards,

Hans