Re: [RFC v2] standardized attributes for powersupply charge behaviour

["Thomas Weißschuh" <linux@weissschuh.net>]

Hi,

On 2021-11-12 19:10+0100, Sebastian Reichel wrote:
> On Mon, Nov 08, 2021 at 08:28:52PM +0100, Thomas Weißschuh wrote:
> > This a revised version of
> > "[RFC] add standardized attributes for force_discharge and inhibit_charge" [0],
> > incorporating discussion results.
> > 
> > The biggest change is the switch from two boolean attributes to a single
> > enum attribute.
> > 
> > [0] https://lore.kernel.org/platform-driver-x86/21569a89-8303-8573-05fb-c2fec29983d1@gmail.com/
> > ---
> >  Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++
> >  drivers/power/supply/power_supply_sysfs.c   |  1 +
> >  include/linux/power_supply.h                |  7 +++++++
> >  3 files changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> > index ca830c6cd809..2f58cfc91420 100644
> > --- a/Documentation/ABI/testing/sysfs-class-power
> > +++ b/Documentation/ABI/testing/sysfs-class-power
> > @@ -455,6 +455,20 @@ Description:
> >  			      "Unknown", "Charging", "Discharging",
> >  			      "Not charging", "Full"
> >  
> > +What:		/sys/class/power_supply/<supply_name>/charge_behaviour
> > +Date:		November 2021
> > +Contact:	linux-pm@vger.kernel.org
> > +Description:
> > +		Represents the charging behaviour.
> > +
> > +		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
> > +
> >  What:		/sys/class/power_supply/<supply_name>/technology
> >  Date:		May 2007
> >  Contact:	linux-pm@vger.kernel.org
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index c3d7cbcd4fad..26c60587dca1 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -172,6 +172,7 @@ static struct power_supply_attr power_supply_attrs[] = {
> >  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
> >  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
> >  	POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> > +	POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
> >  	POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
> >  	POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
> >  	POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> 
> this is missing (and should not compile without it):
> 
> static const char * const POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] = { ... };

The non-RFC version will have that.

> Otherwise LGTM. But you need to send API changes with an API user (i.e. the
> patch updating acpi battery driver using this).

I have an implementation for the thinkpad_acpi driver.

Is it fine if I export helper functions from power_supply_sysfs.c?

The problem is that thinkpad_acpi is using the acpi battery hooks which do
not allow other drivers to extend existing ACPI batteries with proper
powersupply properties but only plain sysfs attributes.

Currently I keep the parsing and formatting of the sysfs file inside
power_supply_sysfs.c, because that is where it belongs but export it to other
modules to use for their custom sysfs attributes.

I'm just not sure if this is acceptable, because no other function from
power_supply_sysfs.c is exported yet.

If it's not clear enough, we can also discuss this on the real patchset which
I'll probably submit tomorrow.

Thanks,
Thomas