Received-SPF: pass (google.com: domain of linux-kernel+bounces-170287-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <e716716e-87fe-46f5-8ea9-5f649f1da11b@t-8ch.de>
From: Dustin Howett <dustin@howett.net>
Date: Mon, 6 May 2024 13:29:32 -0500
Message-ID: <CA+BfgNJzazn55wUMzjX=thqZGYz0LYU4cnO1Pn8U80B5FrSvxQ@mail.gmail.com>
Subject: Re: [PATCH 0/2] platform/chrome: cros_ec_framework_laptop: new driver
To: =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 6, 2024 at 12:43=E2=80=AFPM Thomas Wei=C3=9Fschuh <linux@weisss=
chuh.net> wrote:
>
> On 2024-05-06 08:09:07+0000, Limonciello, Mario wrote:
> >
> >
> > On 5/6/2024 1:09 AM, Thomas Wei=C3=9Fschuh wrote:
> > > On 2024-05-05 22:56:33+0000, Thomas Wei=C3=9Fschuh wrote:
> > > > Framework Laptops are using embedded controller firmware based on t=
he
> > > > 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 v=
ery
> > > 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.

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.

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.

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!
d

[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. :)
[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!

> [...]
>
> > [..]
>
> Thomas