This patch added the support for Xiaomi Pad2 indicator LED. This work
included two parts.
1. Added the KTD2026 swnode description to describe the LED controller.
2. Migrated the original driver to fwnode to support x86 platform.
Moreover, the LED trigger is set to bq27520-0-charging for Xiaomi Pad2
so the LED will be turned on when charging.
--
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.
Kate Hsuan (2):
platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
indicator LED
leds: rgb: leds-ktd202x: Get device properties through fwnode to
support ACPI
drivers/leds/rgb/Kconfig | 1 -
drivers/leds/rgb/leds-ktd202x.c | 60 ++++++++-----
.../platform/x86/x86-android-tablets/other.c | 85 +++++++++++++++++++
.../x86/x86-android-tablets/shared-psy-info.h | 2 +
4 files changed, 127 insertions(+), 21 deletions(-)
--
2.43.2
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 <[email protected]>
---
drivers/leds/rgb/Kconfig | 1 -
drivers/leds/rgb/leds-ktd202x.c | 60 ++++++++++++++++++++++-----------
2 files changed, 40 insertions(+), 21 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..fb0d7c102dea 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 *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 = (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.2
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 <[email protected]>
---
.../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.2
On Wed, 6 Mar 2024, Kate Hsuan 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
> 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 <[email protected]>
FWIW,
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Wed, 6 Mar 2024, Kate Hsuan wrote:
> This patch added the support for Xiaomi Pad2 indicator LED. This work
> included two parts.
> 1. Added the KTD2026 swnode description to describe the LED controller.
> 2. Migrated the original driver to fwnode to support x86 platform.
>
> Moreover, the LED trigger is set to bq27520-0-charging for Xiaomi Pad2
> so the LED will be turned on when charging.
>
> --
> 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.
>
> Kate Hsuan (2):
> platform: x86-android-tablets: other: Add swnode for Xiaomi pad2
> indicator LED
> leds: rgb: leds-ktd202x: Get device properties through fwnode to
> support ACPI
Hi,
I took the patch 1/2 now into pdx86/review-ilpo where it will propagate
into pdx86/for-next.
--
i.
Am Mittwoch, dem 06.03.2024 um 10:58 +0800 schrieb 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 <[email protected]>
Tested-by: André Apitzsch <[email protected]> # on BQ Aquaris M5
Wed, Mar 06, 2024 at 10:58:00AM +0800, Kate Hsuan kirjoitti:
> 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.
Ilpo, Kate, please consider the following remarks.
..
> #include <linux/gpio/machine.h>
> #include <linux/input.h>
> #include <linux/platform_device.h>
+ Blank line?
> +#include <dt-bindings/leds/common.h>
Not sure where to place this, some drivers put it the first, some after linux/*.
..
> +/* main fwnode for ktd2026 */
> +static const struct software_node ktd2026_node = {
No name?
> +};
> +
> +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"),
It's less than 80, why not on a single line?
> +
Redundant blank line.
> + { }
> +};
..
> +/* B */
B for red?!
> +static const struct property_entry ktd2026_red_led_props[] = {
> + PROPERTY_ENTRY_U32("reg", 0),
> + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
> + { }
> +};
..
> +/* R */
R for blue?!
> +static const struct property_entry ktd2026_blue_led_props[] = {
> + PROPERTY_ENTRY_U32("reg", 2),
> + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
> + { }
> +};
..
> +static int __init xiaomi_mipad2_init(void)
> +{
> + return software_node_register_node_group(ktd2026_node_group);
> +}
> +
> +static void xiaomi_mipad2_exit(void)
Don't you want to have __exit here?
> +{
> + software_node_unregister_node_group(ktd2026_node_group);
> +}
--
With Best Regards,
Andy Shevchenko
Wed, Mar 06, 2024 at 10:58:01AM +0800, Kate Hsuan kirjoitti:
> 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.
..
> + fwnode_for_each_available_child_node(np, child) {
Please, rename np to fwnode to avoid confusion.
> + num_channels++;
> + }
..
> - 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);
Must be %pfw now.
> - of_node_put(child);
> + fwnode_handle_put(child);
> return -EINVAL;
Side note: This shouldn't shadow error code when ret != 0.
> }
..
> - 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);
Must be %pfw now.
> - of_node_put(child);
> + fwnode_handle_put(child);
> return 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);
Must be %pfw now.
> return -EINVAL;
> /* 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);
Must be %pfw now.
> return ret;
> }
..
> + struct fwnode_handle *child, *np;
Do not use np for sturct fwnode_handle. It will be quite confusing.
..
> - chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
> + count = device_get_child_node_count(dev);
>
Redundant blank line.
> - count = of_get_available_child_count(np);
> if (!count || count > chip->num_leds)
> return -EINVAL;
..
> + chip->num_leds = (unsigned long)i2c_get_match_data(client);
No warnings during compilation?
..
> +static const struct i2c_device_id ktd202x_id[] = {
> + {"ktd2026", KTD2026_NUM_LEDS},
> + {"ktd2027", KTD2027_NUM_LEDS},
> + {},
N ocomma for the terminator entry.
> +};
> +MODULE_DEVICE_TABLE(i2c, ktd202x_id);
..
> +#ifndef CONFIG_ACPI
Please, no. Drop them.
> 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
This is quite unusual besides being ugly.
> },
> .probe = ktd202x_probe,
> .remove = ktd202x_remove,
> .shutdown = ktd202x_shutdown,
> + .id_table = ktd202x_id,
> };
--
With Best Regards,
Andy Shevchenko
On Tue, 12 Mar 2024, Andy Shevchenko wrote:
> Wed, Mar 06, 2024 at 10:58:00AM +0800, Kate Hsuan kirjoitti:
> > 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.
>
> Ilpo, Kate, please consider the following remarks.
>
> ...
>
> > #include <linux/gpio/machine.h>
> > #include <linux/input.h>
> > #include <linux/platform_device.h>
>
> + Blank line?
>
> > +#include <dt-bindings/leds/common.h>
>
> Not sure where to place this, some drivers put it the first, some after linux/*.
>
> ...
>
> > +/* main fwnode for ktd2026 */
> > +static const struct software_node ktd2026_node = {
>
> No name?
>
> > +};
> > +
> > +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"),
>
> It's less than 80, why not on a single line?
>
> > +
>
> Redundant blank line.
>
> > + { }
> > +};
>
> ...
>
> > +/* B */
>
> B for red?!
>
> > +static const struct property_entry ktd2026_red_led_props[] = {
> > + PROPERTY_ENTRY_U32("reg", 0),
> > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
> > + { }
> > +};
The name with "red" and LED_COLOR_ID_BLUE are also inconsistent.
> ...
>
> > +/* R */
>
> R for blue?!
>
> > +static const struct property_entry ktd2026_blue_led_props[] = {
> > + PROPERTY_ENTRY_U32("reg", 2),
> > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
> > + { }
> > +};
Here as well.
I think it's better I drop this patch (it's only in review-ilpo) and wait
for v5 version as there's some much still to correct.
--
i.
On Tue, Mar 12, 2024 at 6:35 PM Ilpo Järvinen
<[email protected]> wrote:
>
> On Tue, 12 Mar 2024, Andy Shevchenko wrote:
>
> > Wed, Mar 06, 2024 at 10:58:00AM +0800, Kate Hsuan kirjoitti:
> > > 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.
> >
> > Ilpo, Kate, please consider the following remarks.
> >
> > ...
> >
> > > #include <linux/gpio/machine.h>
> > > #include <linux/input.h>
> > > #include <linux/platform_device.h>
> >
> > + Blank line?
> >
> > > +#include <dt-bindings/leds/common.h>
> >
> > Not sure where to place this, some drivers put it the first, some after linux/*.
> >
> > ...
> >
> > > +/* main fwnode for ktd2026 */
> > > +static const struct software_node ktd2026_node = {
> >
> > No name?
> >
> > > +};
> > > +
> > > +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"),
> >
> > It's less than 80, why not on a single line?
> >
> > > +
> >
> > Redundant blank line.
> >
> > > + { }
> > > +};
> >
> > ...
> >
> > > +/* B */
> >
> > B for red?!
> >
> > > +static const struct property_entry ktd2026_red_led_props[] = {
> > > + PROPERTY_ENTRY_U32("reg", 0),
> > > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_BLUE),
> > > + { }
> > > +};
>
> The name with "red" and LED_COLOR_ID_BLUE are also inconsistent.
>
> > ...
> >
> > > +/* R */
> >
> > R for blue?!
> >
> > > +static const struct property_entry ktd2026_blue_led_props[] = {
> > > + PROPERTY_ENTRY_U32("reg", 2),
> > > + PROPERTY_ENTRY_U32("color", LED_COLOR_ID_RED),
> > > + { }
> > > +};
>
> Here as well.
>
> I think it's better I drop this patch (it's only in review-ilpo) and wait
> for v5 version as there's some much still to correct.
>
Hi,
Thank you for reviewing it.
I'll propose the v5 patch to improve them :)
>
> --
> i.
>
--
BR,
Kate
Hi
Thank you for reviewing.
On Tue, Mar 12, 2024 at 7:04 AM Andy Shevchenko
<[email protected]> wrote:
>
> Wed, Mar 06, 2024 at 10:58:01AM +0800, Kate Hsuan kirjoitti:
> > 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.
>
> ...
>
> > + fwnode_for_each_available_child_node(np, child) {
>
> Please, rename np to fwnode to avoid confusion.
>
> > + num_channels++;
> > + }
>
> ...
>
> > - 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);
>
> Must be %pfw now.
>
> > - of_node_put(child);
> > + fwnode_handle_put(child);
>
> > return -EINVAL;
>
> Side note: This shouldn't shadow error code when ret != 0.
>
> > }
>
> ...
>
> > - 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);
>
> Must be %pfw now.
>
> > - of_node_put(child);
> > + fwnode_handle_put(child);
> > return 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);
>
> Must be %pfw now.
>
> > return -EINVAL;
>
> > /* 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);
>
> Must be %pfw now.
>
> > return ret;
> > }
>
> ...
>
> > + struct fwnode_handle *child, *np;
>
> Do not use np for sturct fwnode_handle. It will be quite confusing.
>
> ...
>
> > - chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
> > + count = device_get_child_node_count(dev);
>
> >
>
> Redundant blank line.
>
> > - count = of_get_available_child_count(np);
> > if (!count || count > chip->num_leds)
> > return -EINVAL;
>
> ...
>
> > + chip->num_leds = (unsigned long)i2c_get_match_data(client);
>
> No warnings during compilation?
Yes, the compiler doesn't complain about it.
>
> ...
>
> > +static const struct i2c_device_id ktd202x_id[] = {
> > + {"ktd2026", KTD2026_NUM_LEDS},
> > + {"ktd2027", KTD2027_NUM_LEDS},
> > + {},
>
> N ocomma for the terminator entry.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ktd202x_id);
>
> ...
>
> > +#ifndef CONFIG_ACPI
>
> Please, no. Drop them.
Okay, I've dropped them in v5 patch.
>
> > 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
>
> This is quite unusual besides being ugly.
>
> > },
> > .probe = ktd202x_probe,
> > .remove = ktd202x_remove,
> > .shutdown = ktd202x_shutdown,
> > + .id_table = ktd202x_id,
> > };
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
--
BR,
Kate
On Fri, Mar 22, 2024 at 7:45 AM Kate Hsuan <[email protected]> wrote:
> On Tue, Mar 12, 2024 at 7:04 AM Andy Shevchenko
> <[email protected]> wrote:
..
> > > + chip->num_leds = (unsigned long)i2c_get_match_data(client);
> >
> > No warnings during compilation?
> Yes, the compiler doesn't complain about it.
And for 32-bit mode as well?
..
P.S. You have commented only on the two comments. What about the rest?
--
With Best Regards,
Andy Shevchenko
On Sat, Mar 23, 2024 at 12:02 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 7:45 AM Kate Hsuan <[email protected]> wrote:
> > On Tue, Mar 12, 2024 at 7:04 AM Andy Shevchenko
> > <[email protected]> wrote:
>
> ...
>
> > > > + chip->num_leds = (unsigned long)i2c_get_match_data(client);
> > >
> > > No warnings during compilation?
> > Yes, the compiler doesn't complain about it.
>
> And for 32-bit mode as well?
>
Hi Andy,
I've tested it with 32bits kernel build and the compiler didn't
complain about the warnings.
> ...
>
> P.S. You have commented only on the two comments. What about the rest?
For the rest, including variable renaming, error number shadowing, and
"%pfw" were fixed in the v5 patch.
I sent the patch but I forgot to add you.
I'll resend the v5 patch and keep you in the loop.
Thank you :)
>
> --
> With Best Regards,
> Andy Shevchenko
>
--
BR,
Kate