Re: [PATCH 2/3 v8] thinkpad_acpi: Add support for battery thresholds

[Ognjen Galić <smclt30p@xxxxxxxxx>]

On 21 Dec 2017 2:53 pm, "Andy Shevchenko" <andy.shevchenko@xxxxxxxxx> wrote:

On Thu, Dec 21, 2017 at 12:00 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"

Sorry, didn't notice earlier some things commented below.

>  #include <acpi/video.h>
>
> +
>  /* ThinkPad CMOS commands */

Redundant change.

> +enum {
> +       /* Error condition bit */
> +       METHOD_ERR = (1 << 31),

bitops.h but no BIT() use?

BIT(31) ?

> +};

> +static int tpacpi_battery_set(int what, int battery, int value)
> +{

> +

Redundant.

> +       int param = 0x0, ret = 0xFFFFFFFF;

Useless assignment for param, not sure abour ret.

> +
> +       /* The first 8 bits are the value of the threshold */
> +       param = value;
> +       /* The battery ID is in bits 8-9, 2 bits */
> +       param |= battery << 8;
> +
> +       switch (what) {
> +
> +       case THRESHOLD_START:
> +
> +               if (tpacpi_battery_acpi_eval(SET_START, &ret, param)) {
> +                       pr_err("failed to set charge threshold on battery %d",
> +                                       battery);
> +                       return -ENODEV;
> +               }
> +
> +               return 0;
> +
> +       case THRESHOLD_STOP:
> +
> +               if (tpacpi_battery_acpi_eval(SET_STOP, &ret, param)) {
> +                       pr_err("failed to set charge stop threshold: %d", battery);
> +                       return -ENODEV;
> +               }
> +
> +               return 0;
> +
> +       default:
> +               pr_crit("wrong parameter: %d", what);
> +               return -EINVAL;
> +       }

> +

Redundant.

> +}

> +
> +static int tpacpi_battery_probe(int battery)
> +{
> +       int ret = 0;
> +

> +       /* Reset the struct */

Useless.

> +       memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));

> +}

> +static ssize_t tpacpi_battery_store(struct device *dev,
> +                                  struct device_attribute *devattr,
> +                                  const char *buf, size_t count)
> +{
> +       int attr, battery;
> +       unsigned long value;
> +       struct power_supply *supply = to_power_supply(dev);

Can you use reverse xmas tree, esp. put assignments first.

> +       if (!supply) {

How this could be possible?!

> +               pr_err("Can't upcast to power_supply!");
> +               return -ENODEV;
> +       }

> +       if (kstrtoul(buf, 10, &value))
> +               return -EINVAL;

Don't shadow an error code from kstrtoul().

> +static ssize_t tpacpi_battery_show(struct device *dev,
> +                                 struct device_attribute *devattr,
> +                                 char *buf)
> +{
> +       int ret = 0x0, attr, battery;
> +       struct power_supply *supply = to_power_supply(dev);

> +       if (!supply) {

How this could be possible?!

> +               pr_crit("Can't upcast to power_supply!");
> +               return -ENODEV;
> +       }

> +       return sprintf(buf, "%d\n", ret);

> +}
> +
> +DEVICE_ATTR(charge_start_threshold, 0644,
> +               tpacpi_battery_show, tpacpi_battery_store);
> +DEVICE_ATTR(charge_stop_threshold, 0644,
> +       tpacpi_battery_show, tpacpi_battery_store);

DEVICE_ATTR_RW().

I did not use DEVICE_ATTR_RW() because I can't use a common store and show function for both attributes to minimize the code I need. If I used DEVICE_ATTR_RW() I would need to define 4 more functions that do the same. That seems redundant. 

> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,
>  extern void power_supply_unregister(struct power_supply *psy);
>  extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +#define to_power_supply(device) container_of(device, struct power_supply, dev)
> +
>  extern void *power_supply_get_drvdata(struct power_supply *psy);
>  /* For APM emulation, think legacy userspace. */
>  extern struct class *power_supply_class;

This one sounds to me as a separate change.

At the same time you may convert the current user of it to make sense
of the change.
drivers/power/supply/power_supply_core.c:671:

I think I pointed to this out once.

I wanted to minimize the changes in pm to avoid going through all the review process hassle for a few simple changes, because I'm modifying a third subsystem. But I will do it later tonight.

This is my first patchset and I did not expect for the review process to be this agonizingly slow, so I wanted to speed it up by not touching pm.

Thanks for the comments!

Ognjen

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