Received-SPF: pass (google.com: domain of linux-kernel+bounces-54098-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: Tue, 6 Feb 2024 10:07:09 +1030
From: Jonathan Woithe <jwoithe@just42.net>
To: Armin Wolf <W_Armin@gmx.de>
Cc: Szilard Fabian <szfabian@bluemarch.art>, hdegoede@redhat.com,
        ilpo.jarvinen@linux.intel.com, 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: <ZcFxJf6YqjMAAOou@marvin.atrad.com.au>
References: <20240129163502.161409-2-szfabian@bluemarch.art>
 <20240129175714.164326-2-szfabian@bluemarch.art>
 <fabf391c-933c-4a7b-a23c-d361ad3d7cc0@gmx.de>
 <Zb2GMCSIz1MuWpQZ@N>
 <df011292-48cd-4fbb-856c-20a3db9f99e8@gmx.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <df011292-48cd-4fbb-856c-20a3db9f99e8@gmx.de>

On Mon, Feb 05, 2024 at 06:07:46PM +0100, Armin Wolf wrote:
> 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.

I'm happy to see this work proceed.  Once a revised patch is available I'll
test it on my S7020.  This should exercise the error recovery code because
the functionality being addressed here almost certainly doesn't exist in a
laptop as old as the S7020.  Yes, my S7020 is still operational and in use.

Regards
  jonathan