* [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2
@ 2024-02-16 16:05 Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED Kate Hsuan
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Kate Hsuan @ 2024-02-16 16:05 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, Ilpo Järvinen, André Apitzsch
Cc: Kate Hsuan
The v2 patch includes:
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.
Kate Hsuan (3):
platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
indicator LED
leds: rgb: leds-ktd202x: Get device properties through fwnode to
support ACPI
leds: rgb: leds-ktd202x: Skip requlator settings for Xiaomi pad2
drivers/leds/rgb/Kconfig | 1 -
drivers/leds/rgb/leds-ktd202x.c | 73 +++++++++++-----
.../platform/x86/x86-android-tablets/other.c | 85 +++++++++++++++++++
.../x86/x86-android-tablets/shared-psy-info.h | 2 +
4 files changed, 141 insertions(+), 20 deletions(-)
--
2.43.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED
2024-02-16 16:05 [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Kate Hsuan
@ 2024-02-16 16:05 ` Kate Hsuan
2024-02-19 14:07 ` Hans de Goede
2024-02-16 16:05 ` [PATCH v2 2/3] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI Kate Hsuan
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Kate Hsuan @ 2024-02-16 16:05 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, Ilpo Järvinen, André Apitzsch
Cc: 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 | 85 +++++++++++++++++++
.../x86/x86-android-tablets/shared-psy-info.h | 2 +
2 files changed, 87 insertions(+)
diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
index bc6bbf7ec6ea..542ef6667b7b 100644
--- a/drivers/platform/x86/x86-android-tablets/other.c
+++ b/drivers/platform/x86/x86-android-tablets/other.c
@@ -12,6 +12,7 @@
#include <linux/gpio/machine.h>
#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 +594,87 @@ 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 = {
+};
+
+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,
+};
+
+/* B */
+static const struct property_entry ktd2026_red_led_props[] = {
+ PROPERTY_ENTRY_U32("reg", 0),
+ PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
+ { }
+};
+
+static const struct software_node ktd2026_red_led_node = {
+ .properties = ktd2026_red_led_props,
+ .parent = &ktd2026_rgb_led_node,
+};
+
+/* G */
+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,
+};
+
+/* R */
+static const struct property_entry ktd2026_blue_led_props[] = {
+ PROPERTY_ENTRY_U32("reg", 2),
+ PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
+ { }
+};
+
+static const struct software_node ktd2026_blue_led_node = {
+ .properties = ktd2026_blue_led_props,
+ .parent = &ktd2026_rgb_led_node,
+};
+
+static const struct software_node *ktd2026_node_group[] = {
+ &ktd2026_node,
+ &ktd2026_rgb_led_node,
+ &ktd2026_red_led_node,
+ &ktd2026_green_led_node,
+ &ktd2026_blue_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 +698,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 +707,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.43.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI
2024-02-16 16:05 [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED Kate Hsuan
@ 2024-02-16 16:05 ` Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2 Kate Hsuan
2024-02-19 13:57 ` [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Ilpo Järvinen
3 siblings, 0 replies; 11+ messages in thread
From: Kate Hsuan @ 2024-02-16 16:05 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, Ilpo Järvinen, André Apitzsch
Cc: 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 | 58 ++++++++++++++++++++++-----------
2 files changed, 39 insertions(+), 20 deletions(-)
diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index a6a21f564673..f245dbd9a163 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..8eb79c342fb6 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -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 *np,
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(np, 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(np, child) {
u32 mono_color;
u32 reg;
int ret;
- ret = of_property_read_u32(child, "reg", ®);
+ ret = fwnode_property_read_u32(child, "reg", ®);
if (ret != 0 || reg >= chip->num_leds) {
dev_err(chip->dev, "invalid 'reg' of %pOFn\n", child);
- of_node_put(child);
+ fwnode_handle_put(child);
return -EINVAL;
}
- 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);
+ fwnode_handle_put(child);
return ret;
}
@@ -433,14 +435,14 @@ 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 *np,
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", ®);
+ ret = fwnode_property_read_u32(np, "reg", ®);
if (ret != 0 || reg >= chip->num_leds) {
dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
return -EINVAL;
@@ -454,7 +456,7 @@ 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 *np, unsigned int index)
{
struct ktd202x_led *led = &chip->leds[index];
struct led_init_data init_data = {};
@@ -463,14 +465,14 @@ 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(np, "color", &color);
if (ret < 0 && ret != -EINVAL) {
dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);
return ret;
}
led->chip = chip;
- init_data.fwnode = of_fwnode_handle(np);
+ init_data.fwnode = np;
if (color == LED_COLOR_ID_RGB) {
cdev = &led->mcdev.led_cdev;
@@ -492,26 +494,30 @@ 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, *np;
+ struct device *dev = chip->dev;
int count;
int i = 0;
- chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
+ count = device_get_child_node_count(dev);
- count = of_get_available_child_count(np);
if (!count || count > chip->num_leds)
return -EINVAL;
+ np = dev_fwnode(chip->dev);
+ if (!np)
+ 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(np, child) {
int ret = ktd202x_add_led(chip, child, i);
if (ret) {
- of_node_put(child);
+ fwnode_handle_put(child);
return ret;
}
i++;
@@ -568,6 +574,8 @@ static int ktd202x_probe(struct i2c_client *client)
return ret;
}
+ chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
+
ret = ktd202x_probe_dt(chip);
if (ret < 0) {
regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
@@ -602,21 +610,33 @@ 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);
+
+#ifndef CONFIG_ACPI
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);
+#endif
static struct i2c_driver ktd202x_driver = {
.driver = {
.name = "leds-ktd202x",
+#ifndef CONFIG_ACPI
.of_match_table = ktd202x_match_table,
+#endif
},
.probe = ktd202x_probe,
.remove = ktd202x_remove,
.shutdown = ktd202x_shutdown,
+ .id_table = ktd202x_id,
};
module_i2c_driver(ktd202x_driver);
--
2.43.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2
2024-02-16 16:05 [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 2/3] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI Kate Hsuan
@ 2024-02-16 16:05 ` Kate Hsuan
2024-02-19 13:28 ` Ilpo Järvinen
2024-02-19 13:57 ` [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Ilpo Järvinen
3 siblings, 1 reply; 11+ messages in thread
From: Kate Hsuan @ 2024-02-16 16:05 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, Ilpo Järvinen, André Apitzsch
Cc: Kate Hsuan
The controller is already powered by BP25890RTWR on Xiaomi Pad2 so the
regulator settings can be ignored.
Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
drivers/leds/rgb/leds-ktd202x.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index 8eb79c342fb6..6fd0794988e9 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -14,7 +14,9 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/regmap.h>
+#ifndef CONFIG_ACPI
#include <linux/regulator/consumer.h>
+#endif
#define KTD2026_NUM_LEDS 3
#define KTD2027_NUM_LEDS 4
@@ -105,18 +107,22 @@ struct ktd202x {
static int ktd202x_chip_disable(struct ktd202x *chip)
{
+#ifndef CONFIG_ACPI
int ret;
+#endif
if (!chip->enabled)
return 0;
regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
+#ifndef CONFIG_ACPI
ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
if (ret) {
dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
return ret;
}
+#endif
chip->enabled = false;
return 0;
@@ -129,11 +135,13 @@ static int ktd202x_chip_enable(struct ktd202x *chip)
if (chip->enabled)
return 0;
+#ifndef CONFIG_ACPI
ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), chip->regulators);
if (ret) {
dev_err(chip->dev, "Failed to enable regulators: %d\n", ret);
return ret;
}
+#endif
chip->enabled = true;
ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_WAKE);
@@ -560,6 +568,7 @@ static int ktd202x_probe(struct i2c_client *client)
return ret;
}
+#ifndef CONFIG_ACPI
chip->regulators[0].supply = "vin";
chip->regulators[1].supply = "vio";
ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
@@ -573,10 +582,12 @@ static int ktd202x_probe(struct i2c_client *client)
dev_err_probe(dev, ret, "Failed to enable regulators.\n");
return ret;
}
+#endif
chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
ret = ktd202x_probe_dt(chip);
+#ifndef CONFIG_ACPI
if (ret < 0) {
regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
return ret;
@@ -587,6 +598,10 @@ static int ktd202x_probe(struct i2c_client *client)
dev_err_probe(dev, ret, "Failed to disable regulators.\n");
return ret;
}
+#else
+ if (ret < 0)
+ return ret;
+#endif
mutex_init(&chip->mutex);
--
2.43.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2
2024-02-16 16:05 ` [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2 Kate Hsuan
@ 2024-02-19 13:28 ` Ilpo Järvinen
2024-02-19 14:04 ` Hans de Goede
0 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2024-02-19 13:28 UTC (permalink / raw)
To: Kate Hsuan
Cc: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, André Apitzsch
On Sat, 17 Feb 2024, Kate Hsuan wrote:
> The controller is already powered by BP25890RTWR on Xiaomi Pad2 so the
> regulator settings can be ignored.
>
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
> drivers/leds/rgb/leds-ktd202x.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 8eb79c342fb6..6fd0794988e9 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
> @@ -14,7 +14,9 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/regmap.h>
> +#ifndef CONFIG_ACPI
> #include <linux/regulator/consumer.h>
> +#endif
Why you need #ifndef here?
> #define KTD2026_NUM_LEDS 3
> #define KTD2027_NUM_LEDS 4
> @@ -105,18 +107,22 @@ struct ktd202x {
>
> static int ktd202x_chip_disable(struct ktd202x *chip)
> {
> +#ifndef CONFIG_ACPI
> int ret;
> +#endif
>
> if (!chip->enabled)
> return 0;
>
> regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
>
> +#ifndef CONFIG_ACPI
> ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> if (ret) {
> dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
> return ret;
> }
> +#endif
>
> chip->enabled = false;
> return 0;
> @@ -129,11 +135,13 @@ static int ktd202x_chip_enable(struct ktd202x *chip)
> if (chip->enabled)
> return 0;
>
> +#ifndef CONFIG_ACPI
> ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), chip->regulators);
> if (ret) {
> dev_err(chip->dev, "Failed to enable regulators: %d\n", ret);
> return ret;
> }
> +#endif
> chip->enabled = true;
>
> ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_WAKE);
> @@ -560,6 +568,7 @@ static int ktd202x_probe(struct i2c_client *client)
> return ret;
> }
>
> +#ifndef CONFIG_ACPI
> chip->regulators[0].supply = "vin";
> chip->regulators[1].supply = "vio";
> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
> @@ -573,10 +582,12 @@ static int ktd202x_probe(struct i2c_client *client)
> dev_err_probe(dev, ret, "Failed to enable regulators.\n");
> return ret;
> }
> +#endif
>
> chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
>
> ret = ktd202x_probe_dt(chip);
> +#ifndef CONFIG_ACPI
> if (ret < 0) {
> regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> return ret;
> @@ -587,6 +598,10 @@ static int ktd202x_probe(struct i2c_client *client)
> dev_err_probe(dev, ret, "Failed to disable regulators.\n");
> return ret;
> }
> +#else
> + if (ret < 0)
> + return ret;
> +#endif
>
> mutex_init(&chip->mutex);
To me this entire approach looks quite ugly. It would be much cleaner to
have something along these lines:
#ifndef CONFIG_ACPI
static int ktd202x_regulators_disable(struct ktd202x *chip)
{
int ret;
ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
if (ret)
dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
return ret;
}
...
#else
static inline int ktd202x_regulators_disable(struct ktd202x *chip) { return 0; }
...
#endif
And call that function without any #ifdefs from the other code.
--
i.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2
2024-02-16 16:05 [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Kate Hsuan
` (2 preceding siblings ...)
2024-02-16 16:05 ` [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2 Kate Hsuan
@ 2024-02-19 13:57 ` Ilpo Järvinen
2024-02-20 4:34 ` Kate Hsuan
3 siblings, 1 reply; 11+ messages in thread
From: Ilpo Järvinen @ 2024-02-19 13:57 UTC (permalink / raw)
To: Kate Hsuan
Cc: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, André Apitzsch
On Sat, 17 Feb 2024, Kate Hsuan wrote:
> The v2 patch includes:
> 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.
>
> Kate Hsuan (3):
> platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
> indicator LED
> leds: rgb: leds-ktd202x: Get device properties through fwnode to
> support ACPI
> leds: rgb: leds-ktd202x: Skip requlator settings for Xiaomi pad2
So what's the expectation here? I take the first patch and the two other
go through the LED subsys?
--
i.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2
2024-02-19 13:28 ` Ilpo Järvinen
@ 2024-02-19 14:04 ` Hans de Goede
2024-02-20 4:21 ` Kate Hsuan
0 siblings, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2024-02-19 14:04 UTC (permalink / raw)
To: Ilpo Järvinen, Kate Hsuan
Cc: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
André Apitzsch
Hi Kate, Ilpo,
On 2/19/24 14:28, Ilpo Järvinen wrote:
> On Sat, 17 Feb 2024, Kate Hsuan wrote:
>
>> The controller is already powered by BP25890RTWR on Xiaomi Pad2 so the
>> regulator settings can be ignored.
>>
>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>> ---
>> drivers/leds/rgb/leds-ktd202x.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
>> index 8eb79c342fb6..6fd0794988e9 100644
>> --- a/drivers/leds/rgb/leds-ktd202x.c
>> +++ b/drivers/leds/rgb/leds-ktd202x.c
>> @@ -14,7 +14,9 @@
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> #include <linux/regmap.h>
>> +#ifndef CONFIG_ACPI
>> #include <linux/regulator/consumer.h>
>> +#endif
>
> Why you need #ifndef here?
>
>> #define KTD2026_NUM_LEDS 3
>> #define KTD2027_NUM_LEDS 4
>> @@ -105,18 +107,22 @@ struct ktd202x {
>>
>> static int ktd202x_chip_disable(struct ktd202x *chip)
>> {
>> +#ifndef CONFIG_ACPI
>> int ret;
>> +#endif
>>
>> if (!chip->enabled)
>> return 0;
>>
>> regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
>>
>> +#ifndef CONFIG_ACPI
>> ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
>> if (ret) {
>> dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
>> return ret;
>> }
>> +#endif
>>
>> chip->enabled = false;
>> return 0;
>> @@ -129,11 +135,13 @@ static int ktd202x_chip_enable(struct ktd202x *chip)
>> if (chip->enabled)
>> return 0;
>>
>> +#ifndef CONFIG_ACPI
>> ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), chip->regulators);
>> if (ret) {
>> dev_err(chip->dev, "Failed to enable regulators: %d\n", ret);
>> return ret;
>> }
>> +#endif
>> chip->enabled = true;
>>
>> ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_WAKE);
>> @@ -560,6 +568,7 @@ static int ktd202x_probe(struct i2c_client *client)
>> return ret;
>> }
>>
>> +#ifndef CONFIG_ACPI
>> chip->regulators[0].supply = "vin";
>> chip->regulators[1].supply = "vio";
>> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
>> @@ -573,10 +582,12 @@ static int ktd202x_probe(struct i2c_client *client)
>> dev_err_probe(dev, ret, "Failed to enable regulators.\n");
>> return ret;
>> }
>> +#endif
>>
>> chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
>>
>> ret = ktd202x_probe_dt(chip);
>> +#ifndef CONFIG_ACPI
>> if (ret < 0) {
>> regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
>> return ret;
>> @@ -587,6 +598,10 @@ static int ktd202x_probe(struct i2c_client *client)
>> dev_err_probe(dev, ret, "Failed to disable regulators.\n");
>> return ret;
>> }
>> +#else
>> + if (ret < 0)
>> + return ret;
>> +#endif
>>
>> mutex_init(&chip->mutex);
>
> To me this entire approach looks quite ugly. It would be much cleaner to
> have something along these lines:
>
> #ifndef CONFIG_ACPI
> static int ktd202x_regulators_disable(struct ktd202x *chip)
> {
> int ret;
>
> ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> if (ret)
> dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
>
> return ret;
> }
> ...
> #else
> static inline int ktd202x_regulators_disable(struct ktd202x *chip) { return 0; }
> ...
> #endif
>
> And call that function without any #ifdefs from the other code.
I believe that skipping the regulator stuff in the ACPI case is not
the right solution here.
There likely is some underlying issue which also happens on non ACPI
hw, but I guess no-one has ever tried to remove the module there.
I have the same tablet as on which Kate is testing this. So I plan
to make some time to reproduce this and see if I can come up with
a proper fix.
Regards,
Hans
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED
2024-02-16 16:05 ` [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED Kate Hsuan
@ 2024-02-19 14:07 ` Hans de Goede
2024-02-20 3:22 ` Kate Hsuan
0 siblings, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2024-02-19 14:07 UTC (permalink / raw)
To: Kate Hsuan, Pavel Machek, Lee Jones, linux-leds,
platform-driver-x86, Ilpo Järvinen, André Apitzsch
Hi Kate,
On 2/16/24 17:05, Kate Hsuan 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
> 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 | 85 +++++++++++++++++++
> .../x86/x86-android-tablets/shared-psy-info.h | 2 +
> 2 files changed, 87 insertions(+)
>
> diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
> index bc6bbf7ec6ea..542ef6667b7b 100644
> --- a/drivers/platform/x86/x86-android-tablets/other.c
> +++ b/drivers/platform/x86/x86-android-tablets/other.c
> @@ -12,6 +12,7 @@
> #include <linux/gpio/machine.h>
> #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 +594,87 @@ 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 = {
> +};
> +
> +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"),
> +
> + { }
> +};
What is the result of setting this default trigger on
the multi-color LED class device ?
Will the LED now turn on at whatever color it was last
set (presumably white?) when charging and turn off
again when charging is complete, or the charger is plugged out ?
Regards,
Hans
> +
> +static const struct software_node ktd2026_rgb_led_node = {
> + .name = "multi-led",
> + .properties = ktd2026_rgb_led_props,
> + .parent = &ktd2026_node,
> +};
> +
> +/* B */
> +static const struct property_entry ktd2026_red_led_props[] = {
> + PROPERTY_ENTRY_U32("reg", 0),
> + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
> + { }
> +};
> +
> +static const struct software_node ktd2026_red_led_node = {
> + .properties = ktd2026_red_led_props,
> + .parent = &ktd2026_rgb_led_node,
> +};
> +
> +/* G */
> +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,
> +};
> +
> +/* R */
> +static const struct property_entry ktd2026_blue_led_props[] = {
> + PROPERTY_ENTRY_U32("reg", 2),
> + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
> + { }
> +};
> +
> +static const struct software_node ktd2026_blue_led_node = {
> + .properties = ktd2026_blue_led_props,
> + .parent = &ktd2026_rgb_led_node,
> +};
> +
> +static const struct software_node *ktd2026_node_group[] = {
> + &ktd2026_node,
> + &ktd2026_rgb_led_node,
> + &ktd2026_red_led_node,
> + &ktd2026_green_led_node,
> + &ktd2026_blue_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 +698,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 +707,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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED
2024-02-19 14:07 ` Hans de Goede
@ 2024-02-20 3:22 ` Kate Hsuan
0 siblings, 0 replies; 11+ messages in thread
From: Kate Hsuan @ 2024-02-20 3:22 UTC (permalink / raw)
To: Hans de Goede
Cc: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Ilpo Järvinen, André Apitzsch
Hi Hans,
On Mon, Feb 19, 2024 at 10:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate,
>
> On 2/16/24 17:05, Kate Hsuan 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
> > 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 | 85 +++++++++++++++++++
> > .../x86/x86-android-tablets/shared-psy-info.h | 2 +
> > 2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/platform/x86/x86-android-tablets/other.c b/drivers/platform/x86/x86-android-tablets/other.c
> > index bc6bbf7ec6ea..542ef6667b7b 100644
> > --- a/drivers/platform/x86/x86-android-tablets/other.c
> > +++ b/drivers/platform/x86/x86-android-tablets/other.c
> > @@ -12,6 +12,7 @@
> > #include <linux/gpio/machine.h>
> > #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 +594,87 @@ 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 = {
> > +};
> > +
> > +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"),
> > +
> > + { }
> > +};
>
> What is the result of setting this default trigger on
> the multi-color LED class device ?
Thank you for reviewing it.
>
> Will the LED now turn on at whatever color it was last
> set (presumably white?)
You are right. It is white.
> when charging and turn off
> again when charging is complete, or the charger is plugged out ?
The behavior is simple. It is lit up when the charger is connected. It
is turned off when the charger is disconnected.
Many triggers can be used.
bq27520-0-charging-or-full
[bq27520-0-charging]
bq27520-0-full
bq27520-0-charging-blink-full-solid
I think bq27520-0-charging-or-full or
bq27520-0-charging-blink-full-solid can be used in the v3 patch.
(The battery status is always "charging" for my Xiaomi pad2. Even the
battery is 100%. When the pad is off and then connect the charger, it
will be turned on immediately. The LED will be lit up after the kernel
module is loaded)
>
> Regards,
>
> Hans
>
>
>
>
> > +
> > +static const struct software_node ktd2026_rgb_led_node = {
> > + .name = "multi-led",
> > + .properties = ktd2026_rgb_led_props,
> > + .parent = &ktd2026_node,
> > +};
> > +
> > +/* B */
> > +static const struct property_entry ktd2026_red_led_props[] = {
> > + PROPERTY_ENTRY_U32("reg", 0),
> > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
> > + { }
> > +};
> > +
> > +static const struct software_node ktd2026_red_led_node = {
> > + .properties = ktd2026_red_led_props,
> > + .parent = &ktd2026_rgb_led_node,
> > +};
> > +
> > +/* G */
> > +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,
> > +};
> > +
> > +/* R */
> > +static const struct property_entry ktd2026_blue_led_props[] = {
> > + PROPERTY_ENTRY_U32("reg", 2),
> > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
> > + { }
> > +};
> > +
> > +static const struct software_node ktd2026_blue_led_node = {
> > + .properties = ktd2026_blue_led_props,
> > + .parent = &ktd2026_rgb_led_node,
> > +};
> > +
> > +static const struct software_node *ktd2026_node_group[] = {
> > + &ktd2026_node,
> > + &ktd2026_rgb_led_node,
> > + &ktd2026_red_led_node,
> > + &ktd2026_green_led_node,
> > + &ktd2026_blue_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 +698,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 +707,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
>
--
BR,
Kate
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2
2024-02-19 14:04 ` Hans de Goede
@ 2024-02-20 4:21 ` Kate Hsuan
0 siblings, 0 replies; 11+ messages in thread
From: Kate Hsuan @ 2024-02-20 4:21 UTC (permalink / raw)
To: Hans de Goede
Cc: Ilpo Järvinen, Pavel Machek, Lee Jones, linux-leds,
platform-driver-x86, André Apitzsch
Hi Hans and llpo,
On Mon, Feb 19, 2024 at 10:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate, Ilpo,
>
> On 2/19/24 14:28, Ilpo Järvinen wrote:
> > On Sat, 17 Feb 2024, Kate Hsuan wrote:
> >
> >> The controller is already powered by BP25890RTWR on Xiaomi Pad2 so the
> >> regulator settings can be ignored.
> >>
> >> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> >> ---
> >> drivers/leds/rgb/leds-ktd202x.c | 15 +++++++++++++++
> >> 1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> >> index 8eb79c342fb6..6fd0794988e9 100644
> >> --- a/drivers/leds/rgb/leds-ktd202x.c
> >> +++ b/drivers/leds/rgb/leds-ktd202x.c
> >> @@ -14,7 +14,9 @@
> >> #include <linux/of.h>
> >> #include <linux/of_device.h>
> >> #include <linux/regmap.h>
> >> +#ifndef CONFIG_ACPI
> >> #include <linux/regulator/consumer.h>
> >> +#endif
> >
> > Why you need #ifndef here?
> >
> >> #define KTD2026_NUM_LEDS 3
> >> #define KTD2027_NUM_LEDS 4
> >> @@ -105,18 +107,22 @@ struct ktd202x {
> >>
> >> static int ktd202x_chip_disable(struct ktd202x *chip)
> >> {
> >> +#ifndef CONFIG_ACPI
> >> int ret;
> >> +#endif
> >>
> >> if (!chip->enabled)
> >> return 0;
> >>
> >> regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
> >>
> >> +#ifndef CONFIG_ACPI
> >> ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> >> if (ret) {
> >> dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
> >> return ret;
> >> }
> >> +#endif
> >>
> >> chip->enabled = false;
> >> return 0;
> >> @@ -129,11 +135,13 @@ static int ktd202x_chip_enable(struct ktd202x *chip)
> >> if (chip->enabled)
> >> return 0;
> >>
> >> +#ifndef CONFIG_ACPI
> >> ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), chip->regulators);
> >> if (ret) {
> >> dev_err(chip->dev, "Failed to enable regulators: %d\n", ret);
> >> return ret;
> >> }
> >> +#endif
> >> chip->enabled = true;
> >>
> >> ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_WAKE);
> >> @@ -560,6 +568,7 @@ static int ktd202x_probe(struct i2c_client *client)
> >> return ret;
> >> }
> >>
> >> +#ifndef CONFIG_ACPI
> >> chip->regulators[0].supply = "vin";
> >> chip->regulators[1].supply = "vio";
> >> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
> >> @@ -573,10 +582,12 @@ static int ktd202x_probe(struct i2c_client *client)
> >> dev_err_probe(dev, ret, "Failed to enable regulators.\n");
> >> return ret;
> >> }
> >> +#endif
> >>
> >> chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
> >>
> >> ret = ktd202x_probe_dt(chip);
> >> +#ifndef CONFIG_ACPI
> >> if (ret < 0) {
> >> regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> >> return ret;
> >> @@ -587,6 +598,10 @@ static int ktd202x_probe(struct i2c_client *client)
> >> dev_err_probe(dev, ret, "Failed to disable regulators.\n");
> >> return ret;
> >> }
> >> +#else
> >> + if (ret < 0)
> >> + return ret;
> >> +#endif
> >>
> >> mutex_init(&chip->mutex);
> >
> > To me this entire approach looks quite ugly. It would be much cleaner to
> > have something along these lines:
> >
> > #ifndef CONFIG_ACPI
> > static int ktd202x_regulators_disable(struct ktd202x *chip)
> > {
> > int ret;
> >
> > ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
> > if (ret)
> > dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
> >
> > return ret;
> > }
> > ...
> > #else
> > static inline int ktd202x_regulators_disable(struct ktd202x *chip) { return 0; }
> > ...
> > #endif
> >
> > And call that function without any #ifdefs from the other code.
>
> I believe that skipping the regulator stuff in the ACPI case is not
> the right solution here.
>
> There likely is some underlying issue which also happens on non ACPI
> hw, but I guess no-one has ever tried to remove the module there.
>
> I have the same tablet as on which Kate is testing this. So I plan
> to make some time to reproduce this and see if I can come up with
> a proper fix.
>
> Regards,
>
> Hans
>
Thank you for reviewing it.
This patch is used to prevent the WARN_ON() shown in the following URL.
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2396
I'll drop this patch in the v3 patch. And I can also try to
investigate the issue of the regulator.
--
BR,
Kate
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2
2024-02-19 13:57 ` [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Ilpo Järvinen
@ 2024-02-20 4:34 ` Kate Hsuan
0 siblings, 0 replies; 11+ messages in thread
From: Kate Hsuan @ 2024-02-20 4:34 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Pavel Machek, Lee Jones, linux-leds, platform-driver-x86,
Hans de Goede, André Apitzsch
Hi llpo,
On Mon, Feb 19, 2024 at 9:57 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Sat, 17 Feb 2024, Kate Hsuan wrote:
>
> > The v2 patch includes:
> > 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.
> >
> > Kate Hsuan (3):
> > platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
> > indicator LED
> > leds: rgb: leds-ktd202x: Get device properties through fwnode to
> > support ACPI
> > leds: rgb: leds-ktd202x: Skip requlator settings for Xiaomi pad2
>
> So what's the expectation here?
Thank you for reviewing it.
Sorry for the confusion.
This patch enabled the KTD2026 LED controller on a Xiaomi Pad2. The
controller controls an indicator LED which indicates the status of the
charging or other events.
Since it is an x86-based Android tablet, we need to set the device
information through swnode and revise the driver to use fwnode APIs.
> I take the first patch and the two other
> go through the LED subsys?
Yes. the first patch is for platform-driver-x86 and the second is for
the LED subsystem. If someone would like to test it, they could easily
get the necessary part of this work.
>
> --
> i.
>
--
BR,
Kate
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-02-20 4:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-16 16:05 [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 1/3] platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED Kate Hsuan
2024-02-19 14:07 ` Hans de Goede
2024-02-20 3:22 ` Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 2/3] leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI Kate Hsuan
2024-02-16 16:05 ` [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2 Kate Hsuan
2024-02-19 13:28 ` Ilpo Järvinen
2024-02-19 14:04 ` Hans de Goede
2024-02-20 4:21 ` Kate Hsuan
2024-02-19 13:57 ` [PATCH v2 0/3] KTD2026 indicator LED for X86 Xiaomi Pad2 Ilpo Järvinen
2024-02-20 4:34 ` Kate Hsuan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.