Received-SPF: pass (google.com: domain of linux-kernel+bounces-50766-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sat, 03 Feb 2024 00:17:58 +0000
To: Armin Wolf <W_Armin@gmx.de>
From: 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
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support
Message-ID: <Zb2GMCSIz1MuWpQZ@N>
In-Reply-To: <fabf391c-933c-4a7b-a23c-d361ad3d7cc0@gmx.de>
References: <20240129163502.161409-2-szfabian@bluemarch.art> <20240129175714.164326-2-szfabian@bluemarch.art> <fabf391c-933c-4a7b-a23c-d361ad3d7cc0@gmx.de>
Feedback-ID: 87830438:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello,

On Tue, Jan 30, 2024 at 03:02:09AM +0100, Armin Wolf wrote:
> Am 29.01.24 um 19:00 schrieb Szilard Fabian:
> > +
> > +=09return 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,
> > +=09=09=09       struct acpi_battery_hook *hook)
> > +{
> > +=09/* Check if there is an existing FUJ02E3 ACPI device. */
> > +=09if (fext =3D=3D NULL)
> > +=09=09return -ENODEV;
>=20
> 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.
>=20
> This would guarantee that the battery hook always accesses the correct AC=
PI device
> and you could drop this check.
>=20
> > +
> > +=09/*
> > +=09 * Check if the S006 0x21 method exists by trying to get the curren=
t
> > +=09 * battery charge limit.
> > +=09 */
> > +=09int s006_cc_return;
> > +=09s006_cc_return =3D call_fext_func(fext, FUNC_S006_METHOD,
> > +=09=09=09=09=09CHARGE_CONTROL_RW, 0x21, 0x0);
> > +=09if (s006_cc_return =3D=3D UNSUPPORTED_CMD)
> > +=09=09return -ENODEV;
>=20
> 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 'exi=
t
  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 wit=
h
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