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@xxxxxxxxxxxxxx> 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
