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

[Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>]

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