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
