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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux