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

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

[Ognjen Galic]

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
+		 * 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)
+			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

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

[Christoph Böhmwalder]

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

> +		 * 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?

> +			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

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

[Andy Shevchenko]

On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder
<christoph@boehmwalder.at> 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

>> +             /* When setting inhbitit charge, we set a default vaulue of
>
> This comment does not adhere to the Linux coding style

While you are right in principle, the whole driver is so old and uses
this style. So, for such cases we, as maintainers, prefer less
deviation work, i.e. keeping the
style is a good thing to do.

>> +             /* 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?

Since it's about integer-as-a-boolean, test for bit 0 would be
sufficient, i.e. ~BIT(0). Though, I find this form not so readable
since the input comes actually from the user.
It would be nice to have just kstrtobool() called instead for such
options, but see above. It would need a (huge) refactoring of the
driver first.

-- 
With Best Regards,
Andy Shevchenko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder
<christoph(a)boehmwalder.at> 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

>> +             /* When setting inhbitit charge, we set a default vaulue of
>
> This comment does not adhere to the Linux coding style

While you are right in principle, the whole driver is so old and uses
this style. So, for such cases we, as maintainers, prefer less
deviation work, i.e. keeping the
style is a good thing to do.

>> +             /* 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?

Since it's about integer-as-a-boolean, test for bit 0 would be
sufficient, i.e. ~BIT(0). Though, I find this form not so readable
since the input comes actually from the user.
It would be nice to have just kstrtobool() called instead for such
options, but see above. It would need a (huge) refactoring of the
driver first.

-- 
With Best Regards,
Andy Shevchenko

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

[Henrique de Moraes Holschuh]

On Mon, 14 May 2018, Christoph Böhmwalder wrote:
> > +	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;

Do we know what is in the other bits?  If so, please document the ACPI
method using a comment somewhere in the driver code, like you did for
SET_INHIBIT.

> >  	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

Much on the driver doesn't, because it is _OLD_.  But yeah, it is
preferrable to fix this as we add code, so it would be good to have all
new (and modified) comments switched to modern kernel style.

> > +	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?

Indeed... with a comment that says 0 = main battery, 1 = extra/dock
battery or something.

-- 
  Henrique Holschuh

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

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

[Ognjen Galić]

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

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

[Ognjen Galić]

On Mon, May 14, 2018 at 01:41:33PM +0300, Andy Shevchenko wrote:
> On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder
> <christoph-KeW1gJXA36yNJhrcwGid2A@public.gmane.org> 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
> 
> >> +             /* When setting inhbitit charge, we set a default vaulue of
> >
> > This comment does not adhere to the Linux coding style
> 
> While you are right in principle, the whole driver is so old and uses
> this style. So, for such cases we, as maintainers, prefer less
> deviation work, i.e. keeping the
> style is a good thing to do.

That's what I did, follow the driver style.

> 
> >> +             /* 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?
> 
> Since it's about integer-as-a-boolean, test for bit 0 would be
> sufficient, i.e. ~BIT(0). 

That seems uncessarily complicated. Whats wrong with LT and GT
operators?

> Though, I find this form not so readable
> since the input comes actually from the user.

I agree.

> It would be nice to have just kstrtobool() called instead for such
> options, but see above. It would need a (huge) refactoring of the
> driver first.

That needs a whole lot of refactoring for no real functional benefit.

> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

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

[Ognjen Galić]

On Mon, May 14, 2018 at 08:39:28AM -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 14 May 2018, Christoph Böhmwalder wrote:
> > > +	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;
> 
> Do we know what is in the other bits?  If so, please document the ACPI
> method using a comment somewhere in the driver code, like you did for
> SET_INHIBIT.

I got the specs for the methods in a Lenovo doc I was told not to share
around, including information in it.

> 
> > >  	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
> 
> Much on the driver doesn't, because it is _OLD_.  But yeah, it is
> preferrable to fix this as we add code, so it would be good to have all
> new (and modified) comments switched to modern kernel style.

So what do you people want me to do? Should I fix the comments or leave
as-is?

> 
> > > +	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?
> 
> Indeed... with a comment that says 0 = main battery, 1 = extra/dock
> battery or something.

That seems like obfuscation to me, this way its clear that it must be
either 1 or 0. And inhibiting is set per-battery so 1 is on and not
battery 1.

> 
> -- 
>   Henrique Holschuh

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

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

[Henrique de Moraes Holschuh]

On Mon, 14 May 2018, Ognjen Galić wrote:
> On Mon, May 14, 2018 at 08:39:28AM -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 14 May 2018, Christoph Böhmwalder wrote:
> > > > +	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;
> > 
> > Do we know what is in the other bits?  If so, please document the ACPI
> > method using a comment somewhere in the driver code, like you did for
> > SET_INHIBIT.
> 
> I got the specs for the methods in a Lenovo doc I was told not to share
> around, including information in it.

Hmm, ok. Please send to me in private the email address of the person I
should ask for the documentation since you cannot distribute the
documentation itself.  This isn't the first time it happened.

The usual rule I follow is: document what is either already being used
in the driver or which very likely wil have to be used in the driver.
Unrelated stuff (e.g. for some unrelated functionality the driver does
not implement) doesn't have to be documented.

In this case, we need to at least know what bits need to be RMW, or
always-zero when calling SET_* methods...

> So what do you people want me to do? Should I fix the comments or leave
> as-is?

Since the subsystem maintainer said it should be kept as-is, and I don't
much care, please keep it as-is.  It won't matter for my driver
maintainer ack :-)

> > > > +	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?
> > 
> > Indeed... with a comment that says 0 = main battery, 1 = extra/dock
> > battery or something.
> 
> That seems like obfuscation to me, this way its clear that it must be
> either 1 or 0. And inhibiting is set per-battery so 1 is on and not
> battery 1.

Hmm, yes I got that wrong. "battery" should use 0 <= battery <2 for
checking (if needed), but value can only be 0 or 1.

I'd personally have used a bitmask test for "value", ensuring all other
bits are zero, though.  But that's just a matter of taste and I don't
think any of the choices (including the one you used) are "bad taste",
so all of them are fine as far as I am concerned.

-- 
  Henrique Holschuh

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel