> 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(¤t_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.
