Received-SPF: pass (google.com: domain of linux-kernel+bounces-53065-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <df011292-48cd-4fbb-856c-20a3db9f99e8@gmx.de>
Date: Mon, 5 Feb 2024 18:07:46 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge
 control support
Content-Language: en-US
To: Szilard Fabian <szfabian@bluemarch.art>
Cc: hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com, jwoithe@just42.net,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
References: <20240129163502.161409-2-szfabian@bluemarch.art>
 <20240129175714.164326-2-szfabian@bluemarch.art>
 <fabf391c-933c-4a7b-a23c-d361ad3d7cc0@gmx.de> <Zb2GMCSIz1MuWpQZ@N>
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <Zb2GMCSIz1MuWpQZ@N>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
UI-OutboundReport: notjunk:1;M01:P0:FywLzaAvvJs=;vhBNhZOl0/1zIX+ZbwQKLzw2SwO
 l6nqcTQInmbCUF3ba1JWmvRYn+j7n4xYLwZg1JR/w94Uu6Xit+vJ+SylGrdrIcpv2tJXQZcGf
 ug4FYg03KWiwuepndu8PjqcG6AQ+UQZx5e7yaEmcVuDceGCAAZzTScWA532LAYAAnTEHZVvzC
 cRqZ7Vsm+INQowLQKjWqsVd8/XBveWMzIg76IWt2U60iW3QW9hCtdFOVO146TdHbGbBXoy/gn
 xVeqNVQcHAtcbmPaPByyEouHL6LuTEcNZgdqVvtJRT5fXvUx3hL/mf+E2lqbJ1PPQE5u15xZo
 qS8Ijuhc0lM6YNaNU0SCIGLLflb7FgVs//Q1BXIf9VC626dECJD1eTxhv64uQYUsybZJ2TnSu
 3QOTNMD5iv7a8DH/YfUcOgvnKor/+iT5iuHwS0oe+pnMAAPFF9JX+fH+YPoh9zdWINcHDYzva
 ejGnSYToJPERYm6SM1b10hroPuI2m+v+yGGOFXCPqaOL8xBiusoD1vvFAxwe/cJJGw16Y5z/1
 auI7nNCxx+vGY9NUrA3h+z3vtG4cFxOwTgEfLHEuiSsVQhVTA5HCvnoJ0MR9NCG5DwAfzMp4Z
 /fF4Ur+42ep6gGPmqi7UabPjEt5euiNP9W+mHe/hOjOeLNZpi210KSSsrhdqhA8IO8dFtW5Z+
 UnCrfUY5A7BD3D+sON0d1LzBLBuiCDCo9/nvO5HcWvPSI5EjdBpg+ebqoB/Ym5CFljtooxHkm
 ch4X6pLq7qw/7dsZwReVF2XNWBLjG2/ATzt0uD9QdOGHDWvhTFxgJFAx0NENfPWuz432k7as+
 FW1XNMUndEGeNP90l/4LYum+Bi84tFyQhSQH/QOIyCGqg=

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