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, 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.

> >  	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.

> > +	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.

-- 
  Henrique Holschuh
--
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