Re: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings

[Pali Rohár]

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;
> +}