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

[Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>]

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot