Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Am 03.02.24 um 01:17 schrieb Szilard Fabian:


On Tue, Jan 30, 2024 at 03:02:09AM +0100, Armin Wolf wrote:
Am 29.01.24 um 19:00 schrieb Szilard Fabian:
+	return sprintf(buf, "%d\n", status);
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+/* ACPI battery hook */
+static int fujitsu_battery_add(struct power_supply *battery,
+			       struct acpi_battery_hook *hook)
+	/* Check if there is an existing FUJ02E3 ACPI device. */
+	if (fext == NULL)
+		return -ENODEV;
Can you put the struct acpi_battery_hook into the struct fujitsu_laptop
and then use container_of() to retrieve the ACPI device from there?
The dell-wmi-ddv driver does something similar.

This would guarantee that the battery hook always accesses the correct ACPI device
and you could drop this check.

+	/*
+	 * Check if the S006 0x21 method exists by trying to get the current
+	 * battery charge limit.
+	 */
+	int s006_cc_return;
+	s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD,
+					CHARGE_CONTROL_RW, 0x21, 0x0);
+	if (s006_cc_return == UNSUPPORTED_CMD)
+		return -ENODEV;
Maybe this check should be done once during probe?
What about the following scenario?
- Put a bool into the struct fujitsu_laptop to store information about the
   machine's charge control ability.
- The S006 0x21 method check with `battery_hook_register` gets moved into
   an 'init function'. In that 'init function' the bool gets set accordingly.
- `battery_hook_unregister` gets moved into an 'exit function', where the
   bool gets read and when it's false nothing happens.
- `fext` check gets removed from `fujitsu_battery_add` because it's
   redundant (more about that later).
- The 'init function' gets called in `acpi_fujitsu_laptop_add` and the 'exit
   function' gets called in `acpi_fujitsu_laptop_remove`.

With that scenario the code could be a little bit clearer in my opinion.
And it is possible to drop the `fext` check because if the FUJ02E3 ACPI
device exists `fext` gets set in the `acpi_fujitsu_laptop_add` function with
an error check.
(And the `fujitsu_battery_add` `fext` check was already redundant because
`battery_hook_register` got called in `acpi_fujitsu_laptop_add`. `fext`
gets set in the same function, and there is an error check already.)


This would work too.

Armin Wolf

[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux