Hello Andres,
I have just comments for this change. It looks good. After resolving
them you can add my tag: Reviewed-by: Pali Rohár <pali@xxxxxxxxxx>
On Thursday 15 August 2024 19:28:48 Andres Salomon wrote:
> The Dell BIOS allows you to set custom charging modes, which is useful
> in particular for extending battery life. This adds support for tweaking
> those various settings from Linux via sysfs knobs. One might, for
> example, have their laptop plugged into power at their desk the vast
> majority of the time and choose fairly aggressive battery-saving
> settings (eg, only charging once the battery drops below 50% and only
> charging up to 80%). When leaving for a trip, it would be more useful
> to instead switch to a standard charging mode (top off at 100%, charge
> any time power is available). Rebooting into the BIOS to change the
> charging mode settings is a hassle.
>
> For the Custom charging type mode, we reuse the common
> charge_control_{start,end}_threshold sysfs power_supply entries. The
> BIOS also has a bunch of other charging modes (with varying levels of
> usefulness), so this also adds a 'charge_type' sysfs entry that maps the
> standard values to Dell-specific ones and documents those mappings in
> sysfs-class-power-dell.
>
> This work is based on a patch by Perry Yuan <perry_yuan@xxxxxxxx> and
> Limonciello Mario <Mario_Limonciello@xxxxxxxx> submitted back in 2020:
I think that the information below is not needed to have in commit
message. We also do not include links or information about previous
version of patch sent by you.
> https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@xxxxxxxx/
> Both of their email addresses bounce, so I'm assuming they're no longer
> with the company. I've reworked most of the patch to make it smaller and
> cleaner.
...
> +static ssize_t charge_type_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + bool matched = false;
> + int err, i;
> +
> + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> + if (!(battery_supported_modes & BIT(i)))
> + continue;
> +
> + if (sysfs_streq(battery_modes[i].label, buf)) {
> + matched = true;
> + break;
> + }
> + }
> + if (!matched || !(battery_supported_modes & BIT(i)))
> + return -EINVAL;
Check for "!(battery_supported_modes & BIT(i))" is redundant here.
"matched" can be true only in case if the i-th mode is supported and was
specified in buffer.
> +
> + err = dell_battery_set_mode(battery_modes[i].token);
> + if (err)
> + return err;
> +
> + return size;
> +}
