Re: [PATCH 1/3] thinkpad_acpi: add support for inhibit_charge

["Ognjen Galić" <smclt30p-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Mon, May 14, 2018 at 11:46:20AM +0200, Christoph Böhmwalder wrote:
> On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote:
> > Lenovo ThinkPad systems support the prevention of
> > battery charging via a manual override called Inhibit Charge.
> > 
> > This patch adds support for that attribute and exposes it via the
> > battery ACPI driver in the generic location:
> > 
> > /sys/class/power_supply/BATX/inhibit_charge
> > 
> > Signed-off-by: Ognjen Galic <smclt30p-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++-
> >  1 file changed, 64 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> > index da1ca485..b8b74889 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -9233,6 +9233,11 @@ static struct ibm_struct mute_led_driver_data = {
> >  #define GET_STOP	"BCSG"
> >  #define SET_STOP	"BCSS"
> >  
> > +#define SET_INHIBIT	"BICS"
> > +#define GET_INHIBIT	"BICG"
> > +
> > +#define INHIBIT_ATTR	"inhibit_charge"
> > +
> >  #define START_ATTR "charge_start_threshold"
> >  #define STOP_ATTR  "charge_stop_threshold"
> >  
> > @@ -9251,6 +9256,7 @@ enum {
> >  	/* This is used in the get/set helpers */
> >  	THRESHOLD_START,
> >  	THRESHOLD_STOP,
> > +	INHIBIT_CHARGE
> >  };
> >  
> >  struct tpacpi_battery_data {
> > @@ -9258,6 +9264,7 @@ struct tpacpi_battery_data {
> >  	int start_support;
> >  	int charge_stop;
> >  	int stop_support;
> > +	int inhibit_support;
> >  };
> >  
> >  struct tpacpi_battery_driver_data {
> > @@ -9315,6 +9322,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
> >  		if (*ret == 0)
> >  			*ret = 100;
> >  		return 0;
> > +	case INHIBIT_CHARGE:
> > +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
> > +			return -ENODEV;
> > +
> > +		/* The inhibit charge status is in the first bit */
> > +		*ret = *ret & 0x01;
> > +		return 0;
> >  	default:
> >  		pr_crit("wrong parameter: %d", what);
> >  		return -EINVAL;
> > @@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
> >  			return -ENODEV;
> >  		}
> >  		return 0;
> > +	case INHIBIT_CHARGE:
> > +		/* When setting inhbitit charge, we set a default vaulue of
> 
> This comment does not adhere to the Linux coding style

I followed the style in the driver.

> 
> > +		 * always breaking on AC detach and the effective time is set to
> > +		 * be permanent.
> > +		 * The battery ID is in bits 4-5, 2 bits and the effective time
> > +		 * is in bits 8-23, 2 bytes. A time of FFFF indicates forever.
> > +		 */
> > +		param = value;
> > +		param |= battery << 4;
> > +		param |= 0xFFFF << 8;
> > +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param)) {
> > +			pr_err("failed to set inhibit charge on %d", battery);
> > +			return -ENODEV;
> > +		}
> > +		return 0;
> >  	default:
> >  		pr_crit("wrong parameter: %d", what);
> >  		return -EINVAL;
> > @@ -9359,6 +9388,8 @@ static int tpacpi_battery_probe(int battery)
> >  	 * 2) Check for support
> >  	 * 3) Get the current stop threshold
> >  	 * 4) Check for support
> > +	 * 5) Check for inhibit charge support
> > +	 * 6) Get the current inhibit charge status
> >  	 */
> >  	if (acpi_has_method(hkey_handle, GET_START)) {
> >  		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
> > @@ -9395,10 +9426,16 @@ static int tpacpi_battery_probe(int battery)
> >  			return -ENODEV;
> >  		}
> >  	}
> > -	pr_info("battery %d registered (start %d, stop %d)",
> > +	if (acpi_has_method(hkey_handle, GET_INHIBIT))
> > +		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery)))
> > +			/* Support is marked in bit 5 */
> > +			battery_info.batteries[battery].inhibit_support = ret & BIT(5);
> > +
> > +	pr_info("battery %d registered (start %d, stop %d, inhibit: %d)",
> >  			battery,
> >  			battery_info.batteries[battery].charge_start,
> > -			battery_info.batteries[battery].charge_stop);
> > +			battery_info.batteries[battery].charge_stop,
> > +			battery_info.batteries[battery].inhibit_support);
> >  
> >  	return 0;
> >  }
> > @@ -9484,6 +9521,15 @@ static ssize_t tpacpi_battery_store(int what,
> >  		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
> >  			return -EINVAL;
> >  		return count;
> > +	case INHIBIT_CHARGE:
> > +		if (!battery_info.batteries[battery].inhibit_support)
> > +			return -ENODEV;
> > +		/* The only valid values are 1 and 0 */
> > +		if (value != 0 && value != 1)
> 
> I'm not sure, but maybe `if (value < 2)` is better here?

I think the exisiting validation is just fine. Anything other is
cosmetic and obfuscation IMO.

> 
> > +			return -EINVAL;
> > +		if (tpacpi_battery_set(INHIBIT_CHARGE, battery, value))
> > +			return -ENODEV;
> > +		return count;
> >  	default:
> >  		pr_crit("Wrong parameter: %d", what);
> >  		return -EINVAL;
> > @@ -9546,12 +9592,28 @@ static ssize_t charge_stop_threshold_store(struct device *dev,
> >  	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
> >  }
> >  
> > +static ssize_t inhibit_charge_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	return tpacpi_battery_store(INHIBIT_CHARGE, dev, buf, count);
> > +}
> > +
> > +static ssize_t inhibit_charge_show(struct device *device,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	return tpacpi_battery_show(INHIBIT_CHARGE, device, buf);
> > +}
> > +
> >  static DEVICE_ATTR_RW(charge_start_threshold);
> >  static DEVICE_ATTR_RW(charge_stop_threshold);
> > +static DEVICE_ATTR_RW(inhibit_charge);
> >  
> >  static struct attribute *tpacpi_battery_attrs[] = {
> >  	&dev_attr_charge_start_threshold.attr,
> >  	&dev_attr_charge_stop_threshold.attr,
> > +	&dev_attr_inhibit_charge.attr,
> >  	NULL,
> >  };
> >  
> > -- 
> > 2.17.0
> > 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot