Received-SPF: pass (google.com: domain of linux-kernel+bounces-174348-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 9 May 2024 17:28:58 +0800
From: Tzung-Bi Shih <tzungbi@kernel.org>
To: Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>
Cc: Mario Limonciello <mario.limonciello@amd.com>,
	Lee Jones <lee@kernel.org>, Benson Leung <bleung@chromium.org>,
	Guenter Roeck <groeck@chromium.org>,
	chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org,
	Dustin Howett <dustin@howett.net>
Subject: Re: [PATCH] platform/chrome: cros_kbd_led_backlight: enable probing
 through EC_FEATURE_PWM_KEYB
Message-ID: <ZjyXWrYIjE47--Hl@google.com>
References: <20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2@weissschuh.net>
 <cd9c5b6a-0155-434a-b868-a9ea52e878c9@amd.com>
 <ae29f036-5e39-47ee-98d3-c023a263a3ef@t-8ch.de>
 <ZjxQHV9FPovvm_CY@google.com>
 <9b594929-a827-4682-b4aa-98a2c85a536b@t-8ch.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9b594929-a827-4682-b4aa-98a2c85a536b@t-8ch.de>

On Thu, May 09, 2024 at 10:13:37AM +0200, Thomas Wei?schuh wrote:
> On 2024-05-09 12:25:01+0000, Tzung-Bi Shih wrote:
> > On Mon, May 06, 2024 at 07:38:09PM +0200, Thomas Wei?schuh wrote:
> > > On 2024-05-05 08:42:21+0000, Mario Limonciello wrote:
> > > > On 5/5/2024 04:41, Thomas Wei?schuh wrote:
> > > > > The ChromeOS EC used in Framework laptops supports the standard cros
> > > > > keyboard backlight protocol.
> > > > > However the firmware on these laptops don't implement the ACPI ID
> > > > > GOOG0002 that is recognized by cros_kbd_led_backlight and they also
> > > > > don't use device tree.
> > 
> > If implementing ACPI ID GOOG0002 is not an option, how about adding a new ACPI
> > ID?  For the new ACPI ID, it can use EC PWM for setting the brightness.
> 
> Adding a new ACPI ID would be easier than a full-blown ACPI interface.
> This would still need changes to the drivers probing setup, however.
> 
> What are the advantages of the ACPI ID aproach over EC_FEATURE_PWM_KEYB?
> The EC feature also automatically works on device-tree platforms and
> without any work from system vendors.

Perhaps no advantages but just following its original design.  The driver uses
ACPI table for matching devices since it appears.  We shouldn't remove the
ACPI matching anyway as some existing devices may rely on it.

In addition, adding a new ACPI ID sounds more reasonable than using
keyboard_led_is_mfd_device() to me.

> Adding ACPI ID only for signalling without using ACPI for
> communication on the other hand seems weird.

I have a different view: using a new ACPI ID and another driver data fits
current framework better.  I'm not sure if the reason is strong enough for
applying a new ACPI ID though.

We could wait to see if others in the mailing list may have more inputs.

> > > > Something I'd wonder is if the GOOG0002 ACPI ID can go away entirely with
> > > > this type of change.  Presumably the Chromebooks with ChromeOS EC /also/
> > > > advertise EC_FEATURE_PWM_KEYB.
> > > 
> > > Sounds good to me in general. It would make the code cleaner.
> > > 
> > > But I have no idea how CrOS kernels are set up in general.
> > > If they are not using CONFIG_MFD_CROS_EC_DEV for some reason that
> > > wouldn't work.
> > > 
> > > If the CrOS folks agree with that aproach I'll be happy to implement it.
> > 
> > I would say NO as some existing devices (with legacy firmware and kernel) may
> > rely on it.
> 
> Ack, makes sense.
> 
> You mention legacy kernels, but these would not be affected.

We never know if a device would run legacy firmware with new kernel in some
day.