| Message ID | 20250311165406.331046-6-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | hwmon: (oxpsensors) Add devices, features, fix ABI and move to platform/x86 | expand |
On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote: > > OneXPlayer devices have a charge bypass feature > that allows the user to select between it being > active always or only when the device is on. > > Therefore, add attribute inhibit-charge-s0 to > charge_behaviour to allow the user to select > that bypass should only be on when the device is > in the s0 state. > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- > drivers/power/supply/power_supply_sysfs.c | 1 + > drivers/power/supply/test_power.c | 1 + > include/linux/power_supply.h | 1 + > 4 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > index 2a5c1a09a28f..4a187ca11f92 100644 > --- a/Documentation/ABI/testing/sysfs-class-power > +++ b/Documentation/ABI/testing/sysfs-class-power > @@ -508,11 +508,12 @@ Description: > Access: Read, Write > > Valid values: > - ================ ==================================== > - auto: Charge normally, respect thresholds > - inhibit-charge: Do not charge while AC is attached > - force-discharge: Force discharge while AC is attached > - ================ ==================================== > + ================== ===================================== > + auto: Charge normally, respect thresholds > + inhibit-charge: Do not charge while AC is attached > + inhibit-charge-s0: same as inhibit-charge but only in S0 > + force-discharge: Force discharge while AC is attached > + ================== ===================================== > > What: /sys/class/power_supply/<supply_name>/technology > Date: May 2007 > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index edb058c19c9c..1a98fc26ce96 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { > static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { > [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", > [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", > + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", > [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", > }; > > diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c > index 2a975a110f48..4bc5ab84a9d6 100644 > --- a/drivers/power/supply/test_power.c > +++ b/drivers/power/supply/test_power.c > @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { > .property_is_writeable = test_power_battery_property_is_writeable, > .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) > + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), > }, > [TEST_USB] = { > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index 6ed53b292162..b1ca5e148759 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -212,6 +212,7 @@ enum power_supply_usb_type { > enum power_supply_charge_behaviour { > POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, > POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, > + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, > POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, > }; > > -- > 2.48.1 > Hi Guenter, I think I need an ack here, and then someone from platform-x86 to triage the series. Do I need to cc anyone extra? Thanks, Antheas
On 3/16/25 04:40, Antheas Kapenekakis wrote: > On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote: >> >> OneXPlayer devices have a charge bypass feature >> that allows the user to select between it being >> active always or only when the device is on. >> >> Therefore, add attribute inhibit-charge-s0 to >> charge_behaviour to allow the user to select >> that bypass should only be on when the device is >> in the s0 state. >> >> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> >> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >> --- >> Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- >> drivers/power/supply/power_supply_sysfs.c | 1 + >> drivers/power/supply/test_power.c | 1 + >> include/linux/power_supply.h | 1 + >> 4 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >> index 2a5c1a09a28f..4a187ca11f92 100644 >> --- a/Documentation/ABI/testing/sysfs-class-power >> +++ b/Documentation/ABI/testing/sysfs-class-power >> @@ -508,11 +508,12 @@ Description: >> Access: Read, Write >> >> Valid values: >> - ================ ==================================== >> - auto: Charge normally, respect thresholds >> - inhibit-charge: Do not charge while AC is attached >> - force-discharge: Force discharge while AC is attached >> - ================ ==================================== >> + ================== ===================================== >> + auto: Charge normally, respect thresholds >> + inhibit-charge: Do not charge while AC is attached >> + inhibit-charge-s0: same as inhibit-charge but only in S0 >> + force-discharge: Force discharge while AC is attached >> + ================== ===================================== >> >> What: /sys/class/power_supply/<supply_name>/technology >> Date: May 2007 >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >> index edb058c19c9c..1a98fc26ce96 100644 >> --- a/drivers/power/supply/power_supply_sysfs.c >> +++ b/drivers/power/supply/power_supply_sysfs.c >> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { >> static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", >> + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", >> }; >> >> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c >> index 2a975a110f48..4bc5ab84a9d6 100644 >> --- a/drivers/power/supply/test_power.c >> +++ b/drivers/power/supply/test_power.c >> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { >> .property_is_writeable = test_power_battery_property_is_writeable, >> .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) >> + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), >> }, >> [TEST_USB] = { >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h >> index 6ed53b292162..b1ca5e148759 100644 >> --- a/include/linux/power_supply.h >> +++ b/include/linux/power_supply.h >> @@ -212,6 +212,7 @@ enum power_supply_usb_type { >> enum power_supply_charge_behaviour { >> POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, >> POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, >> + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, >> POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, >> }; >> >> -- >> 2.48.1 >> > > Hi Guenter, > I think I need an ack here, and then someone from platform-x86 to > triage the series. > > Do I need to cc anyone extra? > You need to cc the maintainers of affected subsystems. Copying the mailing list is insufficient. Guenter
On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/16/25 04:40, Antheas Kapenekakis wrote: > > On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote: > >> > >> OneXPlayer devices have a charge bypass feature > >> that allows the user to select between it being > >> active always or only when the device is on. > >> > >> Therefore, add attribute inhibit-charge-s0 to > >> charge_behaviour to allow the user to select > >> that bypass should only be on when the device is > >> in the s0 state. > >> > >> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > >> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > >> --- > >> Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- > >> drivers/power/supply/power_supply_sysfs.c | 1 + > >> drivers/power/supply/test_power.c | 1 + > >> include/linux/power_supply.h | 1 + > >> 4 files changed, 9 insertions(+), 5 deletions(-) > >> > >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > >> index 2a5c1a09a28f..4a187ca11f92 100644 > >> --- a/Documentation/ABI/testing/sysfs-class-power > >> +++ b/Documentation/ABI/testing/sysfs-class-power > >> @@ -508,11 +508,12 @@ Description: > >> Access: Read, Write > >> > >> Valid values: > >> - ================ ==================================== > >> - auto: Charge normally, respect thresholds > >> - inhibit-charge: Do not charge while AC is attached > >> - force-discharge: Force discharge while AC is attached > >> - ================ ==================================== > >> + ================== ===================================== > >> + auto: Charge normally, respect thresholds > >> + inhibit-charge: Do not charge while AC is attached > >> + inhibit-charge-s0: same as inhibit-charge but only in S0 > >> + force-discharge: Force discharge while AC is attached > >> + ================== ===================================== > >> > >> What: /sys/class/power_supply/<supply_name>/technology > >> Date: May 2007 > >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > >> index edb058c19c9c..1a98fc26ce96 100644 > >> --- a/drivers/power/supply/power_supply_sysfs.c > >> +++ b/drivers/power/supply/power_supply_sysfs.c > >> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { > >> static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", > >> + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", > >> }; > >> > >> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c > >> index 2a975a110f48..4bc5ab84a9d6 100644 > >> --- a/drivers/power/supply/test_power.c > >> +++ b/drivers/power/supply/test_power.c > >> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { > >> .property_is_writeable = test_power_battery_property_is_writeable, > >> .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) > >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) > >> + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) > >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), > >> }, > >> [TEST_USB] = { > >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > >> index 6ed53b292162..b1ca5e148759 100644 > >> --- a/include/linux/power_supply.h > >> +++ b/include/linux/power_supply.h > >> @@ -212,6 +212,7 @@ enum power_supply_usb_type { > >> enum power_supply_charge_behaviour { > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, > >> + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, > >> }; > >> > >> -- > >> 2.48.1 > >> > > > > Hi Guenter, > > I think I need an ack here, and then someone from platform-x86 to > > triage the series. > > > > Do I need to cc anyone extra? > > > > You need to cc the maintainers of affected subsystems. Copying the mailing > list is insufficient. > > Guenter > Can you tell me who to cc from platform-x86 and linux-pm? Is it Armin and Rafael? Best, Antheas
On Sun, 16 Mar 2025 at 17:50, Derek John Clark <derekjohn.clark@gmail.com> wrote: > > > > On Sun, Mar 16, 2025, 09:47 Antheas Kapenekakis <lkml@antheas.dev> wrote: >> >> On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote: >> > >> > On 3/16/25 04:40, Antheas Kapenekakis wrote: >> > > On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote: >> > >> >> > >> OneXPlayer devices have a charge bypass feature >> > >> that allows the user to select between it being >> > >> active always or only when the device is on. >> > >> >> > >> Therefore, add attribute inhibit-charge-s0 to >> > >> charge_behaviour to allow the user to select >> > >> that bypass should only be on when the device is >> > >> in the s0 state. >> > >> >> > >> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> >> > >> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >> > >> --- >> > >> Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- >> > >> drivers/power/supply/power_supply_sysfs.c | 1 + >> > >> drivers/power/supply/test_power.c | 1 + >> > >> include/linux/power_supply.h | 1 + >> > >> 4 files changed, 9 insertions(+), 5 deletions(-) >> > >> >> > >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >> > >> index 2a5c1a09a28f..4a187ca11f92 100644 >> > >> --- a/Documentation/ABI/testing/sysfs-class-power >> > >> +++ b/Documentation/ABI/testing/sysfs-class-power >> > >> @@ -508,11 +508,12 @@ Description: >> > >> Access: Read, Write >> > >> >> > >> Valid values: >> > >> - ================ ==================================== >> > >> - auto: Charge normally, respect thresholds >> > >> - inhibit-charge: Do not charge while AC is attached >> > >> - force-discharge: Force discharge while AC is attached >> > >> - ================ ==================================== >> > >> + ================== ===================================== >> > >> + auto: Charge normally, respect thresholds >> > >> + inhibit-charge: Do not charge while AC is attached >> > >> + inhibit-charge-s0: same as inhibit-charge but only in S0 >> > >> + force-discharge: Force discharge while AC is attached >> > >> + ================== ===================================== >> > >> >> > >> What: /sys/class/power_supply/<supply_name>/technology >> > >> Date: May 2007 >> > >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >> > >> index edb058c19c9c..1a98fc26ce96 100644 >> > >> --- a/drivers/power/supply/power_supply_sysfs.c >> > >> +++ b/drivers/power/supply/power_supply_sysfs.c >> > >> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { >> > >> static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { >> > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", >> > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", >> > >> + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", >> > >> [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", >> > >> }; >> > >> >> > >> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c >> > >> index 2a975a110f48..4bc5ab84a9d6 100644 >> > >> --- a/drivers/power/supply/test_power.c >> > >> +++ b/drivers/power/supply/test_power.c >> > >> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { >> > >> .property_is_writeable = test_power_battery_property_is_writeable, >> > >> .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) >> > >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) >> > >> + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) >> > >> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), >> > >> }, >> > >> [TEST_USB] = { >> > >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h >> > >> index 6ed53b292162..b1ca5e148759 100644 >> > >> --- a/include/linux/power_supply.h >> > >> +++ b/include/linux/power_supply.h >> > >> @@ -212,6 +212,7 @@ enum power_supply_usb_type { >> > >> enum power_supply_charge_behaviour { >> > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, >> > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, >> > >> + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, >> > >> POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, >> > >> }; >> > >> >> > >> -- >> > >> 2.48.1 >> > >> >> > > >> > > Hi Guenter, >> > > I think I need an ack here, and then someone from platform-x86 to >> > > triage the series. >> > > >> > > Do I need to cc anyone extra? >> > > >> > >> > You need to cc the maintainers of affected subsystems. Copying the mailing >> > list is insufficient. >> > >> > Guenter >> > >> >> Can you tell me who to cc from platform-x86 and linux-pm? >> >> Is it Armin and Rafael? > > > Hans, Ilpo, Armin, and Mario > > - Derek Sure. Full series on [1]. Antheas btw, your reply was non-text. [1] https://lore.kernel.org/all/20250311165406.331046-1-lkml@antheas.dev/
On 3/16/25 09:46, Antheas Kapenekakis wrote: [ ... ] >>> Do I need to cc anyone extra? >>> >> >> You need to cc the maintainers of affected subsystems. Copying the mailing >> list is insufficient. >> >> Guenter >> > > Can you tell me who to cc from platform-x86 and linux-pm? > Just use scripts/get_maintainer.pl. Guenter
Hi Antheas, Thanks you for your work on this. On 16-Mar-25 17:46, Antheas Kapenekakis wrote: > On Sun, 16 Mar 2025 at 14:56, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 3/16/25 04:40, Antheas Kapenekakis wrote: >>> On Tue, 11 Mar 2025 at 17:54, Antheas Kapenekakis <lkml@antheas.dev> wrote: >>>> >>>> OneXPlayer devices have a charge bypass feature >>>> that allows the user to select between it being >>>> active always or only when the device is on. >>>> >>>> Therefore, add attribute inhibit-charge-s0 to >>>> charge_behaviour to allow the user to select >>>> that bypass should only be on when the device is >>>> in the s0 state. >>>> >>>> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> >>>> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> >>>> --- >>>> Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- >>>> drivers/power/supply/power_supply_sysfs.c | 1 + >>>> drivers/power/supply/test_power.c | 1 + >>>> include/linux/power_supply.h | 1 + >>>> 4 files changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >>>> index 2a5c1a09a28f..4a187ca11f92 100644 >>>> --- a/Documentation/ABI/testing/sysfs-class-power >>>> +++ b/Documentation/ABI/testing/sysfs-class-power >>>> @@ -508,11 +508,12 @@ Description: >>>> Access: Read, Write >>>> >>>> Valid values: >>>> - ================ ==================================== >>>> - auto: Charge normally, respect thresholds >>>> - inhibit-charge: Do not charge while AC is attached >>>> - force-discharge: Force discharge while AC is attached >>>> - ================ ==================================== >>>> + ================== ===================================== >>>> + auto: Charge normally, respect thresholds >>>> + inhibit-charge: Do not charge while AC is attached >>>> + inhibit-charge-s0: same as inhibit-charge but only in S0 >>>> + force-discharge: Force discharge while AC is attached >>>> + ================== ===================================== >>>> >>>> What: /sys/class/power_supply/<supply_name>/technology >>>> Date: May 2007 >>>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >>>> index edb058c19c9c..1a98fc26ce96 100644 >>>> --- a/drivers/power/supply/power_supply_sysfs.c >>>> +++ b/drivers/power/supply/power_supply_sysfs.c >>>> @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { >>>> static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { >>>> [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", >>>> [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", >>>> + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", >>>> [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", >>>> }; >>>> >>>> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c >>>> index 2a975a110f48..4bc5ab84a9d6 100644 >>>> --- a/drivers/power/supply/test_power.c >>>> +++ b/drivers/power/supply/test_power.c >>>> @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { >>>> .property_is_writeable = test_power_battery_property_is_writeable, >>>> .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) >>>> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) >>>> + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) >>>> | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), >>>> }, >>>> [TEST_USB] = { >>>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h >>>> index 6ed53b292162..b1ca5e148759 100644 >>>> --- a/include/linux/power_supply.h >>>> +++ b/include/linux/power_supply.h >>>> @@ -212,6 +212,7 @@ enum power_supply_usb_type { >>>> enum power_supply_charge_behaviour { >>>> POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, >>>> POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, >>>> + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, >>>> POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, >>>> }; >>>> >>>> -- >>>> 2.48.1 >>>> >>> >>> Hi Guenter, >>> I think I need an ack here, and then someone from platform-x86 to >>> triage the series. >>> >>> Do I need to cc anyone extra? >>> >> >> You need to cc the maintainers of affected subsystems. Copying the mailing >> list is insufficient. >> >> Guenter >> > > Can you tell me who to cc from platform-x86 and linux-pm? Sebastian Reichel <sre@kernel.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) linux-pm@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) linux-kernel@vger.kernel.org (open list) And something which the tools will not tell you, since charge_behaviour support was added by Thomas Thomas should be added to the Cc too: Thomas Weißschuh <linux@weissschuh.net> Note I also have some generic remarks about this patch, I'll do a top-level reply to the patch for those. Regards, Hans
Hi Antheas, On 11-Mar-25 17:53, Antheas Kapenekakis wrote: > OneXPlayer devices have a charge bypass The term "charge bypass" is typically used for the case where the external charger gets directly connected to the battery cells, bypassing the charge-IC inside the device, in making the external charger directly responsible for battery/charge management. Yet you name the feature inhibit charge, so I guess it simply disables charging of the battery rather then doing an actual chaerger-IC bypass ? Assuming I have this correct, please stop using the term charge-bypass as that has a specific (different) meaning. > feature > that allows the user to select between it being > active always or only when the device is on. > > Therefore, add attribute inhibit-charge-s0 to > charge_behaviour to allow the user to select > that bypass should only be on when the device is Also don't use bypass here please. > in the s0 state. > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- > drivers/power/supply/power_supply_sysfs.c | 1 + > drivers/power/supply/test_power.c | 1 + > include/linux/power_supply.h | 1 + > 4 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > index 2a5c1a09a28f..4a187ca11f92 100644 > --- a/Documentation/ABI/testing/sysfs-class-power > +++ b/Documentation/ABI/testing/sysfs-class-power > @@ -508,11 +508,12 @@ Description: > Access: Read, Write > > Valid values: > - ================ ==================================== > - auto: Charge normally, respect thresholds > - inhibit-charge: Do not charge while AC is attached > - force-discharge: Force discharge while AC is attached > - ================ ==================================== > + ================== ===================================== > + auto: Charge normally, respect thresholds > + inhibit-charge: Do not charge while AC is attached > + inhibit-charge-s0: same as inhibit-charge but only in S0 Only in S0 suggests that charging gets disabled when the device is on / in-use, I guess this is intended to avoid generating extra heat while the device is on? What about when the device is suspended, should the battery charge then ? On x86 we've 2 sorts of suspends S3, and the current name suggests that the device will charge (no inhibit) then. But modern hw almost always uses s0i3 / suspend to idle suspend and the name suggests charging would then still be inhibited? Also s0 is an ACPI specific term, so basically 2 remarks here: 1. The name should probably be "inhibit-charge-when-on" since the power_supply calls is platform agnositic and "S0" is not. 2. We need to clearly define what happens when the device is suspended and then make sure that the driver matches this (e.g. if we want to *not* inhibit during suspend we may need to turn this feature off during suspend). Regards, Hans > + force-discharge: Force discharge while AC is attached > + ================== ===================================== > > What: /sys/class/power_supply/<supply_name>/technology > Date: May 2007 > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index edb058c19c9c..1a98fc26ce96 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { > static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { > [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", > [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", > + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", > [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", > }; > > diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c > index 2a975a110f48..4bc5ab84a9d6 100644 > --- a/drivers/power/supply/test_power.c > +++ b/drivers/power/supply/test_power.c > @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { > .property_is_writeable = test_power_battery_property_is_writeable, > .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) > + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) > | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), > }, > [TEST_USB] = { > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index 6ed53b292162..b1ca5e148759 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -212,6 +212,7 @@ enum power_supply_usb_type { > enum power_supply_charge_behaviour { > POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, > POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, > + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, > POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, > }; >
On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Antheas, > > On 11-Mar-25 17:53, Antheas Kapenekakis wrote: > > OneXPlayer devices have a charge bypass > > The term "charge bypass" is typically used for the case where the > external charger gets directly connected to the battery cells, > bypassing the charge-IC inside the device, in making > the external charger directly responsible for battery/charge > management. > > Yet you name the feature inhibit charge, so I guess it simply > disables charging of the battery rather then doing an actual > chaerger-IC bypass ? > > Assuming I have this correct, please stop using the term > charge-bypass as that has a specific (different) meaning. Unfortunately, this is how the feature is called in Windows. On both OneXPlayer and Ayaneo. Manufacturers are centralizing around that term. Under the hood, it should be bypassing the charger circuitry, but it is not obvious during use. The user behavior mirrors `inhibit-charge`, as the battery just stops charging, so the endpoint is appropriate. The previous versions of this patch used the charge_type endpoint. However, it does not have autodetection, so it needs to be hardcoded [1]. There is also an ayaneo-platform out-of-tree driver with a PR for that endpoint, which is where I found out about it [2]. So I thought it would be more appropriate. I can rewrite the commit message to not include the word bypass. [1] https://github.com/hhd-dev/adjustor/blob/6a4a192898c4f9d8495e6554d1f0bc41fef550c7/src/adjustor/drivers/battery/__init__.py#L141-L156 [2] https://github.com/ShadowBlip/ayaneo-platform > > feature > > that allows the user to select between it being > > active always or only when the device is on. > > > > Therefore, add attribute inhibit-charge-s0 to > > charge_behaviour to allow the user to select > > that bypass should only be on when the device is > > Also don't use bypass here please. > > > in the s0 state. > > > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > Documentation/ABI/testing/sysfs-class-power | 11 ++++++----- > > drivers/power/supply/power_supply_sysfs.c | 1 + > > drivers/power/supply/test_power.c | 1 + > > include/linux/power_supply.h | 1 + > > 4 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > > index 2a5c1a09a28f..4a187ca11f92 100644 > > --- a/Documentation/ABI/testing/sysfs-class-power > > +++ b/Documentation/ABI/testing/sysfs-class-power > > @@ -508,11 +508,12 @@ Description: > > Access: Read, Write > > > > Valid values: > > - ================ ==================================== > > - auto: Charge normally, respect thresholds > > - inhibit-charge: Do not charge while AC is attached > > - force-discharge: Force discharge while AC is attached > > - ================ ==================================== > > + ================== ===================================== > > + auto: Charge normally, respect thresholds > > + inhibit-charge: Do not charge while AC is attached > > + inhibit-charge-s0: same as inhibit-charge but only in S0 > > Only in S0 suggests that charging gets disabled when the device is on / in-use, > I guess this is intended to avoid generating extra heat while the device is on? > > What about when the device is suspended, should the battery charge then ? > > On x86 we've 2 sorts of suspends S3, and the current name suggests that the > device will charge (no inhibit) then. But modern hw almost always uses > s0i3 / suspend to idle suspend and the name suggests charging would then > still be inhibited? > > Also s0 is an ACPI specific term, so basically 2 remarks here: > > 1. The name should probably be "inhibit-charge-when-on" since the power_supply > calls is platform agnositic and "S0" is not. I tried to be minimal. If we want to make the name longer, I vote for "inhibit-charge-awake". I can spin a v5 with that. The device does not charge while asleep. Only when it is off. > 2. We need to clearly define what happens when the device is suspended and then > make sure that the driver matches this (e.g. if we want to *not* inhibit during > suspend we may need to turn this feature off during suspend). This is handled by the device when it comes to OneXPlayer. No driver changes are needed. > Regards, > > Hans > > > snip > > > Best, Antheas
Hi, On 17-Mar-25 13:38, Antheas Kapenekakis wrote: > On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Antheas, >> >> On 11-Mar-25 17:53, Antheas Kapenekakis wrote: >>> OneXPlayer devices have a charge bypass >> >> The term "charge bypass" is typically used for the case where the >> external charger gets directly connected to the battery cells, >> bypassing the charge-IC inside the device, in making >> the external charger directly responsible for battery/charge >> management. >> >> Yet you name the feature inhibit charge, so I guess it simply >> disables charging of the battery rather then doing an actual >> chaerger-IC bypass ? >> >> Assuming I have this correct, please stop using the term >> charge-bypass as that has a specific (different) meaning. > > Unfortunately, this is how the feature is called in Windows. On both > OneXPlayer and Ayaneo. Manufacturers are centralizing around that > term. Ok, so I just did a quick duckduckgo for this and it looks like you are right. > Under the hood, it should be bypassing the charger circuitry, but it > is not obvious during use. Ack reading up on this it seems the idea is not to connect the external charger directly to the battery to allow fast-charging without the charge-IC inside the device adding heat, which is the traditional bypass mode. Instead the whole battery + charging-IC are cut out of the circuit (so bypassed) and the charger is now directly powering the device without the battery acting as a buffer if the power-draw superseeds what the external charger can deliver. > The user behavior mirrors `inhibit-charge`, > as the battery just stops charging, so the endpoint is appropriate. Hmm this new bypass mode indeed does seem to mirror inhibit charge from a user pov, but it does more. It reminds me of the battery disconnect option which some charge-ICs have which just puts the battery FET in high impedance mode effectively disconnecting the battery. Now that feature is intended for long term storage of devices with a builtin battery and it typically also immediately powers off the device ... Still I wonder if it would make sense to add a new "disconnect" charge_behaviour or charge_types enum value for this ? <snip> >>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >>> index 2a5c1a09a28f..4a187ca11f92 100644 >>> --- a/Documentation/ABI/testing/sysfs-class-power >>> +++ b/Documentation/ABI/testing/sysfs-class-power >>> @@ -508,11 +508,12 @@ Description: >>> Access: Read, Write >>> >>> Valid values: >>> - ================ ==================================== >>> - auto: Charge normally, respect thresholds >>> - inhibit-charge: Do not charge while AC is attached >>> - force-discharge: Force discharge while AC is attached >>> - ================ ==================================== >>> + ================== ===================================== >>> + auto: Charge normally, respect thresholds >>> + inhibit-charge: Do not charge while AC is attached >>> + inhibit-charge-s0: same as inhibit-charge but only in S0 >> >> Only in S0 suggests that charging gets disabled when the device is on / in-use, >> I guess this is intended to avoid generating extra heat while the device is on? >> >> What about when the device is suspended, should the battery charge then ? >> >> On x86 we've 2 sorts of suspends S3, and the current name suggests that the >> device will charge (no inhibit) then. But modern hw almost always uses >> s0i3 / suspend to idle suspend and the name suggests charging would then >> still be inhibited? >> >> Also s0 is an ACPI specific term, so basically 2 remarks here: >> >> 1. The name should probably be "inhibit-charge-when-on" since the power_supply >> calls is platform agnositic and "S0" is not. > > I tried to be minimal. If we want to make the name longer, I vote for > "inhibit-charge-awake". I can spin a v5 with that. > > The device does not charge while asleep. Only when it is off. Is suspend awake though ? >> 2. We need to clearly define what happens when the device is suspended and then >> make sure that the driver matches this (e.g. if we want to *not* inhibit during >> suspend we may need to turn this feature off during suspend). > > This is handled by the device when it comes to OneXPlayer. No driver > changes are needed. Well you say no charging is done when suspended, the question also is what behavior do we want here? I'm fine with the default behaviour, but a case could be made that charging while suspended might be desirable (dependent on the use case) in which case we would need to disable the inhibit when suspending to get the desired behavior. Also what if other firmware interfaces with a bypass^W inhibit option work differently and do charge during suspend ? It is important that we clearly define the expected behavior now so that future devices can be made to behave the same. Regards, Hans
On Mon, 17 Mar 2025 at 14:31, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 17-Mar-25 13:38, Antheas Kapenekakis wrote: > > On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi Antheas, > >> > >> On 11-Mar-25 17:53, Antheas Kapenekakis wrote: > >>> OneXPlayer devices have a charge bypass > >> > >> The term "charge bypass" is typically used for the case where the > >> external charger gets directly connected to the battery cells, > >> bypassing the charge-IC inside the device, in making > >> the external charger directly responsible for battery/charge > >> management. > >> > >> Yet you name the feature inhibit charge, so I guess it simply > >> disables charging of the battery rather then doing an actual > >> chaerger-IC bypass ? > >> > >> Assuming I have this correct, please stop using the term > >> charge-bypass as that has a specific (different) meaning. > > > > Unfortunately, this is how the feature is called in Windows. On both > > OneXPlayer and Ayaneo. Manufacturers are centralizing around that > > term. > > Ok, so I just did a quick duckduckgo for this and it looks like > you are right. > > > Under the hood, it should be bypassing the charger circuitry, but it > > is not obvious during use. > > Ack reading up on this it seems the idea is not to connect the external > charger directly to the battery to allow fast-charging without > the charge-IC inside the device adding heat, which is the traditional > bypass mode. > > Instead the whole battery + charging-IC are cut out of the circuit > (so bypassed) and the charger is now directly powering the device > without the battery acting as a buffer if the power-draw superseeds > what the external charger can deliver. > > > The user behavior mirrors `inhibit-charge`, > > as the battery just stops charging, so the endpoint is appropriate. > > Hmm this new bypass mode indeed does seem to mirror inhibit charge > from a user pov, but it does more. It reminds me of the battery disconnect > option which some charge-ICs have which just puts the battery FET in > high impedance mode effectively disconnecting the battery. Now that > feature is intended for long term storage of devices with a builtin > battery and it typically also immediately powers off the device ... > > Still I wonder if it would make sense to add a new "disconnect" > charge_behaviour or charge_types enum value for this ? > The battery is not disconnected. It still provides backup. Unplugging the charger does not turn off the device. So it is more murky. From a userspace perspective it is inhibit-charge 1-1. > > > <snip> > > >>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > >>> index 2a5c1a09a28f..4a187ca11f92 100644 > >>> --- a/Documentation/ABI/testing/sysfs-class-power > >>> +++ b/Documentation/ABI/testing/sysfs-class-power > >>> @@ -508,11 +508,12 @@ Description: > >>> Access: Read, Write > >>> > >>> Valid values: > >>> - ================ ==================================== > >>> - auto: Charge normally, respect thresholds > >>> - inhibit-charge: Do not charge while AC is attached > >>> - force-discharge: Force discharge while AC is attached > >>> - ================ ==================================== > >>> + ================== ===================================== > >>> + auto: Charge normally, respect thresholds > >>> + inhibit-charge: Do not charge while AC is attached > >>> + inhibit-charge-s0: same as inhibit-charge but only in S0 > >> > >> Only in S0 suggests that charging gets disabled when the device is on / in-use, > >> I guess this is intended to avoid generating extra heat while the device is on? > >> > >> What about when the device is suspended, should the battery charge then ? > >> > >> On x86 we've 2 sorts of suspends S3, and the current name suggests that the > >> device will charge (no inhibit) then. But modern hw almost always uses > >> s0i3 / suspend to idle suspend and the name suggests charging would then > >> still be inhibited? > >> > >> Also s0 is an ACPI specific term, so basically 2 remarks here: > >> > >> 1. The name should probably be "inhibit-charge-when-on" since the power_supply > >> calls is platform agnositic and "S0" is not. > > > > I tried to be minimal. If we want to make the name longer, I vote for > > "inhibit-charge-awake". I can spin a v5 with that. > > > > The device does not charge while asleep. Only when it is off. > > Is suspend awake though ? Sorry I mispoke. When inhibit-charge-awake, the device only charges while in s0i0. When inhibit-charge, it never charges. This includes s0i3, S4, and S5. The devices that support this only support modern standby. I just verified this. > >> 2. We need to clearly define what happens when the device is suspended and then > >> make sure that the driver matches this (e.g. if we want to *not* inhibit during > >> suspend we may need to turn this feature off during suspend). > > > > This is handled by the device when it comes to OneXPlayer. No driver > > changes are needed. > > Well you say no charging is done when suspended, the question also is what > behavior do we want here? I'm fine with the default behaviour, but a case > could be made that charging while suspended might be desirable (dependent on > the use case) in which case we would need to disable the inhibit when > suspending to get the desired behavior. > > Also what if other firmware interfaces with a bypass^W inhibit option work > differently and do charge during suspend ? > > It is important that we clearly define the expected behavior now so that > future devices can be made to behave the same. Sorry I mispoke. Charging happens under modern standby under -awake. So -awake would mean awake (s0i0) here. If other devices charge during sleep and awake, another option could be added. > Regards, > > Hans > >
Hi, On 17-Mar-25 14:50, Antheas Kapenekakis wrote: > On Mon, 17 Mar 2025 at 14:31, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 17-Mar-25 13:38, Antheas Kapenekakis wrote: >>> On Mon, 17 Mar 2025 at 13:27, Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi Antheas, >>>> >>>> On 11-Mar-25 17:53, Antheas Kapenekakis wrote: >>>>> OneXPlayer devices have a charge bypass >>>> >>>> The term "charge bypass" is typically used for the case where the >>>> external charger gets directly connected to the battery cells, >>>> bypassing the charge-IC inside the device, in making >>>> the external charger directly responsible for battery/charge >>>> management. >>>> >>>> Yet you name the feature inhibit charge, so I guess it simply >>>> disables charging of the battery rather then doing an actual >>>> chaerger-IC bypass ? >>>> >>>> Assuming I have this correct, please stop using the term >>>> charge-bypass as that has a specific (different) meaning. >>> >>> Unfortunately, this is how the feature is called in Windows. On both >>> OneXPlayer and Ayaneo. Manufacturers are centralizing around that >>> term. >> >> Ok, so I just did a quick duckduckgo for this and it looks like >> you are right. >> >>> Under the hood, it should be bypassing the charger circuitry, but it >>> is not obvious during use. >> >> Ack reading up on this it seems the idea is not to connect the external >> charger directly to the battery to allow fast-charging without >> the charge-IC inside the device adding heat, which is the traditional >> bypass mode. >> >> Instead the whole battery + charging-IC are cut out of the circuit >> (so bypassed) and the charger is now directly powering the device >> without the battery acting as a buffer if the power-draw superseeds >> what the external charger can deliver. >> >>> The user behavior mirrors `inhibit-charge`, >>> as the battery just stops charging, so the endpoint is appropriate. >> >> Hmm this new bypass mode indeed does seem to mirror inhibit charge >> from a user pov, but it does more. It reminds me of the battery disconnect >> option which some charge-ICs have which just puts the battery FET in >> high impedance mode effectively disconnecting the battery. Now that >> feature is intended for long term storage of devices with a builtin >> battery and it typically also immediately powers off the device ... >> >> Still I wonder if it would make sense to add a new "disconnect" >> charge_behaviour or charge_types enum value for this ? >> > > The battery is not disconnected. It still provides backup. Unplugging > the charger does not turn off the device. So it is more murky. > > From a userspace perspective it is inhibit-charge 1-1. Ok, lets go with inhibit then. >> <snip> >> >>>>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >>>>> index 2a5c1a09a28f..4a187ca11f92 100644 >>>>> --- a/Documentation/ABI/testing/sysfs-class-power >>>>> +++ b/Documentation/ABI/testing/sysfs-class-power >>>>> @@ -508,11 +508,12 @@ Description: >>>>> Access: Read, Write >>>>> >>>>> Valid values: >>>>> - ================ ==================================== >>>>> - auto: Charge normally, respect thresholds >>>>> - inhibit-charge: Do not charge while AC is attached >>>>> - force-discharge: Force discharge while AC is attached >>>>> - ================ ==================================== >>>>> + ================== ===================================== >>>>> + auto: Charge normally, respect thresholds >>>>> + inhibit-charge: Do not charge while AC is attached >>>>> + inhibit-charge-s0: same as inhibit-charge but only in S0 >>>> >>>> Only in S0 suggests that charging gets disabled when the device is on / in-use, >>>> I guess this is intended to avoid generating extra heat while the device is on? >>>> >>>> What about when the device is suspended, should the battery charge then ? >>>> >>>> On x86 we've 2 sorts of suspends S3, and the current name suggests that the >>>> device will charge (no inhibit) then. But modern hw almost always uses >>>> s0i3 / suspend to idle suspend and the name suggests charging would then >>>> still be inhibited? >>>> >>>> Also s0 is an ACPI specific term, so basically 2 remarks here: >>>> >>>> 1. The name should probably be "inhibit-charge-when-on" since the power_supply >>>> calls is platform agnositic and "S0" is not. >>> >>> I tried to be minimal. If we want to make the name longer, I vote for >>> "inhibit-charge-awake". I can spin a v5 with that. >>> >>> The device does not charge while asleep. Only when it is off. >> >> Is suspend awake though ? > > Sorry I mispoke. When inhibit-charge-awake, the device only charges > while in s0i0. When inhibit-charge, it never charges. This includes > s0i3, S4, and S5. The devices that support this only support modern > standby. > > I just verified this. Ok that sounds good / sane, it likely just disables charging while in s0i0 to avoid generating extra heat while in s0i0, so inhibit-charge-awake sounds good to me. > >>>> 2. We need to clearly define what happens when the device is suspended and then >>>> make sure that the driver matches this (e.g. if we want to *not* inhibit during >>>> suspend we may need to turn this feature off during suspend). >>> >>> This is handled by the device when it comes to OneXPlayer. No driver >>> changes are needed. >> >> Well you say no charging is done when suspended, the question also is what >> behavior do we want here? I'm fine with the default behaviour, but a case >> could be made that charging while suspended might be desirable (dependent on >> the use case) in which case we would need to disable the inhibit when >> suspending to get the desired behavior. >> >> Also what if other firmware interfaces with a bypass^W inhibit option work >> differently and do charge during suspend ? >> >> It is important that we clearly define the expected behavior now so that >> future devices can be made to behave the same. > > Sorry I mispoke. Charging happens under modern standby under -awake. > > So -awake would mean awake (s0i0) here. > > If other devices charge during sleep and awake, another option could be added. ack, as mentioned above inhibit-charge-awake sounds good to me, thank you for clarifying. Regards, Hans
diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power index 2a5c1a09a28f..4a187ca11f92 100644 --- a/Documentation/ABI/testing/sysfs-class-power +++ b/Documentation/ABI/testing/sysfs-class-power @@ -508,11 +508,12 @@ Description: Access: Read, Write Valid values: - ================ ==================================== - auto: Charge normally, respect thresholds - inhibit-charge: Do not charge while AC is attached - force-discharge: Force discharge while AC is attached - ================ ==================================== + ================== ===================================== + auto: Charge normally, respect thresholds + inhibit-charge: Do not charge while AC is attached + inhibit-charge-s0: same as inhibit-charge but only in S0 + force-discharge: Force discharge while AC is attached + ================== ===================================== What: /sys/class/power_supply/<supply_name>/technology Date: May 2007 diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index edb058c19c9c..1a98fc26ce96 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -140,6 +140,7 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = { static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { [POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO] = "auto", [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE] = "inhibit-charge", + [POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0] = "inhibit-charge-s0", [POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE] = "force-discharge", }; diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c index 2a975a110f48..4bc5ab84a9d6 100644 --- a/drivers/power/supply/test_power.c +++ b/drivers/power/supply/test_power.c @@ -214,6 +214,7 @@ static const struct power_supply_desc test_power_desc[] = { .property_is_writeable = test_power_battery_property_is_writeable, .charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE) + | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0) | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE), }, [TEST_USB] = { diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index 6ed53b292162..b1ca5e148759 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -212,6 +212,7 @@ enum power_supply_usb_type { enum power_supply_charge_behaviour { POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO = 0, POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE, + POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE_S0, POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE, };