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
