RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds and charging mode switch.

[Yuan, Perry]

> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Wednesday, July 29, 2020 3:32 PM
> To: Yuan, Perry
> Cc: Sebastian Reichel; Matthew Garrett; Pali Rohár; Darren Hart; Andy
> Shevchenko; Limonciello, Mario; Linux PM; Linux Kernel Mailing List; Platform
> Driver
> Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds
> and charging mode switch.
> 
> 
> [EXTERNAL EMAIL]
> 
> On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan <Perry.Yuan@xxxxxxxx> wrote:
> >
> > From: perry_yuan <perry_yuan@xxxxxxxx>
> 
> Fix your name, please. Or do you have it spelled in the official documents like
> above?
> 
> > The patch control battery charging thresholds when system is under
> > custom charging mode through smbios API and driver`s sys attributes.It
> > also set the percentage bounds for custom charge.
> > Start value must lie in the range [50, 95],End value must lie in the
> > range [55, 100],END must be at least (START + 5).
> >
> > The patch also add the battery charging modes switch support.User can
> > switch the battery charging mode through the new sysfs entry.
> 
> The commit message needs rewording. I would recommend reading [1] and [2].
> 
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> (section 2)
> [2]: https://chris.beams.io/posts/git-commit/
> 
> > Primary battery charging modes valid choices are:
> > ['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
> 
> This and documentation are different (at least in terms of case).
> 
> Looks like this is something that should be agreed upon on the format and gets
> supported by Power Supply framework in the first place,
> 
> >  Documentation/ABI/testing/sysfs-class-power |  23 ++
> 
> In any case it should be a separate patch. It has nothing to do with Dell, on
> contrary it's a certain device which relies on generic attributes.
> 
> ...
> 
> >  #include <linux/debugfs.h>
> >  #include <linux/seq_file.h>
> 
> >  #include <acpi/video.h>
> > +#include <acpi/battery.h>
> 
> Keep it ordered.
> 
> > +#include <linux/string.h>
> 
> And this is a generic one, should be in generic headers block.
> 
> >  #include "dell-rbtn.h"
> >  #include "dell-smbios.h"
> 
> ...
> 
> > +static int dell_battery_get(int *start, int *end) {
> > +       struct calling_interface_buffer buffer;
> > +       struct calling_interface_token *token;
> > +       int ret;
> > +
> 
> > +       if (start) {
> > +               token =
> dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
> > +               if (!token)
> > +                       return -ENODEV;
> > +               dell_fill_request(&buffer, token->location, 0, 0, 0);
> > +               ret = dell_send_request(&buffer,
> > +                                       CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > +               *start = buffer.output[1];
> > +       }
> 
> (1)
> 
> > +       if (end) {
> > +               token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
> > +               if (!token)
> > +                       return -ENODEV;
> > +               dell_fill_request(&buffer, token->location, 0, 0, 0);
> > +               ret = dell_send_request(&buffer,
> > +                                       CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > +               if (ret)
> > +                       return -EIO;
> > +               *end = buffer.output[1];
> > +       }
> 
> (2)
> 
> (1) and (2) look like twins. Care to provide a helper to simplify?
> 
> > +       return 0;
> > +}
> 
> ...
> 
> > +       ret = dell_send_request(&buffer,
> > +                               CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > +       if (ret)
> 
> > +               return -EIO;
> 
> Why shadowing error code?
> 
> ...
> 
> > +       ret = dell_send_request(&buffer,
> > +                               CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > +       if (ret)
> > +               return -EIO;
> 
> Ditto.
> 
> > +       return ret;
> 
> And here perhaps simple 'return dell_send_request();'?
> 
> ...
> 
> > +       switch (mode) {
> > +       case BAT_STANDARD_MODE:
> > +               token =
> > + dell_smbios_find_token(BAT_STANDARD_MODE_TOKEN);
> 
> > +               if (!token)
> > +                       return -ENODEV;
> 
> > +               break;
> > +       case BAT_EXPRESS_MODE:
> > +               token =
> > + dell_smbios_find_token(BAT_EXPRESS_MODE_TOKEN);
> 
> > +               if (!token)
> > +                       return -ENODEV;
> 
> > +               break;
> > +       case BAT_PRIMARILY_AC_MODE:
> > +               token =
> > + dell_smbios_find_token(BAT_PRIMARILY_AC_MODE_TOKEN);
> 
> > +               if (!token)
> > +                       return -ENODEV;
> 
> > +               break;
> > +       case BAT_CUSTOM_MODE:
> > +               token = dell_smbios_find_token(BAT_CUSTOM_MODE_TOKEN);
> 
> > +               if (!token)
> > +                       return -ENODEV;
> 
> > +               break;
> > +       case BAT_ADAPTIVE_MODE:
> > +               token =
> > + dell_smbios_find_token(BAT_ADAPTIVE_MODE_TOKEN);
> 
> > +               if (!token)
> > +                       return -ENODEV;
> 
> Five times the same lines. Please deduplicate them. One is enough.
> 
> > +               break;
> > +       default:
> 
> > +               pr_warn("unspported charging mode!\n");
> 
> When you have a device use dev_*() to print messages.
> 
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> > +       ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE,
> SELECT_TOKEN_STD);
> > +       if (ret)
> > +               return -EIO;
> > +
> > +       return ret;
> 
> return dell_send_request(...);
> 
> ...
> 
> > +       ret = dell_send_request(&buffer, CLASS_TOKEN_READ,
> SELECT_TOKEN_STD);
> > +       if (ret)
> > +               return -EIO;
> 
> Do not shadow error codes.
> 
> > +       if (ret == 0)
> 
> What is the point?
> 
> > +               *mode = buffer.output[1];
> 
> > +       return ret;
> 
> Any difference to return 0; ?
> 
> ...
> 
> > +static ssize_t charge_control_charging_mode_show(struct device *dev,
> > +               struct device_attribute *attr,
> > +               char *buf)
> > +{
> > +       enum battery_charging_mode mode;
> > +       char *s = buf;
> > +
> > +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > +               if (battery_state[mode]) {
> > +                       if (mode == bat_chg_current)
> > +                               s += sprintf(s, "[%s] ", battery_state[mode]);
> > +                       else
> > +                               s += sprintf(s, "%s ",
> > + battery_state[mode]);
> 
> You have to control buffer boundaries.
> 
> > +               }
> > +       }
> 
> > +       if (s != buf)
> 
> if (s == buf)
>   return 0;
> 
> > +               /* convert the last space to a newline */
> > +               *(s-1) = '\n';
> > +       return (s - buf);
> > +}
> 
> ...
> 
> > +static ssize_t charge_control_charging_mode_store(struct device *dev,
> > +               struct device_attribute *attr,
> > +               const char *buf, size_t size) {
> > +       int err;
> > +       enum battery_charging_mode mode;
> > +       char *p;
> > +       int len;
> > +       const char *label;
> 
> > +       p = memchr(buf, '\n', size);
> > +       len = p ? p - buf : size;
> > +
> > +       for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > +               label = battery_state[mode];
> > +               if (label && len == strlen(label) &&
> > +                       !strncmp(buf, label, len)) {
> > +                       bat_chg_current = mode;
> > +                       break;
> > +               }
> > +       }
> 
> sysfs_match_string()
> 
> > +       if (mode > BAT_NONE_MODE && mode < BAT_MAX_MODE)
> > +               err = battery_charging_mode_set(mode);
> > +       else
> > +               err = -EINVAL;
> > +
> > +       return err ? err : size;
> > +}
> 
> ...
> 
> > +static ssize_t charge_control_thresholds_store(struct device *dev,
> > +               struct device_attribute *attr,
> > +               const char *buf, size_t size) {
> > +       int err, start, end;
> 
> > +       if (sscanf(buf, "%d %d", &start, &end) != 2)
> > +               return -EINVAL;
> 
> It's bad. The rule of thumb is one node per one property.
> On top of that sscanf() is not good, it doesn't check for overflow, use one of
> kstrto*().
> 
> > +       err = dell_battery_set(start, end);
> > +       if (err)
> > +               return err;
> > +
> > +       return size;
> > +}
> 
> ...
> 
> > +static int dell_battery_add(struct power_supply *battery) {
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_start_threshold);
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_end_threshold);
> > +       device_create_file(&battery->dev,
> > +               &dev_attr_charge_control_charging_mode);
> 
> Can it be an attribute group?
> 
> > +}
> 
> ...
> 
> > +static struct acpi_battery_hook dell_battery_hook = {
> > +       .add_battery = dell_battery_add,
> > +       .remove_battery = dell_battery_remove,
> > +       .name = "Dell Battery Extension"
> 
> Missed comma.
> 
> > +};
> 
> ...
> 
> > +static void dell_battery_setup(struct device *dev) {
> > +       enum battery_charging_mode current_mode = BAT_NONE_MODE;
> > +
> > +       battery_charging_mode_get(&current_mode);
> > +       if (current_mode) {
> > +               bat_chg_current = current_mode;
> 
> > +               pr_debug("battery is present\n");
> > +       } else {
> > +               pr_debug("battery is not present\n");
> > +       }
> 
> Why not dev_dbg()? Why do we have these messages at all?
> 
> > +       battery_hook_register(&dell_battery_hook);
> 
> > +       device_create_file(dev, &dev_attr_charge_control_thresholds);
> 
> Hmm...
> 
> > +}
> > +
> > +static void dell_battery_exit(struct device *dev) {
> > +       if (bat_chg_current != BAT_NONE_MODE) {
> > +               battery_hook_unregister(&dell_battery_hook);
> > +               device_remove_file(dev, &dev_attr_charge_control_thresholds);
> > +       }
> > +}
> 
> ...
> 
> > +/*Battery Charging Modes */
> 
> Indentation issues.
> Same for many other comments in this change.
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Andy .
Thanks a lot for your review, I am working on the V2 to drive your feedback.