[PATCH v5 RESEND 0/6] KTD2026 indicator LED for X86 Xiaomi Pad2

[PATCH v5 RESEND 0/6] KTD2026 indicator LED for X86 Xiaomi Pad2

[Kate Hsuan]

This patch added the support for Xiaomi Pad2 indicator LED. This work
included:
1. Added the KTD2026 swnode description to describe the LED controller.
2. Migrated the original driver to fwnode to support x86 platform.
3. Support for multi-color LED trigger event.
4. The LED will be red when charging and the LED will be green when the
   battery is full.

Moreover, the LED trigger is set to the new trigger, called
"bq27520-0-charging-red-full-green" for Xiaomi Pad2 so the LED will be
red when charging and the LED will be green when the battery is full.

The new LED API led_mc_trigger_event() can be found in the following
URL.
https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t

--
Changes in v5:
1. Fix swnode LED color settings.
2. Improve the driver based on the comments.
3. Introduce a LED new API- led_mc_trigger_event() to make the LED
   color can be changed according to the trigger.
4. Introduced a new trigger "charging-red-full-green". The LED will be
   red when charging and the the LED will be green when the battery is
   full.
5. Set the default trigger to "bq27520-0-charging-red-full-green" for
   Xiaomi Pad2.

Changes in v4:
1. Fix double casting.
2. Since force casting a pointer value to int will trigger a compiler
   warning, the type of num_leds was changed to unsigned long.

Changes in v3:
1. Drop the patch "leds-ktd202x: Skip regulator settings for Xiaomi
   pad2"

Changes in v2:
1. Typo and style fixes.
2. The patch 0003 skips all the regulator setup for Xiaomi pad2 since
   KTD2026 on Xiaomi pad2 is already powered by BP25890RTWR. So, the
   sleep can be removed when removing the module.

Hans de Goede (2):
  leds: core: Add led_mc_set_brightness() function
  leds: trigger: Add led_mc_trigger_event() function

Kate Hsuan (4):
  platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
    indicator LED
  leds: rgb: leds-ktd202x: Get device properties through fwnode to
    support ACPI
  power: supply: power-supply-leds: Add charging_red_full_green trigger
    for RGB LED
  platform: x86-android-tablets: others: Set the LED trigger to
    charging_red_full_green for Xiaomi pad2

 drivers/leds/led-class-multicolor.c           |  1 +
 drivers/leds/led-core.c                       | 31 +++++++
 drivers/leds/led-triggers.c                   | 20 +++++
 drivers/leds/rgb/Kconfig                      |  1 -
 drivers/leds/rgb/leds-ktd202x.c               | 75 ++++++++++-------
 .../platform/x86/x86-android-tablets/other.c  | 82 +++++++++++++++++++
 .../x86/x86-android-tablets/shared-psy-info.h |  2 +
 drivers/power/supply/power_supply_leds.c      | 25 ++++++
 include/linux/leds.h                          | 26 ++++++
 include/linux/power_supply.h                  |  2 +
 10 files changed, 235 insertions(+), 30 deletions(-)

-- 
2.44.0

[PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Kate Hsuan]

There is a KTD2026 LED controller to manage the indicator LED for Xiaomi
pad2. The ACPI for it is not properly made so the kernel can't get
a correct description of it.

This work add a description for this RGB LED controller and also set a
trigger to indicate the chaging event (bq27520-0-charging). When it is
charging, the indicator LED will be turn on.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../platform/x86/x86-android-tablets/other.c  | 82 +++++++++++++++++++
 .../x86/x86-android-tablets/shared-psy-info.h |  2 +
 2 files changed, 84 insertions(+)

diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index bc6bbf7ec6ea..1012a158f7b7 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -13,6 +13,8 @@
 #include <linux/input.h>
 #include <linux/platform_device.h>
 
+#include <dt-bindings/leds/common.h>
+
 #include "shared-psy-info.h"
 #include "x86-android-tablets.h"
 
@@ -593,6 +595,83 @@ const struct x86_dev_info whitelabel_tm800a550l_info __initconst = {
 	.gpiod_lookup_tables = whitelabel_tm800a550l_gpios,
 };
 
+/*
+ * The fwnode for ktd2026 on Xaomi pad2. It composed of a RGB LED node
+ * with three subnodes for each color (B/G/R). The RGB LED node is named
+ * "multi-led" to align with the name in the device tree.
+ */
+
+/* main fwnode for ktd2026 */
+static const struct software_node ktd2026_node = {
+	.name = "ktd2026"
+};
+
+static const struct property_entry ktd2026_rgb_led_props[] = {
+	PROPERTY_ENTRY_U32("reg", 0),
+	PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RGB),
+	PROPERTY_ENTRY_STRING("function", "indicator"),
+	PROPERTY_ENTRY_STRING("linux,default-trigger", "bq27520-0-charging"),
+	{ }
+};
+
+static const struct software_node ktd2026_rgb_led_node = {
+	.name = "multi-led",
+	.properties = ktd2026_rgb_led_props,
+	.parent = &ktd2026_node,
+};
+
+static const struct property_entry ktd2026_blue_led_props[] = {
+	PROPERTY_ENTRY_U32("reg", 0),
+	PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
+	{ }
+};
+
+static const struct software_node ktd2026_blue_led_node = {
+	.properties = ktd2026_blue_led_props,
+	.parent = &ktd2026_rgb_led_node,
+};
+
+static const struct property_entry ktd2026_green_led_props[] = {
+	PROPERTY_ENTRY_U32("reg", 1),
+	PROPERTY_ENTRY_U32("color", LED_COLOR_ID_GREEN),
+	{ }
+};
+
+static const struct software_node ktd2026_green_led_node = {
+	.properties = ktd2026_green_led_props,
+	.parent = &ktd2026_rgb_led_node,
+};
+
+static const struct property_entry ktd2026_red_led_props[] = {
+	PROPERTY_ENTRY_U32("reg", 2),
+	PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
+	{ }
+};
+
+static const struct software_node ktd2026_red_led_node = {
+	.properties = ktd2026_red_led_props,
+	.parent = &ktd2026_rgb_led_node,
+};
+
+static const struct software_node *ktd2026_node_group[] = {
+	&ktd2026_node,
+	&ktd2026_rgb_led_node,
+	&ktd2026_green_led_node,
+	&ktd2026_blue_led_node,
+	&ktd2026_red_led_node,
+	NULL
+};
+
+static int __init xiaomi_mipad2_init(void)
+{
+	return software_node_register_node_group(ktd2026_node_group);
+}
+
+static void xiaomi_mipad2_exit(void)
+{
+	software_node_unregister_node_group(ktd2026_node_group);
+}
+
 /*
  * If the EFI bootloader is not Xiaomi's own signed Android loader, then the
  * Xiaomi Mi Pad 2 X86 tablet sets OSID in the DSDT to 1 (Windows), causing
@@ -616,6 +695,7 @@ static const struct x86_i2c_client_info xiaomi_mipad2_i2c_clients[] __initconst
 			.type = "ktd2026",
 			.addr = 0x30,
 			.dev_name = "ktd2026",
+			.swnode = &ktd2026_node,
 		},
 		.adapter_path = "\\_SB_.PCI0.I2C3",
 	},
@@ -624,4 +704,6 @@ static const struct x86_i2c_client_info xiaomi_mipad2_i2c_clients[] __initconst
 const struct x86_dev_info xiaomi_mipad2_info __initconst = {
 	.i2c_client_info = xiaomi_mipad2_i2c_clients,
 	.i2c_client_count = ARRAY_SIZE(xiaomi_mipad2_i2c_clients),
+	.init = xiaomi_mipad2_init,
+	.exit = xiaomi_mipad2_exit,
 };
diff --git a/drivers/platform/x86/x86-android-tablets/shared-psy-info.h b/drivers/platform/x86/x86-android-tablets/shared-psy-info.h
index c2d2968cddc2..8c33ec47ee12 100644
--- a/drivers/platform/x86/x86-android-tablets/shared-psy-info.h
+++ b/drivers/platform/x86/x86-android-tablets/shared-psy-info.h
@@ -29,4 +29,6 @@ extern const char * const bq24190_modules[];
 extern const struct platform_device_info int3496_pdevs[];
 extern struct gpiod_lookup_table int3496_reference_gpios;
 
+extern const struct software_node ktd2026_leds_node;
+
 #endif
-- 
2.44.0

[PATCH v5 RESEND 2/6] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Kate Hsuan]

This LED controller also installed on a Xiaomi pad2 and it is a x86
platform. The original driver is based on device tree and can't be
used for this ACPI based system. This patch migrated the driver to
use fwnode to access the properties. Moreover, the fwnode API
supports device tree so this work won't effect the original
implementations.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/leds/rgb/Kconfig        |  1 -
 drivers/leds/rgb/leds-ktd202x.c | 75 ++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index e66bd21b9852..14d6b294a786 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -17,7 +17,6 @@ config LEDS_GROUP_MULTICOLOR
 config LEDS_KTD202X
 	tristate "LED support for KTD202x Chips"
 	depends on I2C
-	depends on OF
 	select REGMAP_I2C
 	help
 	  This option enables support for the Kinetic KTD2026/KTD2027
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 514965795a10..a75c300040c7 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -99,7 +99,7 @@ struct ktd202x {
 	struct device *dev;
 	struct regmap *regmap;
 	bool enabled;
-	int num_leds;
+	unsigned long num_leds;
 	struct ktd202x_led leds[] __counted_by(num_leds);
 };
 
@@ -381,16 +381,18 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
 				 mc->num_colors);
 }
 
-static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
+static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwnode,
 				 struct ktd202x_led *led, struct led_init_data *init_data)
 {
+	struct fwnode_handle *child;
 	struct led_classdev *cdev;
-	struct device_node *child;
 	struct mc_subled *info;
-	int num_channels;
+	int num_channels = 0;
 	int i = 0;
 
-	num_channels = of_get_available_child_count(np);
+	fwnode_for_each_available_child_node(fwnode, child) {
+		num_channels++;
+	}
 	if (!num_channels || num_channels > chip->num_leds)
 		return -EINVAL;
 
@@ -398,22 +400,22 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
 	if (!info)
 		return -ENOMEM;
 
-	for_each_available_child_of_node(np, child) {
+	fwnode_for_each_available_child_node(fwnode, child) {
 		u32 mono_color;
 		u32 reg;
 		int ret;
 
-		ret = of_property_read_u32(child, "reg", &reg);
+		ret = fwnode_property_read_u32(child, "reg", &reg);
 		if (ret != 0 || reg >= chip->num_leds) {
-			dev_err(chip->dev, "invalid 'reg' of %pOFn\n", child);
-			of_node_put(child);
-			return -EINVAL;
+			dev_err(chip->dev, "invalid 'reg' of %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
 		}
 
-		ret = of_property_read_u32(child, "color", &mono_color);
+		ret = fwnode_property_read_u32(child, "color", &mono_color);
 		if (ret < 0 && ret != -EINVAL) {
-			dev_err(chip->dev, "failed to parse 'color' of %pOF\n", child);
-			of_node_put(child);
+			dev_err(chip->dev, "failed to parse 'color' of %pfw\n", child);
+			fwnode_handle_put(child);
 			return ret;
 		}
 
@@ -433,16 +435,16 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
 	return devm_led_classdev_multicolor_register_ext(chip->dev, &led->mcdev, init_data);
 }
 
-static int ktd202x_setup_led_single(struct ktd202x *chip, struct device_node *np,
+static int ktd202x_setup_led_single(struct ktd202x *chip, struct fwnode_handle *fwnode,
 				    struct ktd202x_led *led, struct led_init_data *init_data)
 {
 	struct led_classdev *cdev;
 	u32 reg;
 	int ret;
 
-	ret = of_property_read_u32(np, "reg", &reg);
+	ret = fwnode_property_read_u32(fwnode, "reg", &reg);
 	if (ret != 0 || reg >= chip->num_leds) {
-		dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
+		dev_err(chip->dev, "invalid 'reg' of %pfw\n", fwnode);
 		return -EINVAL;
 	}
 	led->index = reg;
@@ -454,7 +456,9 @@ static int ktd202x_setup_led_single(struct ktd202x *chip, struct device_node *np
 	return devm_led_classdev_register_ext(chip->dev, &led->cdev, init_data);
 }
 
-static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigned int index)
+static int ktd202x_add_led(struct ktd202x *chip,
+			   struct fwnode_handle *fwnode_color,
+			   unsigned int index)
 {
 	struct ktd202x_led *led = &chip->leds[index];
 	struct led_init_data init_data = {};
@@ -463,21 +467,21 @@ static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigne
 	int ret;
 
 	/* Color property is optional in single color case */
-	ret = of_property_read_u32(np, "color", &color);
+	ret = fwnode_property_read_u32(fwnode_color, "color", &color);
 	if (ret < 0 && ret != -EINVAL) {
-		dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);
+		dev_err(chip->dev, "failed to parse 'color' of %pfw\n", fwnode_color);
 		return ret;
 	}
 
 	led->chip = chip;
-	init_data.fwnode = of_fwnode_handle(np);
+	init_data.fwnode = fwnode_color;
 
 	if (color == LED_COLOR_ID_RGB) {
 		cdev = &led->mcdev.led_cdev;
-		ret = ktd202x_setup_led_rgb(chip, np, led, &init_data);
+		ret = ktd202x_setup_led_rgb(chip, fwnode_color, led, &init_data);
 	} else {
 		cdev = &led->cdev;
-		ret = ktd202x_setup_led_single(chip, np, led, &init_data);
+		ret = ktd202x_setup_led_single(chip, fwnode_color, led, &init_data);
 	}
 
 	if (ret) {
@@ -492,26 +496,29 @@ static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigne
 
 static int ktd202x_probe_dt(struct ktd202x *chip)
 {
-	struct device_node *np = dev_of_node(chip->dev), *child;
+	struct fwnode_handle *child, *fwnode;
+	struct device *dev = chip->dev;
 	int count;
 	int i = 0;
 
-	chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
-
-	count = of_get_available_child_count(np);
+	count = device_get_child_node_count(dev);
 	if (!count || count > chip->num_leds)
 		return -EINVAL;
 
+	fwnode = dev_fwnode(chip->dev);
+	if (!fwnode)
+		return -ENODEV;
+
 	regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_RSTR_RESET);
 
 	/* Allow the device to execute the complete reset */
 	usleep_range(200, 300);
 
-	for_each_available_child_of_node(np, child) {
+	fwnode_for_each_available_child_node(fwnode, child) {
 		int ret = ktd202x_add_led(chip, child, i);
 
 		if (ret) {
-			of_node_put(child);
+			fwnode_handle_put(child);
 			return ret;
 		}
 		i++;
@@ -554,6 +561,8 @@ static int ktd202x_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	chip->num_leds = (unsigned long)i2c_get_match_data(client);
+
 	chip->regulators[0].supply = "vin";
 	chip->regulators[1].supply = "vio";
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
@@ -602,10 +611,17 @@ static void ktd202x_shutdown(struct i2c_client *client)
 	regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_RSTR_RESET);
 }
 
+static const struct i2c_device_id ktd202x_id[] = {
+	{"ktd2026", KTD2026_NUM_LEDS},
+	{"ktd2027", KTD2027_NUM_LEDS},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ktd202x_id);
+
 static const struct of_device_id ktd202x_match_table[] = {
 	{ .compatible = "kinetic,ktd2026", .data = (void *)KTD2026_NUM_LEDS },
 	{ .compatible = "kinetic,ktd2027", .data = (void *)KTD2027_NUM_LEDS },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(of, ktd202x_match_table);
 
@@ -617,6 +633,7 @@ static struct i2c_driver ktd202x_driver = {
 	.probe = ktd202x_probe,
 	.remove = ktd202x_remove,
 	.shutdown = ktd202x_shutdown,
+	.id_table = ktd202x_id,
 };
 module_i2c_driver(ktd202x_driver);
 
-- 
2.44.0

[PATCH v5 RESEND 3/6] leds: core: Add led_mc_set_brightness() function

[Kate Hsuan]

From: Hans de Goede <hdegoede@redhat.com>

Add a new led_mc_set_brightness() function for in kernel color/brightness
changing of multi-color LEDs.

led-class-multicolor can be build as a module and led_mc_set_brightness()
will have builtin callers, so put led_mc_set_brightness() inside led-core
instead, just like how led_set_brightness() is part of the core and not
of the led-class object.

This also adds a new LED_MULTI_COLOR led_classdev flag to allow
led_mc_set_brightness() to verify that it is operating on a multi-color
LED classdev, avoiding casting the passed in LED classdev to a multi-color
LED classdev, when it actually is not a multi-color LED.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-class-multicolor.c |  1 +
 drivers/leds/led-core.c             | 31 +++++++++++++++++++++++++++++
 include/linux/leds.h                | 20 +++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index ec62a4811613..df01c0e66c8b 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -134,6 +134,7 @@ int led_classdev_multicolor_register_ext(struct device *parent,
 		return -EINVAL;
 
 	led_cdev = &mcled_cdev->led_cdev;
+	led_cdev->flags |= LED_MULTI_COLOR;
 	mcled_cdev->led_cdev.groups = led_multicolor_groups;
 
 	return led_classdev_register_ext(parent, led_cdev, init_data);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 89c9806cc97f..5889753ebc74 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/led-class-multicolor.h>
 #include <linux/leds.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -362,6 +363,36 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
 
+/*
+ * This is a led-core function because just like led_set_brightness()
+ * it is used in kernel by e.g. triggers.
+ */
+void led_mc_set_brightness(struct led_classdev *led_cdev,
+			   unsigned int *intensity_value, unsigned int num_colors,
+			   unsigned int brightness)
+{
+	struct led_classdev_mc *mcled_cdev;
+	unsigned int i;
+
+	if (!(led_cdev->flags & LED_MULTI_COLOR)) {
+		dev_err_once(led_cdev->dev, "%s: error not a multi-color LED\n",  __func__);
+		return;
+	}
+
+	mcled_cdev = lcdev_to_mccdev(led_cdev);
+	if (num_colors != mcled_cdev->num_colors) {
+		dev_err_once(led_cdev->dev, "%s: error num_colors mismatch %d != %d\n",
+			     __func__, num_colors, mcled_cdev->num_colors);
+		return;
+	}
+
+	for (i = 0; i < mcled_cdev->num_colors; i++)
+		mcled_cdev->subled_info[i].intensity = intensity_value[i];
+
+	led_set_brightness(led_cdev, brightness);
+}
+EXPORT_SYMBOL_GPL(led_mc_set_brightness);
+
 int led_update_brightness(struct led_classdev *led_cdev)
 {
 	int ret;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 4754b02d3a2c..fed88eb9e170 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -115,6 +115,7 @@ struct led_classdev {
 #define LED_BRIGHT_HW_CHANGED	BIT(21)
 #define LED_RETAIN_AT_SHUTDOWN	BIT(22)
 #define LED_INIT_DEFAULT_TRIGGER BIT(23)
+#define LED_MULTI_COLOR		BIT(24)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
@@ -392,6 +393,25 @@ void led_set_brightness(struct led_classdev *led_cdev, unsigned int brightness);
  */
 int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value);
 
+/**
+ * led_mc_set_brightness - set mc LED color intensity values and brightness
+ * @led_cdev: the LED to set
+ * @intensity_value: array of per color intensity values to set
+ * @num_colors: amount of entries in intensity_value array
+ * @brightness: the brightness to set the LED to
+ *
+ * Set a multi-color LED's per color intensity values and brightness.
+ * If necessary, this cancels the software blink timer. This function is
+ * guaranteed not to sleep.
+ *
+ * Calling this function on a non multi-color led_classdev or with the wrong
+ * num_colors value is an error. In this case an error will be logged once
+ * and the call will do nothing.
+ */
+void led_mc_set_brightness(struct led_classdev *led_cdev,
+			   unsigned int *intensity_value, unsigned int num_colors,
+			   unsigned int brightness);
+
 /**
  * led_update_brightness - update LED brightness
  * @led_cdev: the LED to query
-- 
2.44.0

[PATCH v5 RESEND 4/6] leds: trigger: Add led_mc_trigger_event() function

[Kate Hsuan]

From: Hans de Goede <hdegoede@redhat.com>

Add a new led_mc_trigger_event() function for triggers which want to
change the color of a multi-color LED based on their trigger conditions.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-triggers.c | 20 ++++++++++++++++++++
 include/linux/leds.h        |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index bd59a14a4a90..fcc4e7a7b12b 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -380,6 +380,26 @@ void led_trigger_event(struct led_trigger *trig,
 }
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
+void led_mc_trigger_event(struct led_trigger *trig,
+			  unsigned int *intensity_value, unsigned int num_colors,
+			  enum led_brightness brightness)
+{
+	struct led_classdev *led_cdev;
+
+	if (!trig)
+		return;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
+		if (!(led_cdev->flags & LED_MULTI_COLOR))
+			continue;
+
+		led_mc_set_brightness(led_cdev, intensity_value, num_colors, brightness);
+	}
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(led_mc_trigger_event);
+
 static void led_trigger_blink_setup(struct led_trigger *trig,
 			     unsigned long delay_on,
 			     unsigned long delay_off,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index fed88eb9e170..5378e4cd03ff 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -526,6 +526,9 @@ void led_trigger_register_simple(const char *name,
 				struct led_trigger **trigger);
 void led_trigger_unregister_simple(struct led_trigger *trigger);
 void led_trigger_event(struct led_trigger *trigger,  enum led_brightness event);
+void led_mc_trigger_event(struct led_trigger *trig,
+			  unsigned int *intensity_value, unsigned int num_colors,
+			  enum led_brightness brightness);
 void led_trigger_blink(struct led_trigger *trigger, unsigned long delay_on,
 		       unsigned long delay_off);
 void led_trigger_blink_oneshot(struct led_trigger *trigger,
@@ -562,6 +565,9 @@ static inline void led_trigger_register_simple(const char *name,
 static inline void led_trigger_unregister_simple(struct led_trigger *trigger) {}
 static inline void led_trigger_event(struct led_trigger *trigger,
 				enum led_brightness event) {}
+static inline void led_mc_trigger_event(struct led_trigger *trig,
+				unsigned int *intensity_value, unsigned int num_colors,
+				enum led_brightness brightness) {}
 static inline void led_trigger_blink(struct led_trigger *trigger,
 				      unsigned long delay_on,
 				      unsigned long delay_off) {}
-- 
2.44.0

[PATCH v5 RESEND 5/6] power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED

[Kate Hsuan]

Add a charging_red_full_green LED trigger and the trigger is based on
led_mc_trigger_event() which can set an RGB LED when the trigger is
triggered. The LED will show red when the battery status is charging.
The LED will show green when the battery status is full.

Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t
Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/power/supply/power_supply_leds.c | 25 ++++++++++++++++++++++++
 include/linux/power_supply.h             |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
index c7db29d5fcb8..bd9c8fec5870 100644
--- a/drivers/power/supply/power_supply_leds.c
+++ b/drivers/power/supply/power_supply_leds.c
@@ -22,6 +22,8 @@
 static void power_supply_update_bat_leds(struct power_supply *psy)
 {
 	union power_supply_propval status;
+	unsigned int intensity_green[3] = {255, 0, 0};
+	unsigned int intensity_red[3] = {0, 0, 255};
 
 	if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status))
 		return;
@@ -36,12 +38,20 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
 		/* Going from blink to LED on requires a LED_OFF event to stop blink */
 		led_trigger_event(psy->charging_blink_full_solid_trig, LED_OFF);
 		led_trigger_event(psy->charging_blink_full_solid_trig, LED_FULL);
+		led_mc_trigger_event(psy->charging_red_full_green_trig,
+				     intensity_green,
+				     3,
+				     LED_FULL);
 		break;
 	case POWER_SUPPLY_STATUS_CHARGING:
 		led_trigger_event(psy->charging_full_trig, LED_FULL);
 		led_trigger_event(psy->charging_trig, LED_FULL);
 		led_trigger_event(psy->full_trig, LED_OFF);
 		led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0);
+		led_mc_trigger_event(psy->charging_red_full_green_trig,
+				     intensity_red,
+				     3,
+				     LED_FULL);
 		break;
 	default:
 		led_trigger_event(psy->charging_full_trig, LED_OFF);
@@ -49,6 +59,10 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
 		led_trigger_event(psy->full_trig, LED_OFF);
 		led_trigger_event(psy->charging_blink_full_solid_trig,
 			LED_OFF);
+		led_mc_trigger_event(psy->charging_red_full_green_trig,
+				     intensity_red,
+				     3,
+				     LED_OFF);
 		break;
 	}
 }
@@ -74,6 +88,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
 	if (!psy->charging_blink_full_solid_trig_name)
 		goto charging_blink_full_solid_failed;
 
+	psy->charging_red_full_green_trig_name = kasprintf(GFP_KERNEL,
+		"%s-charging-red-full-green", psy->desc->name);
+	if (!psy->charging_red_full_green_trig_name)
+		goto charging_red_full_green_failed;
+
 	led_trigger_register_simple(psy->charging_full_trig_name,
 				    &psy->charging_full_trig);
 	led_trigger_register_simple(psy->charging_trig_name,
@@ -82,9 +101,13 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
 				    &psy->full_trig);
 	led_trigger_register_simple(psy->charging_blink_full_solid_trig_name,
 				    &psy->charging_blink_full_solid_trig);
+	led_trigger_register_simple(psy->charging_red_full_green_trig_name,
+				    &psy->charging_red_full_green_trig);
 
 	return 0;
 
+charging_red_full_green_failed:
+	kfree(psy->charging_blink_full_solid_trig_name);
 charging_blink_full_solid_failed:
 	kfree(psy->full_trig_name);
 full_failed:
@@ -101,10 +124,12 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy)
 	led_trigger_unregister_simple(psy->charging_trig);
 	led_trigger_unregister_simple(psy->full_trig);
 	led_trigger_unregister_simple(psy->charging_blink_full_solid_trig);
+	led_trigger_unregister_simple(psy->charging_red_full_green_trig);
 	kfree(psy->charging_blink_full_solid_trig_name);
 	kfree(psy->full_trig_name);
 	kfree(psy->charging_trig_name);
 	kfree(psy->charging_full_trig_name);
+	kfree(psy->charging_red_full_green_trig_name);
 }
 
 /* Generated power specific LEDs triggers. */
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c0992a77feea..1d7c0b43070f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -318,6 +318,8 @@ struct power_supply {
 	char *online_trig_name;
 	struct led_trigger *charging_blink_full_solid_trig;
 	char *charging_blink_full_solid_trig_name;
+	struct led_trigger *charging_red_full_green_trig;
+	char *charging_red_full_green_trig_name;
 #endif
 };
 
-- 
2.44.0

[PATCH v5 RESEND 6/6] platform: x86-android-tablets: others: Set the LED trigger to charging_red_full_green for Xiaomi pad2

[Kate Hsuan]

Set the default trigger to bq27520-0-charging-red-full-green. The LED
will show red when the battery is charging. The LED will show green
when the battery status is full.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/platform/x86/x86-android-tablets/other.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index 1012a158f7b7..eccfea7b01c0 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -610,7 +610,7 @@ static const struct property_entry ktd2026_rgb_led_props[] = {
 	PROPERTY_ENTRY_U32("reg", 0),
 	PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RGB),
 	PROPERTY_ENTRY_STRING("function", "indicator"),
-	PROPERTY_ENTRY_STRING("linux,default-trigger", "bq27520-0-charging"),
+	PROPERTY_ENTRY_STRING("linux,default-trigger", "bq27520-0-charging-red-full-green"),
 	{ }
 };
 
-- 
2.44.0

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Andy Shevchenko]

On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> There is a KTD2026 LED controller to manage the indicator LED for Xiaomi
> pad2. The ACPI for it is not properly made so the kernel can't get
> a correct description of it.
>
> This work add a description for this RGB LED controller and also set a

adds
sets

> trigger to indicate the chaging event (bq27520-0-charging). When it is

charging

> charging, the indicator LED will be turn on.

turned

...

> +/* main fwnode for ktd2026 */
> +static const struct software_node ktd2026_node = {
> +       .name = "ktd2026"

Leave a comma, this is not a terminator.

> +};

When I asked about the name I relied on the fact that you have an idea
how it works. So, assuming my understanding is correct, this platform
may not have more than a single LED of this type. Dunno if we need a
comment about this.

...

> +static int __init xiaomi_mipad2_init(void)
> +{
> +       return software_node_register_node_group(ktd2026_node_group);
> +}
> +
> +static void xiaomi_mipad2_exit(void)

__exit ?

> +{
> +       software_node_unregister_node_group(ktd2026_node_group);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 2/6] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Andy Shevchenko]

On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> This LED controller also installed on a Xiaomi pad2 and it is a x86
> platform. The original driver is based on device tree and can't be

the device

> used for this ACPI based system. This patch migrated the driver to
> use fwnode to access the properties. Moreover, the fwnode API
> supports device tree so this work won't effect the original

affect

> implementations.

...

> +       fwnode_for_each_available_child_node(fwnode, child) {
> +               num_channels++;
> +       }

{} are not needed.

>         if (!num_channels || num_channels > chip->num_leds)
>                 return -EINVAL;

...

> +static int ktd202x_add_led(struct ktd202x *chip,
> +                          struct fwnode_handle *fwnode_color,

Can it be simply fwnode? (Originally it was np, so I assume there is
no name collision)

...

> +       count = device_get_child_node_count(dev);
>         if (!count || count > chip->num_leds)
>                 return -EINVAL;

> +       fwnode = dev_fwnode(chip->dev);

Why not dev?

> +       if (!fwnode)
> +               return -ENODEV;

This is dead code. Please remove these three lines.

...

> +       .id_table = ktd202x_id,

Seems to me that you may split the I²C ID table addition into a separate change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 3/6] leds: core: Add led_mc_set_brightness() function

[Andy Shevchenko]

On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> From: Hans de Goede <hdegoede@redhat.com>
>
> Add a new led_mc_set_brightness() function for in kernel color/brightness
> changing of multi-color LEDs.
>
> led-class-multicolor can be build as a module and led_mc_set_brightness()
> will have builtin callers, so put led_mc_set_brightness() inside led-core

the builtin

> instead, just like how led_set_brightness() is part of the core and not
> of the led-class object.
>
> This also adds a new LED_MULTI_COLOR led_classdev flag to allow
> led_mc_set_brightness() to verify that it is operating on a multi-color
> LED classdev, avoiding casting the passed in LED classdev to a multi-color
> LED classdev, when it actually is not a multi-color LED.

...

> +/*
> + * This is a led-core function because just like led_set_brightness()
> + * it is used in kernel by e.g. triggers.

in the kernel

> + */

...

> +       if (!(led_cdev->flags & LED_MULTI_COLOR)) {
> +               dev_err_once(led_cdev->dev, "%s: error not a multi-color LED\n",  __func__);

Not sure how __func__ helps here.

> +               return;
> +       }
> +
> +       mcled_cdev = lcdev_to_mccdev(led_cdev);
> +       if (num_colors != mcled_cdev->num_colors) {
> +               dev_err_once(led_cdev->dev, "%s: error num_colors mismatch %d != %d\n",

Should be '...%u != %u...'.

> +                            __func__, num_colors, mcled_cdev->num_colors);

Ditto about __func__.

> +               return;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 5/6] power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED

[Andy Shevchenko]

On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> Add a charging_red_full_green LED trigger and the trigger is based on
> led_mc_trigger_event() which can set an RGB LED when the trigger is
> triggered. The LED will show red when the battery status is charging.
> The LED will show green when the battery status is full.
>
> Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t

You can drop the 'T/#t' part.

...

> +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> +                                    intensity_green,
> +                                    3,

ARRAY_SIZE()

> +                                    LED_FULL);

...

> +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> +                                    intensity_red,
> +                                    3,

Ditto.

> +                                    LED_FULL);

...

> +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> +                                    intensity_red,
> +                                    3,

Ditto.

> +                                    LED_OFF);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 5/6] power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED

[Kate Hsuan]

Hi Andy,

On Mon, Mar 25, 2024 at 4:11 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
> >
> > Add a charging_red_full_green LED trigger and the trigger is based on
> > led_mc_trigger_event() which can set an RGB LED when the trigger is
> > triggered. The LED will show red when the battery status is charging.
> > The LED will show green when the battery status is full.
> >
> > Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t
>
> You can drop the 'T/#t' part.
>
> ...
>
> > +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                    intensity_green,
> > +                                    3,
>
> ARRAY_SIZE()
>
> > +                                    LED_FULL);
>
> ...
>
> > +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                    intensity_red,
> > +                                    3,
>
> Ditto.
>
> > +                                    LED_FULL);
>
> ...
>
> > +               led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                    intensity_red,
> > +                                    3,
>
> Ditto.
>
> > +                                    LED_OFF);
>
> --
> With Best Regards,
> Andy Shevchenko
>

Thank you for reviewing it.
I'll fix it in the v6 patch.

-- 
BR,
Kate

Re: [PATCH v5 RESEND 2/6] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Kate Hsuan]

Hi Andy,

Thank you for reviewing it.

On Mon, Mar 25, 2024 at 3:57 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
> >
> > This LED controller also installed on a Xiaomi pad2 and it is a x86
> > platform. The original driver is based on device tree and can't be
>
> the device
>
> > used for this ACPI based system. This patch migrated the driver to
> > use fwnode to access the properties. Moreover, the fwnode API
> > supports device tree so this work won't effect the original
>
> affect
>
> > implementations.
>
> ...
>
> > +       fwnode_for_each_available_child_node(fwnode, child) {
> > +               num_channels++;
> > +       }
>
> {} are not needed.
>
> >         if (!num_channels || num_channels > chip->num_leds)
> >                 return -EINVAL;
>
> ...
>
> > +static int ktd202x_add_led(struct ktd202x *chip,
> > +                          struct fwnode_handle *fwnode_color,
>
> Can it be simply fwnode? (Originally it was np, so I assume there is
> no name collision)
It can be. I'll revise this.

>
> ...
>
> > +       count = device_get_child_node_count(dev);
> >         if (!count || count > chip->num_leds)
> >                 return -EINVAL;
>
> > +       fwnode = dev_fwnode(chip->dev);
>
> Why not dev?
I'll use dev. I had declared it.

>
> > +       if (!fwnode)
> > +               return -ENODEV;
>
> This is dead code. Please remove these three lines.

Okay.

>
> ...
>
> > +       .id_table = ktd202x_id,
>
> Seems to me that you may split the I²C ID table addition into a separate change.

Could you please describe this more clearly? Thank you

>
> --
> With Best Regards,
> Andy Shevchenko
>

I'll propose the v6 patch according to your comments.


-- 
BR,
Kate

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Kate Hsuan]

Hi Andy,

Thank you for reviewing it.

On Mon, Mar 25, 2024 at 3:30 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
> >
> > There is a KTD2026 LED controller to manage the indicator LED for Xiaomi
> > pad2. The ACPI for it is not properly made so the kernel can't get
> > a correct description of it.
> >
> > This work add a description for this RGB LED controller and also set a
>
> adds
> sets
>
> > trigger to indicate the chaging event (bq27520-0-charging). When it is
>
> charging
>
> > charging, the indicator LED will be turn on.
>
> turned
>
> ...
>
> > +/* main fwnode for ktd2026 */
> > +static const struct software_node ktd2026_node = {
> > +       .name = "ktd2026"
>
> Leave a comma, this is not a terminator.
>
> > +};
>
> When I asked about the name I relied on the fact that you have an idea
> how it works. So, assuming my understanding is correct, this platform
> may not have more than a single LED of this type. Dunno if we need a
> comment about this.

I'll make a comment to describe the configuration.
This LED controller can be configured to an RGB LED like this. Also,
it can be configured as three single-color (RGB) LEDs to show red,
green, and blue only.
I think the name can be "ktd2026-multi-color". Is it good for you?


>
> ...
>
> > +static int __init xiaomi_mipad2_init(void)
> > +{
> > +       return software_node_register_node_group(ktd2026_node_group);
> > +}
> > +
> > +static void xiaomi_mipad2_exit(void)
>
> __exit ?
No need.
x86-andriod-tablet is based on platform_driver and platform_device so
it doesn't need __exit.

I put __exit and the compiler complained about the warning.
===
WARNING: modpost:
drivers/platform/x86/x86-android-tablets/x86-android-tablets: section
mismatch in reference: xiaomi_mipad2_info+0x50 (section: .init.rodata)
-> xiaomi_mipad2_exit (section: .exit.text)
===
>
> > +{
> > +       software_node_unregister_node_group(ktd2026_node_group);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>

I'll propose the v6 patch to fix them according to your comments.

--
BR,
Kate

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Andy Shevchenko]

On Wed, Mar 27, 2024 at 9:58 AM Kate Hsuan <hpa@redhat.com> wrote:
> On Mon, Mar 25, 2024 at 3:30 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:

...

> > > +/* main fwnode for ktd2026 */
> > > +static const struct software_node ktd2026_node = {
> > > +       .name = "ktd2026"
> >
> > Leave a comma, this is not a terminator.
> >
> > > +};
> >
> > When I asked about the name I relied on the fact that you have an idea
> > how it works. So, assuming my understanding is correct, this platform
> > may not have more than a single LED of this type. Dunno if we need a
> > comment about this.
>
> I'll make a comment to describe the configuration.
> This LED controller can be configured to an RGB LED like this. Also,
> it can be configured as three single-color (RGB) LEDs to show red,
> green, and blue only.
> I think the name can be "ktd2026-multi-color". Is it good for you?

My point here is that the name is static and if you have more than one
LED in the system, the second one won't be registered due to sysfs
name collisions. Question here is how many of these types of LEDs are
possible on the platform? If more than one, the name has to be
dropped. Writing this I think a comment would be good to have in any
case.

...

> > > +static int __init xiaomi_mipad2_init(void)
> > > +{
> > > +       return software_node_register_node_group(ktd2026_node_group);
> > > +}
> > > +
> > > +static void xiaomi_mipad2_exit(void)
> >
> > __exit ?
> No need.
> x86-andriod-tablet is based on platform_driver and platform_device so
> it doesn't need __exit.
>
> I put __exit and the compiler complained about the warning.
> ===
> WARNING: modpost:
> drivers/platform/x86/x86-android-tablets/x86-android-tablets: section
> mismatch in reference: xiaomi_mipad2_info+0x50 (section: .init.rodata)
> -> xiaomi_mipad2_exit (section: .exit.text)
> ===

This is interesting. Why then do we call them symmetrically?

Hans, do we need to have anything here been amended?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 2/6] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Andy Shevchenko]

On Wed, Mar 27, 2024 at 8:53 AM Kate Hsuan <hpa@redhat.com> wrote:
> On Mon, Mar 25, 2024 at 3:57 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:

...

> > > +       .id_table = ktd202x_id,
> >
> > Seems to me that you may split the I²C ID table addition into a separate change.
>
> Could you please describe this more clearly? Thank you

I don't see how the introduction of the I²C ID table is related to
this patch. If needed it can be done separately, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Hans de Goede]

Hi,

On 3/27/24 12:05 PM, Andy Shevchenko wrote:
> On Wed, Mar 27, 2024 at 9:58 AM Kate Hsuan <hpa@redhat.com> wrote:
>> On Mon, Mar 25, 2024 at 3:30 AM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
> 
> ...
> 
>>>> +/* main fwnode for ktd2026 */
>>>> +static const struct software_node ktd2026_node = {
>>>> +       .name = "ktd2026"
>>>
>>> Leave a comma, this is not a terminator.
>>>
>>>> +};
>>>
>>> When I asked about the name I relied on the fact that you have an idea
>>> how it works. So, assuming my understanding is correct, this platform
>>> may not have more than a single LED of this type. Dunno if we need a
>>> comment about this.
>>
>> I'll make a comment to describe the configuration.
>> This LED controller can be configured to an RGB LED like this. Also,
>> it can be configured as three single-color (RGB) LEDs to show red,
>> green, and blue only.
>> I think the name can be "ktd2026-multi-color". Is it good for you?
> 
> My point here is that the name is static and if you have more than one
> LED in the system, the second one won't be registered due to sysfs
> name collisions. Question here is how many of these types of LEDs are
> possible on the platform? If more than one, the name has to be
> dropped. Writing this I think a comment would be good to have in any
> case.
> 
> ...
> 
>>>> +static int __init xiaomi_mipad2_init(void)
>>>> +{
>>>> +       return software_node_register_node_group(ktd2026_node_group);
>>>> +}
>>>> +
>>>> +static void xiaomi_mipad2_exit(void)
>>>
>>> __exit ?
>> No need.
>> x86-andriod-tablet is based on platform_driver and platform_device so
>> it doesn't need __exit.
>>
>> I put __exit and the compiler complained about the warning.
>> ===
>> WARNING: modpost:
>> drivers/platform/x86/x86-android-tablets/x86-android-tablets: section
>> mismatch in reference: xiaomi_mipad2_info+0x50 (section: .init.rodata)
>> -> xiaomi_mipad2_exit (section: .exit.text)
>> ===
> 
> This is interesting. Why then do we call them symmetrically?
> 
> Hans, do we need to have anything here been amended?

No this is all as expected.

The platform driver's probe() function can be __init because
the platform device is registered with the special:
platform_create_bundle() function which takes a probe() function
and the actual "struct platform_driver" does not have .probe
set at all.

Since we need to do manual cleanup on remove() however we need
a remove() callback and that does sit in the struct platform_driver
and since remove() can normally also be called on manual unbind
of the driver through sysfs it cannot be in the __exit section.

I say normally because IIRC platform_create_bundle() disables
manual unbinding but the section checking code does not know this,
all it sees is that the "struct platform_driver" is not __exit
and that it references the remove() callback and therefor the
remove() callback itself cannot be __exit.

Regards,

Hans

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Andy Shevchenko]

On Wed, Mar 27, 2024 at 1:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 3/27/24 12:05 PM, Andy Shevchenko wrote:
> > On Wed, Mar 27, 2024 at 9:58 AM Kate Hsuan <hpa@redhat.com> wrote:
> >> On Mon, Mar 25, 2024 at 3:30 AM Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >>> On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:

...

> >>>> +static int __init xiaomi_mipad2_init(void)
> >>>> +{
> >>>> +       return software_node_register_node_group(ktd2026_node_group);
> >>>> +}
> >>>> +
> >>>> +static void xiaomi_mipad2_exit(void)
> >>>
> >>> __exit ?
> >> No need.
> >> x86-andriod-tablet is based on platform_driver and platform_device so
> >> it doesn't need __exit.
> >>
> >> I put __exit and the compiler complained about the warning.
> >> ===
> >> WARNING: modpost:
> >> drivers/platform/x86/x86-android-tablets/x86-android-tablets: section
> >> mismatch in reference: xiaomi_mipad2_info+0x50 (section: .init.rodata)
> >> -> xiaomi_mipad2_exit (section: .exit.text)
> >> ===
> >
> > This is interesting. Why then do we call them symmetrically?
> >
> > Hans, do we need to have anything here been amended?
>
> No this is all as expected.
>
> The platform driver's probe() function can be __init because
> the platform device is registered with the special:
> platform_create_bundle() function which takes a probe() function
> and the actual "struct platform_driver" does not have .probe
> set at all.
>
> Since we need to do manual cleanup on remove() however we need
> a remove() callback and that does sit in the struct platform_driver
> and since remove() can normally also be called on manual unbind
> of the driver through sysfs it cannot be in the __exit section.
>
> I say normally because IIRC platform_create_bundle() disables
> manual unbinding but the section checking code does not know this,
> all it sees is that the "struct platform_driver" is not __exit
> and that it references the remove() callback and therefor the
> remove() callback itself cannot be __exit.

Thank you for the detailed explanation!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 RESEND 1/6] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED

[Kate Hsuan]

On Wed, Mar 27, 2024 at 7:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 9:58 AM Kate Hsuan <hpa@redhat.com> wrote:
> > On Mon, Mar 25, 2024 at 3:30 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> ...
>
> > > > +/* main fwnode for ktd2026 */
> > > > +static const struct software_node ktd2026_node = {
> > > > +       .name = "ktd2026"
> > >
> > > Leave a comma, this is not a terminator.
> > >
> > > > +};
> > >
> > > When I asked about the name I relied on the fact that you have an idea
> > > how it works. So, assuming my understanding is correct, this platform
> > > may not have more than a single LED of this type. Dunno if we need a
> > > comment about this.
> >
> > I'll make a comment to describe the configuration.
> > This LED controller can be configured to an RGB LED like this. Also,
> > it can be configured as three single-color (RGB) LEDs to show red,
> > green, and blue only.
> > I think the name can be "ktd2026-multi-color". Is it good for you?
>
> My point here is that the name is static and if you have more than one
> LED in the system, the second one won't be registered due to sysfs
> name collisions. Question here is how many of these types of LEDs are
> possible on the platform? If more than one, the name has to be
> dropped. Writing this I think a comment would be good to have in any
> case.

Only one RGB LED controller on this platform so we can name it. I also
tested the LED with and without the name and the LED worked properly.
I think the name can be dropped and put a comment there to describe
the usage and configuration of the LED controller.

Thank you :)
>
> ...
>
> > > > +static int __init xiaomi_mipad2_init(void)
> > > > +{
> > > > +       return software_node_register_node_group(ktd2026_node_group);
> > > > +}
> > > > +
> > > > +static void xiaomi_mipad2_exit(void)
> > >
> > > __exit ?
> > No need.
> > x86-andriod-tablet is based on platform_driver and platform_device so
> > it doesn't need __exit.
> >
> > I put __exit and the compiler complained about the warning.
> > ===
> > WARNING: modpost:
> > drivers/platform/x86/x86-android-tablets/x86-android-tablets: section
> > mismatch in reference: xiaomi_mipad2_info+0x50 (section: .init.rodata)
> > -> xiaomi_mipad2_exit (section: .exit.text)
> > ===
>
> This is interesting. Why then do we call them symmetrically?
>
> Hans, do we need to have anything here been amended?
>
> --
> With Best Regards,
> Andy Shevchenko
>


-- 
BR,
Kate

Re: [PATCH v5 RESEND 2/6] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI

[Kate Hsuan]

On Wed, Mar 27, 2024 at 7:08 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 8:53 AM Kate Hsuan <hpa@redhat.com> wrote:
> > On Mon, Mar 25, 2024 at 3:57 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Mar 24, 2024 at 5:02 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> ...
>
> > > > +       .id_table = ktd202x_id,
> > >
> > > Seems to me that you may split the I²C ID table addition into a separate change.
> >
> > Could you please describe this more clearly? Thank you
>
> I don't see how the introduction of the I²C ID table is related to
> this patch. If needed it can be done separately, no?

Okay. I'll prepare a separate patch to describe the i2c ID table.

Thank you :)

>
> --
> With Best Regards,
> Andy Shevchenko
>


-- 
BR,
Kate

Re: [PATCH v5 RESEND 5/6] power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED

[Sebastian Reichel]

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

Hello Kate,

On Sun, Mar 24, 2024 at 11:01:06PM +0800, Kate Hsuan wrote:
> Add a charging_red_full_green LED trigger and the trigger is based on
> led_mc_trigger_event() which can set an RGB LED when the trigger is
> triggered. The LED will show red when the battery status is charging.
> The LED will show green when the battery status is full.
> 
> Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---

Have you considered using orange instead of red? Using orange as
charging indicator seems to be more common nowadays and allows

green  = battery full
orange = battery charging
red    = battery empty / battery dead / error

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply_leds.c | 25 ++++++++++++++++++++++++
>  include/linux/power_supply.h             |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
> index c7db29d5fcb8..bd9c8fec5870 100644
> --- a/drivers/power/supply/power_supply_leds.c
> +++ b/drivers/power/supply/power_supply_leds.c
> @@ -22,6 +22,8 @@
>  static void power_supply_update_bat_leds(struct power_supply *psy)
>  {
>  	union power_supply_propval status;
> +	unsigned int intensity_green[3] = {255, 0, 0};
> +	unsigned int intensity_red[3] = {0, 0, 255};
>  
>  	if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status))
>  		return;
> @@ -36,12 +38,20 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
>  		/* Going from blink to LED on requires a LED_OFF event to stop blink */
>  		led_trigger_event(psy->charging_blink_full_solid_trig, LED_OFF);
>  		led_trigger_event(psy->charging_blink_full_solid_trig, LED_FULL);
> +		led_mc_trigger_event(psy->charging_red_full_green_trig,
> +				     intensity_green,
> +				     3,
> +				     LED_FULL);
>  		break;
>  	case POWER_SUPPLY_STATUS_CHARGING:
>  		led_trigger_event(psy->charging_full_trig, LED_FULL);
>  		led_trigger_event(psy->charging_trig, LED_FULL);
>  		led_trigger_event(psy->full_trig, LED_OFF);
>  		led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0);
> +		led_mc_trigger_event(psy->charging_red_full_green_trig,
> +				     intensity_red,
> +				     3,
> +				     LED_FULL);
>  		break;
>  	default:
>  		led_trigger_event(psy->charging_full_trig, LED_OFF);
> @@ -49,6 +59,10 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
>  		led_trigger_event(psy->full_trig, LED_OFF);
>  		led_trigger_event(psy->charging_blink_full_solid_trig,
>  			LED_OFF);
> +		led_mc_trigger_event(psy->charging_red_full_green_trig,
> +				     intensity_red,
> +				     3,
> +				     LED_OFF);
>  		break;
>  	}
>  }
> @@ -74,6 +88,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
>  	if (!psy->charging_blink_full_solid_trig_name)
>  		goto charging_blink_full_solid_failed;
>  
> +	psy->charging_red_full_green_trig_name = kasprintf(GFP_KERNEL,
> +		"%s-charging-red-full-green", psy->desc->name);
> +	if (!psy->charging_red_full_green_trig_name)
> +		goto charging_red_full_green_failed;
> +
>  	led_trigger_register_simple(psy->charging_full_trig_name,
>  				    &psy->charging_full_trig);
>  	led_trigger_register_simple(psy->charging_trig_name,
> @@ -82,9 +101,13 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
>  				    &psy->full_trig);
>  	led_trigger_register_simple(psy->charging_blink_full_solid_trig_name,
>  				    &psy->charging_blink_full_solid_trig);
> +	led_trigger_register_simple(psy->charging_red_full_green_trig_name,
> +				    &psy->charging_red_full_green_trig);
>  
>  	return 0;
>  
> +charging_red_full_green_failed:
> +	kfree(psy->charging_blink_full_solid_trig_name);
>  charging_blink_full_solid_failed:
>  	kfree(psy->full_trig_name);
>  full_failed:
> @@ -101,10 +124,12 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy)
>  	led_trigger_unregister_simple(psy->charging_trig);
>  	led_trigger_unregister_simple(psy->full_trig);
>  	led_trigger_unregister_simple(psy->charging_blink_full_solid_trig);
> +	led_trigger_unregister_simple(psy->charging_red_full_green_trig);
>  	kfree(psy->charging_blink_full_solid_trig_name);
>  	kfree(psy->full_trig_name);
>  	kfree(psy->charging_trig_name);
>  	kfree(psy->charging_full_trig_name);
> +	kfree(psy->charging_red_full_green_trig_name);
>  }
>  
>  /* Generated power specific LEDs triggers. */
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index c0992a77feea..1d7c0b43070f 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -318,6 +318,8 @@ struct power_supply {
>  	char *online_trig_name;
>  	struct led_trigger *charging_blink_full_solid_trig;
>  	char *charging_blink_full_solid_trig_name;
> +	struct led_trigger *charging_red_full_green_trig;
> +	char *charging_red_full_green_trig_name;
>  #endif
>  };
>  
> -- 
> 2.44.0
> 
> 

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

Re: [PATCH v5 RESEND 5/6] power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED

[Kate Hsuan]

Hi

On Sat, Mar 30, 2024 at 12:24 AM Sebastian Reichel <sre@kernel.org> wrote:
>
> Hello Kate,
>
> On Sun, Mar 24, 2024 at 11:01:06PM +0800, Kate Hsuan wrote:
> > Add a charging_red_full_green LED trigger and the trigger is based on
> > led_mc_trigger_event() which can set an RGB LED when the trigger is
> > triggered. The LED will show red when the battery status is charging.
> > The LED will show green when the battery status is full.
> >
> > Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
>
> Have you considered using orange instead of red? Using orange as
> charging indicator seems to be more common nowadays and allows

Sounds good.
I'll change the color for them.

Thank you

>
> green  = battery full
> orange = battery charging
> red    = battery empty / battery dead / error
>
> Greetings,
>
> -- Sebastian
>
> >  drivers/power/supply/power_supply_leds.c | 25 ++++++++++++++++++++++++
> >  include/linux/power_supply.h             |  2 ++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
> > index c7db29d5fcb8..bd9c8fec5870 100644
> > --- a/drivers/power/supply/power_supply_leds.c
> > +++ b/drivers/power/supply/power_supply_leds.c
> > @@ -22,6 +22,8 @@
> >  static void power_supply_update_bat_leds(struct power_supply *psy)
> >  {
> >       union power_supply_propval status;
> > +     unsigned int intensity_green[3] = {255, 0, 0};
> > +     unsigned int intensity_red[3] = {0, 0, 255};
> >
> >       if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status))
> >               return;
> > @@ -36,12 +38,20 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
> >               /* Going from blink to LED on requires a LED_OFF event to stop blink */
> >               led_trigger_event(psy->charging_blink_full_solid_trig, LED_OFF);
> >               led_trigger_event(psy->charging_blink_full_solid_trig, LED_FULL);
> > +             led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                  intensity_green,
> > +                                  3,
> > +                                  LED_FULL);
> >               break;
> >       case POWER_SUPPLY_STATUS_CHARGING:
> >               led_trigger_event(psy->charging_full_trig, LED_FULL);
> >               led_trigger_event(psy->charging_trig, LED_FULL);
> >               led_trigger_event(psy->full_trig, LED_OFF);
> >               led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0);
> > +             led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                  intensity_red,
> > +                                  3,
> > +                                  LED_FULL);
> >               break;
> >       default:
> >               led_trigger_event(psy->charging_full_trig, LED_OFF);
> > @@ -49,6 +59,10 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
> >               led_trigger_event(psy->full_trig, LED_OFF);
> >               led_trigger_event(psy->charging_blink_full_solid_trig,
> >                       LED_OFF);
> > +             led_mc_trigger_event(psy->charging_red_full_green_trig,
> > +                                  intensity_red,
> > +                                  3,
> > +                                  LED_OFF);
> >               break;
> >       }
> >  }
> > @@ -74,6 +88,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
> >       if (!psy->charging_blink_full_solid_trig_name)
> >               goto charging_blink_full_solid_failed;
> >
> > +     psy->charging_red_full_green_trig_name = kasprintf(GFP_KERNEL,
> > +             "%s-charging-red-full-green", psy->desc->name);
> > +     if (!psy->charging_red_full_green_trig_name)
> > +             goto charging_red_full_green_failed;
> > +
> >       led_trigger_register_simple(psy->charging_full_trig_name,
> >                                   &psy->charging_full_trig);
> >       led_trigger_register_simple(psy->charging_trig_name,
> > @@ -82,9 +101,13 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
> >                                   &psy->full_trig);
> >       led_trigger_register_simple(psy->charging_blink_full_solid_trig_name,
> >                                   &psy->charging_blink_full_solid_trig);
> > +     led_trigger_register_simple(psy->charging_red_full_green_trig_name,
> > +                                 &psy->charging_red_full_green_trig);
> >
> >       return 0;
> >
> > +charging_red_full_green_failed:
> > +     kfree(psy->charging_blink_full_solid_trig_name);
> >  charging_blink_full_solid_failed:
> >       kfree(psy->full_trig_name);
> >  full_failed:
> > @@ -101,10 +124,12 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy)
> >       led_trigger_unregister_simple(psy->charging_trig);
> >       led_trigger_unregister_simple(psy->full_trig);
> >       led_trigger_unregister_simple(psy->charging_blink_full_solid_trig);
> > +     led_trigger_unregister_simple(psy->charging_red_full_green_trig);
> >       kfree(psy->charging_blink_full_solid_trig_name);
> >       kfree(psy->full_trig_name);
> >       kfree(psy->charging_trig_name);
> >       kfree(psy->charging_full_trig_name);
> > +     kfree(psy->charging_red_full_green_trig_name);
> >  }
> >
> >  /* Generated power specific LEDs triggers. */
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index c0992a77feea..1d7c0b43070f 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -318,6 +318,8 @@ struct power_supply {
> >       char *online_trig_name;
> >       struct led_trigger *charging_blink_full_solid_trig;
> >       char *charging_blink_full_solid_trig_name;
> > +     struct led_trigger *charging_red_full_green_trig;
> > +     char *charging_red_full_green_trig_name;
> >  #endif
> >  };
> >
> > --
> > 2.44.0
> >
> >



-- 
BR,
Kate