Received-SPF: pass (google.com: domain of linux-kernel+bounces-170730-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: Tue, 7 May 2024 08:11:04 +0200
From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To: Dustin Howett <dustin@howett.net>
Cc: "Limonciello, Mario" <mario.limonciello@amd.com>, 
	Lee Jones <lee@kernel.org>, Benson Leung <bleung@chromium.org>, 
	Guenter Roeck <groeck@chromium.org>, Tzung-Bi Shih <tzungbi@kernel.org>, linux-kernel@vger.kernel.org, 
	chrome-platform@lists.linux.dev, Sebastian Reichel <sre@kernel.org>, linux-pm@vger.kernel.org
Subject: Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver
Message-ID: <99ae4866-a8cd-408c-8227-006f96f14dc7@t-8ch.de>
References: <20240505-cros_ec-framework-v1-0-402662d6276b@weissschuh.net>
 <613369f9-42c5-4a59-b83f-45bd1773ffe4@t-8ch.de>
 <a7ae8fc0-5e53-487a-86c6-f49dc6623688@amd.com>
 <e716716e-87fe-46f5-8ea9-5f649f1da11b@t-8ch.de>
 <CA+BfgNJzazn55wUMzjX=thqZGYz0LYU4cnO1Pn8U80B5FrSvxQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+BfgNJzazn55wUMzjX=thqZGYz0LYU4cnO1Pn8U80B5FrSvxQ@mail.gmail.com>

Hi Dustin,

On 2024-05-06 13:29:32+0000, Dustin Howett wrote:
> On Mon, May 6, 2024 at 12:43 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > On 2024-05-06 08:09:07+0000, Limonciello, Mario wrote:
> > > On 5/6/2024 1:09 AM, Thomas Weißschuh wrote:
> > > > On 2024-05-05 22:56:33+0000, Thomas Weißschuh wrote:
> > > > > Framework Laptops are using embedded controller firmware based on the
> > > > > ChromeOS EC project.
> > > > > In addition to the standard upstream commands some vendor-specific
> > > > > commands are implemented.
> > > > >
> > > > > Add a driver that implements battery charge thresholds using these
> > > > > custom commands.
> > > >
> > > > It turns out that standard ChromesOS EC defines EC_CMD_CHARGE_CONTROL.
> > > > The kernel headers however only define v1 of the protocol, which is very
> > > > limited.
> > > >
> > > > But in the upstream firmware repo there is a v3 which is much better.
> > > >
> > > > The Framework laptop only implements v2 which is also fine.
> > > > Given that v3 was only introduced late last year, it seems better to
> > > > stick to v2 anyways for now.
> > > >
> > > > So please disregard Patch 2, I'll see on how to use this via a normal
> > > > cros_ec driver.
> > > >
> > > > There are some other Framework-only features that will use Patch 1,
> > > > so feedback for that would still be good.
> > >
> > > What other kinds of features do you have in mind?
> >
> 
> Definitely privacy switch reporting belongs in a driver like this.

If it can't be done via one of the upstream CrOS EC commands, surely.

> Overall, I'm not sure about making it a subjugate driver under the
> cros_ec_mfd virtual "bus"... even though a lot of the features take a
> dependency on cros_ec.
> Doing so centralizes the work in the platform-chrome tree and may
> serve as a guidepost for any future laptop OEMs that derive their
> embedded controller firmware from ChromeOS's.
> If the owners of this tree sign off on that, that's awesome! I'd be
> concerned about making it all their responsibility.

Yes, some guidance from the maintainers will be great.

> I may be a bit biased, as I have been working on a driver of my own[1]
> for this purpose. It currently supports battery charge limiting[3],
> reporting fan speed via hwmon, the keyboard backlight[2], and has an
> open pull request that exposes the status of the privacy switches.

I have taken a look at that driver but wasn't fond of the fact that it
is not using cros_ec mfd. Taking a reference on a completely different
device looks iffy to me and in violation of the device hierarchy.

FYI I have completely non-Framework-specific implementations for
keyboard backlight [0], charge limiting [1] and hwmon [2].
(I didn't look at the privacy switches yet, maybe there is a generic
solution)
(I'm currently polishing [1] and [2], any feedback already would also be
much appreciated)

All of them work correctly on my Framework 13 AMD, Firmware 3.05.
These standard APIs are more powerful than the Framework-only ones.

Charge control can do start_threshold, stop_threshold and
charge_behaviour. Hwmon can do fans and temperature sensors.

Keyboard backlight just reuses the existing mainline driver.

> It is destined--once I find the time to clean it up--for
> drivers\platforms\x86 instead of ...\chrome.
> 
> This may be a good place for us to combine our efforts!

Surely!

Personally I only have the AMD 13 device (Azalea),
so I can't test anything else.
And I'd like to focus on the mainline-compatible APIs (first).

Feel free to contact me (privately?) if you have any suggestions.

> [1] https://github.com/DHowett/framework-laptop-kmod
> [2] I found that the Azalea did not report its keyboard backlight
> values through the standard cros ec KBLIGHT interface like hx20/30
> did, so the driver as it stands implements a fallback that uses the
> raw PWM state. I'm sure that you'd've noticed this if it was still
> true... so I am always happy to drop an unnecessary workaround. :)

For me the posted driver under [0] works as expected.

> [3] Which I believe still requires a special host command and is not
> integrated into the charge manager, at least as of Azalea/Lotus and
> _definitely_ not as of hx20/30!

This also works for me correctly with [1].
Do you know if there are plans by Framework to move the older devices to
a newer firmware?
This would also make their own maintenance work easier in the future,
especially considering their commitment to software longevity[3].

> [..]

Thomas

[0] https://lore.kernel.org/lkml/20240505-cros_ec-kbd-led-framework-v1-1-bfcca69013d2@weissschuh.net/
[1] https://git.sr.ht/~t-8ch/linux/tree/b4/cros_ec-charge-control
[2] https://git.sr.ht/~t-8ch/linux/tree/b4/cros_ec-hwmon
[3] https://frame.work/de/en/blog/enabling-software-longevity