On Mon, Jan 01, 2018 at 12:24:39PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 31, 2017 at 4:17 PM, Ognjen Galic <smclt30p@xxxxxxxxx> wrote:
> > thinkpad_acpi registers two new attributes for each battery:
> >
> > 1) Charge start threshold
> > /sys/class/power_supply/BATN/charge_start_threshold
> >
> > Valid values are [0, 99]. A value of 0 turns off the
> > start threshold wear control.
> >
> > 2) Charge stop threshold
> > /sys/class/power_supply/BATN/charge_stop_threshold
> >
> > Valid values are [1, 100]. A value of 100 turns off
> > the stop threshold wear control. This must be
> > configured first.
> >
>
> > This patch depends on the following patches:
> >
> > "battery: Add the battery hooking API"
>
> Since this is series, no need to put above into changelog.
>
> AFAICS it's not going to be backported either.
>
>
> > +/**
> > + * This evaluates a ACPI method call specific to the battery
> > + * ACPI extension. The specifics are that an error is marked
> > + * in the 32rd bit of the response, so we just check that here.
> > + *
> > + * Returns 0 on success
> > + */
> > +static int tpacpi_battery_acpi_eval(char *method, int *ret, int param)
> > +{
>
> One problem and one trick you may do.
>
> A problem: you return ACPI status as int. We have special type for
> that. So, be consistent with it.
> Looking to acpi_evalf() which is private to the module, I think you
> need to get rid of ACPI return codes here.
What do you mean by "get rid of ACPI return codes"?
Are you suggesting this?
static void tpacpi_battery_ac....
How would I check if the ACPI call failed if I made the function return
void?
I use a function return value to check if the ACPI call failed,
and a return integer pointer to which I write the return data to.
>
> A trick: since you are using only least significant byte of the
> response, you can declare it as u8 *value (yeah, ret is not best name
> here I think).
Neat trick.
>
> So, something like
>
> ..._eval(..., u8 *value, ...)
> {
> int response;
>
> if (!..._evalf(..., &response, ...)) {
> ...
>
> > + if (!acpi_evalf(hkey_handle, ret, method, "dd", param)) {
> > + acpi_handle_err(hkey_handle, "%s: evaluate failed", method);
> > + return AE_ERROR;
> > + }
> > +
> > + if (*ret & METHOD_ERR) {
>
> if (response & ...) {
>
> > + acpi_handle_err(hkey_handle,
> > + "%s evaluated but flagged as error", method);
> > + return AE_ERROR;
> > + }
>
> *value = response;
>
> > +
> > + return AE_OK;
> > +}
>
> > +static int tpacpi_battery_get(int what, int battery, int *ret)
> > +{
> > + switch (what) {
> > +
> > + case THRESHOLD_START:
> > +
> > + if (tpacpi_battery_acpi_eval(GET_START, ret, battery))
> > + return -ENODEV;
> > +
>
> > + /* The value is in the low 8 bits of the response */
> > + *ret = *ret & 0xFF;
>
> So, this will gone with a trick above.
>
> > + return 0;
> > +
> > + case THRESHOLD_STOP:
> > +
> > + if (tpacpi_battery_acpi_eval(GET_STOP, ret, battery))
> > + return -ENODEV;
> > +
> > + /* Value is in lower 8 bits */
> > + *ret = *ret & 0xFF;
>
> Ditto.
>
> > +
> > + /*
> > + * On the stop value, if we return 0 that
> > + * does not make any sense. 0 means Default, which
> > + * means that charging stops at 100%, so we return
> > + * that.
> > + */
> > + if (*ret == 0)
> > + *ret = 100;
> > +
> > + return 0;
> > +
> > + default:
> > + pr_crit("wrong parameter: %d", what);
> > + return -EINVAL;
> > + }
> > +
> > +}
>
> > +static int tpacpi_battery_probe(int battery)
> > +{
> > + int ret = 0;
> > +
> > + memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> > +
> > + /*
> > + * 1) Get the current start threshold
> > + * 2) Check for support
> > + * 3) Get the current stop threshold
> > + * 4) Check for support
> > + */
> > +
> > + if (acpi_has_method(hkey_handle, GET_START)) {
> > +
> > + if (tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
> > + pr_err("Error probing battery %d\n", battery);
> > + return -ENODEV;
> > + }
> > +
> > + /* Individual addressing is in bit 9 */
> > + if (ret & BIT(9))
> > + battery_info.individual_addressing = true;
> > +
> > + /* Support is marked in bit 8 */
> > + if (ret & BIT(8))
> > + battery_info.batteries[battery].start_support = 1;
> > + else
> > + return -ENODEV;
> > +
> > + if (tpacpi_battery_get(THRESHOLD_START, battery,
> > + &battery_info.batteries[battery].charge_start)) {
> > + pr_err("Error probing battery %d\n", battery);
> > + return -ENODEV;
> > + }
>
> > +
>
> Please, get rid of all useless empty lines like this one, or above (in
> between of two if:s).
>
> > + }
>
> > +/* General helper functions */
> > +
> > +static int tpacpi_battery_get_id(const char *battery_name)
> > +{
> > +
> > + if (strcmp(battery_name, "BAT0") == 0)
> > + return BAT_PRIMARY;
> > + if (strcmp(battery_name, "BAT1") == 0)
> > + return BAT_SECONDARY;
> > +
> > + /*
> > + * If for some reason the battery is not BAT0 nor is it
> > + * BAT1, we will assume it's the default, first battery,
> > + * AKA primary.
> > + */
> > + pr_warn("unknown battery %s, assuming primary", battery_name);
> > + return BAT_PRIMARY;
> > +}
> > +
> > +/* sysfs interface */
> > +
> > +static ssize_t tpacpi_battery_store(int what,
> > + struct device *dev,
> > + const char *buf, size_t count)
> > +{
> > + struct power_supply *supply = to_power_supply(dev);
> > + unsigned long value;
> > + int battery, errno;
>
> > + errno = kstrtoul(buf, 10, &value);
>
> > +
>
> Dont' split lines like these where one returns some error code
> followed by conditional check.
>
> > + if (errno)
>
> > + return errno;
>
> Besides, errno is a bad name, please consider what is used elsewhere
> in this module (rc, ret, rval, ...?).
>
> --
> 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
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
