| Message ID | 20240324150107.976025-2-hpa@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | KTD2026 indicator LED for X86 Xiaomi Pad2 | expand |
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); > +}
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
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?
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
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!
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 >
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
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(+)