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

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

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