| Message ID | 20240324150107.976025-1-hpa@redhat.com |
|---|---|
| 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: > > Add a charging_red_full_green LED trigger and the trigger is based on > led_mc_trigger_event() which can set an RGB LED when the trigger is > triggered. The LED will show red when the battery status is charging. > The LED will show green when the battery status is full. > > Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t You can drop the 'T/#t' part. ... > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_green, > + 3, ARRAY_SIZE() > + LED_FULL); ... > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_red, > + 3, Ditto. > + LED_FULL); ... > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_red, > + 3, Ditto. > + LED_OFF);
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!
Hello Kate, On Sun, Mar 24, 2024 at 11:01:06PM +0800, Kate Hsuan wrote: > Add a charging_red_full_green LED trigger and the trigger is based on > led_mc_trigger_event() which can set an RGB LED when the trigger is > triggered. The LED will show red when the battery status is charging. > The LED will show green when the battery status is full. > > Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t > Signed-off-by: Kate Hsuan <hpa@redhat.com> > --- Have you considered using orange instead of red? Using orange as charging indicator seems to be more common nowadays and allows green = battery full orange = battery charging red = battery empty / battery dead / error Greetings, -- Sebastian > drivers/power/supply/power_supply_leds.c | 25 ++++++++++++++++++++++++ > include/linux/power_supply.h | 2 ++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c > index c7db29d5fcb8..bd9c8fec5870 100644 > --- a/drivers/power/supply/power_supply_leds.c > +++ b/drivers/power/supply/power_supply_leds.c > @@ -22,6 +22,8 @@ > static void power_supply_update_bat_leds(struct power_supply *psy) > { > union power_supply_propval status; > + unsigned int intensity_green[3] = {255, 0, 0}; > + unsigned int intensity_red[3] = {0, 0, 255}; > > if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status)) > return; > @@ -36,12 +38,20 @@ static void power_supply_update_bat_leds(struct power_supply *psy) > /* Going from blink to LED on requires a LED_OFF event to stop blink */ > led_trigger_event(psy->charging_blink_full_solid_trig, LED_OFF); > led_trigger_event(psy->charging_blink_full_solid_trig, LED_FULL); > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_green, > + 3, > + LED_FULL); > break; > case POWER_SUPPLY_STATUS_CHARGING: > led_trigger_event(psy->charging_full_trig, LED_FULL); > led_trigger_event(psy->charging_trig, LED_FULL); > led_trigger_event(psy->full_trig, LED_OFF); > led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0); > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_red, > + 3, > + LED_FULL); > break; > default: > led_trigger_event(psy->charging_full_trig, LED_OFF); > @@ -49,6 +59,10 @@ static void power_supply_update_bat_leds(struct power_supply *psy) > led_trigger_event(psy->full_trig, LED_OFF); > led_trigger_event(psy->charging_blink_full_solid_trig, > LED_OFF); > + led_mc_trigger_event(psy->charging_red_full_green_trig, > + intensity_red, > + 3, > + LED_OFF); > break; > } > } > @@ -74,6 +88,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy) > if (!psy->charging_blink_full_solid_trig_name) > goto charging_blink_full_solid_failed; > > + psy->charging_red_full_green_trig_name = kasprintf(GFP_KERNEL, > + "%s-charging-red-full-green", psy->desc->name); > + if (!psy->charging_red_full_green_trig_name) > + goto charging_red_full_green_failed; > + > led_trigger_register_simple(psy->charging_full_trig_name, > &psy->charging_full_trig); > led_trigger_register_simple(psy->charging_trig_name, > @@ -82,9 +101,13 @@ static int power_supply_create_bat_triggers(struct power_supply *psy) > &psy->full_trig); > led_trigger_register_simple(psy->charging_blink_full_solid_trig_name, > &psy->charging_blink_full_solid_trig); > + led_trigger_register_simple(psy->charging_red_full_green_trig_name, > + &psy->charging_red_full_green_trig); > > return 0; > > +charging_red_full_green_failed: > + kfree(psy->charging_blink_full_solid_trig_name); > charging_blink_full_solid_failed: > kfree(psy->full_trig_name); > full_failed: > @@ -101,10 +124,12 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy) > led_trigger_unregister_simple(psy->charging_trig); > led_trigger_unregister_simple(psy->full_trig); > led_trigger_unregister_simple(psy->charging_blink_full_solid_trig); > + led_trigger_unregister_simple(psy->charging_red_full_green_trig); > kfree(psy->charging_blink_full_solid_trig_name); > kfree(psy->full_trig_name); > kfree(psy->charging_trig_name); > kfree(psy->charging_full_trig_name); > + kfree(psy->charging_red_full_green_trig_name); > } > > /* Generated power specific LEDs triggers. */ > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index c0992a77feea..1d7c0b43070f 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -318,6 +318,8 @@ struct power_supply { > char *online_trig_name; > struct led_trigger *charging_blink_full_solid_trig; > char *charging_blink_full_solid_trig_name; > + struct led_trigger *charging_red_full_green_trig; > + char *charging_red_full_green_trig_name; > #endif > }; > > -- > 2.44.0 > >
Hi On Sat, Mar 30, 2024 at 12:24 AM Sebastian Reichel <sre@kernel.org> wrote: > > Hello Kate, > > On Sun, Mar 24, 2024 at 11:01:06PM +0800, Kate Hsuan wrote: > > Add a charging_red_full_green LED trigger and the trigger is based on > > led_mc_trigger_event() which can set an RGB LED when the trigger is > > triggered. The LED will show red when the battery status is charging. > > The LED will show green when the battery status is full. > > > > Link: https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t > > Signed-off-by: Kate Hsuan <hpa@redhat.com> > > --- > > Have you considered using orange instead of red? Using orange as > charging indicator seems to be more common nowadays and allows Sounds good. I'll change the color for them. Thank you > > green = battery full > orange = battery charging > red = battery empty / battery dead / error > > Greetings, > > -- Sebastian > > > drivers/power/supply/power_supply_leds.c | 25 ++++++++++++++++++++++++ > > include/linux/power_supply.h | 2 ++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c > > index c7db29d5fcb8..bd9c8fec5870 100644 > > --- a/drivers/power/supply/power_supply_leds.c > > +++ b/drivers/power/supply/power_supply_leds.c > > @@ -22,6 +22,8 @@ > > static void power_supply_update_bat_leds(struct power_supply *psy) > > { > > union power_supply_propval status; > > + unsigned int intensity_green[3] = {255, 0, 0}; > > + unsigned int intensity_red[3] = {0, 0, 255}; > > > > if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status)) > > return; > > @@ -36,12 +38,20 @@ static void power_supply_update_bat_leds(struct power_supply *psy) > > /* Going from blink to LED on requires a LED_OFF event to stop blink */ > > led_trigger_event(psy->charging_blink_full_solid_trig, LED_OFF); > > led_trigger_event(psy->charging_blink_full_solid_trig, LED_FULL); > > + led_mc_trigger_event(psy->charging_red_full_green_trig, > > + intensity_green, > > + 3, > > + LED_FULL); > > break; > > case POWER_SUPPLY_STATUS_CHARGING: > > led_trigger_event(psy->charging_full_trig, LED_FULL); > > led_trigger_event(psy->charging_trig, LED_FULL); > > led_trigger_event(psy->full_trig, LED_OFF); > > led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0); > > + led_mc_trigger_event(psy->charging_red_full_green_trig, > > + intensity_red, > > + 3, > > + LED_FULL); > > break; > > default: > > led_trigger_event(psy->charging_full_trig, LED_OFF); > > @@ -49,6 +59,10 @@ static void power_supply_update_bat_leds(struct power_supply *psy) > > led_trigger_event(psy->full_trig, LED_OFF); > > led_trigger_event(psy->charging_blink_full_solid_trig, > > LED_OFF); > > + led_mc_trigger_event(psy->charging_red_full_green_trig, > > + intensity_red, > > + 3, > > + LED_OFF); > > break; > > } > > } > > @@ -74,6 +88,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy) > > if (!psy->charging_blink_full_solid_trig_name) > > goto charging_blink_full_solid_failed; > > > > + psy->charging_red_full_green_trig_name = kasprintf(GFP_KERNEL, > > + "%s-charging-red-full-green", psy->desc->name); > > + if (!psy->charging_red_full_green_trig_name) > > + goto charging_red_full_green_failed; > > + > > led_trigger_register_simple(psy->charging_full_trig_name, > > &psy->charging_full_trig); > > led_trigger_register_simple(psy->charging_trig_name, > > @@ -82,9 +101,13 @@ static int power_supply_create_bat_triggers(struct power_supply *psy) > > &psy->full_trig); > > led_trigger_register_simple(psy->charging_blink_full_solid_trig_name, > > &psy->charging_blink_full_solid_trig); > > + led_trigger_register_simple(psy->charging_red_full_green_trig_name, > > + &psy->charging_red_full_green_trig); > > > > return 0; > > > > +charging_red_full_green_failed: > > + kfree(psy->charging_blink_full_solid_trig_name); > > charging_blink_full_solid_failed: > > kfree(psy->full_trig_name); > > full_failed: > > @@ -101,10 +124,12 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy) > > led_trigger_unregister_simple(psy->charging_trig); > > led_trigger_unregister_simple(psy->full_trig); > > led_trigger_unregister_simple(psy->charging_blink_full_solid_trig); > > + led_trigger_unregister_simple(psy->charging_red_full_green_trig); > > kfree(psy->charging_blink_full_solid_trig_name); > > kfree(psy->full_trig_name); > > kfree(psy->charging_trig_name); > > kfree(psy->charging_full_trig_name); > > + kfree(psy->charging_red_full_green_trig_name); > > } > > > > /* Generated power specific LEDs triggers. */ > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > > index c0992a77feea..1d7c0b43070f 100644 > > --- a/include/linux/power_supply.h > > +++ b/include/linux/power_supply.h > > @@ -318,6 +318,8 @@ struct power_supply { > > char *online_trig_name; > > struct led_trigger *charging_blink_full_solid_trig; > > char *charging_blink_full_solid_trig_name; > > + struct led_trigger *charging_red_full_green_trig; > > + char *charging_red_full_green_trig_name; > > #endif > > }; > > > > -- > > 2.44.0 > > > >
This patch added the support for Xiaomi Pad2 indicator LED. This work included: 1. Added the KTD2026 swnode description to describe the LED controller. 2. Migrated the original driver to fwnode to support x86 platform. 3. Support for multi-color LED trigger event. 4. The LED will be red when charging and the LED will be green when the battery is full. Moreover, the LED trigger is set to the new trigger, called "bq27520-0-charging-red-full-green" for Xiaomi Pad2 so the LED will be red when charging and the LED will be green when the battery is full. The new LED API led_mc_trigger_event() can be found in the following URL. https://lore.kernel.org/linux-leds/f40a0b1a-ceac-e269-c2dd-0158c5b4a1ad@gmail.com/T/#t -- Changes in v5: 1. Fix swnode LED color settings. 2. Improve the driver based on the comments. 3. Introduce a LED new API- led_mc_trigger_event() to make the LED color can be changed according to the trigger. 4. Introduced a new trigger "charging-red-full-green". The LED will be red when charging and the the LED will be green when the battery is full. 5. Set the default trigger to "bq27520-0-charging-red-full-green" for Xiaomi Pad2. Changes in v4: 1. Fix double casting. 2. Since force casting a pointer value to int will trigger a compiler warning, the type of num_leds was changed to unsigned long. Changes in v3: 1. Drop the patch "leds-ktd202x: Skip regulator settings for Xiaomi pad2" Changes in v2: 1. Typo and style fixes. 2. The patch 0003 skips all the regulator setup for Xiaomi pad2 since KTD2026 on Xiaomi pad2 is already powered by BP25890RTWR. So, the sleep can be removed when removing the module. Hans de Goede (2): leds: core: Add led_mc_set_brightness() function leds: trigger: Add led_mc_trigger_event() function Kate Hsuan (4): platform: x86-android-tablets: other: Add swnode for Xiaomi pad2 indicator LED leds: rgb: leds-ktd202x: Get device properties through fwnode to support ACPI power: supply: power-supply-leds: Add charging_red_full_green trigger for RGB LED platform: x86-android-tablets: others: Set the LED trigger to charging_red_full_green for Xiaomi pad2 drivers/leds/led-class-multicolor.c | 1 + drivers/leds/led-core.c | 31 +++++++ drivers/leds/led-triggers.c | 20 +++++ drivers/leds/rgb/Kconfig | 1 - drivers/leds/rgb/leds-ktd202x.c | 75 ++++++++++------- .../platform/x86/x86-android-tablets/other.c | 82 +++++++++++++++++++ .../x86/x86-android-tablets/shared-psy-info.h | 2 + drivers/power/supply/power_supply_leds.c | 25 ++++++ include/linux/leds.h | 26 ++++++ include/linux/power_supply.h | 2 + 10 files changed, 235 insertions(+), 30 deletions(-)