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

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

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