Messages in this thread | | | Date | Mon, 5 Feb 2024 18:07:46 +0100 | Subject | Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support | From | Armin Wolf <> |
| |
Am 03.02.24 um 01:17 schrieb Szilard Fabian:
> Hello, > > 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.) > > Thanks, > Szilard > This would work too.
Armin Wolf
| |