Re: [PATCH 1/2] platform/chrome: cros_ec_framework_laptop: introduce driver

["Thomas Weißschuh" <linux@weissschuh.net>]

On 2024-05-06 08:10:58+0000, Limonciello, Mario wrote:
> 
> 
> On 5/5/2024 3:56 PM, 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
> > ones are implemented.
> > 
> > Add a driver for those custom EC commands.
> > 
> > At first, provide an empty driver that only takes care of scaffolding and
> > device binding.
> > Further patches will add functionality to the driver.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   MAINTAINERS                                        |  5 ++
> >   drivers/mfd/cros_ec_dev.c                          | 13 ++++++
> >   drivers/platform/chrome/Kconfig                    | 11 +++++
> >   drivers/platform/chrome/Makefile                   |  1 +
> >   drivers/platform/chrome/cros_ec_framework_laptop.c | 53 ++++++++++++++++++++++
> >   5 files changed, 83 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c23fda1aa1f0..60699c289757 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4988,6 +4988,11 @@ S:	Maintained
> >   F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
> >   F:	sound/soc/codecs/cros_ec_codec.*
> > +CHROMEOS EC FRAMEWORK LAPTOP EXTENSIONS
> > +M:	Thomas Weißschuh <linux@weissschuh.net>
> > +S:	Maintained
> > +F:	drivers/platform/chrome/cros_ec_framework_laptop.c
> > +
> >   CHROMEOS EC SUBDRIVERS
> >   M:	Benson Leung <bleung@chromium.org>
> >   R:	Guenter Roeck <groeck@chromium.org>
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index a52d59cc2b1e..0a36e77e5039 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -145,6 +145,10 @@ static const struct mfd_cell cros_ec_vbc_cells[] = {
> >   	{ .name = "cros-ec-vbc", }
> >   };
> > +static const struct mfd_cell cros_ec_framework_cells[] = {
> > +	{ .name = "cros-ec-framework", }
> > +};
> > +
> >   static void cros_ec_class_release(struct device *dev)
> >   {
> >   	kfree(to_cros_ec_dev(dev));
> > @@ -299,6 +303,15 @@ static int ec_device_probe(struct platform_device *pdev)
> >   				 retval);
> >   	}
> > +	 /* The EC on Framework laptops implements some nonstandard features */
> 
> I don't think there is a spec really for cros_ec is there?  I think it will
> depend upon what features you're talking about if this is the right way to
> go.

I equate "standard" with "mainline".

> The reason I say this is that maybe some of the same kinds of features will
> make sense for chromebooks that use cros_ec in the future and thus they
> should be "generic" cros_ec mfd cells to probe for in some way.

The commands identifiers are just numbers.
So if multiple vendors use the same numbers there could be collisions.
By loading the subdriver only for the correct vendors devices we avoid those.

If other vendors or mainline CrOS EC implements the same commands we can
revisit this and move the functionality to a generic mfd cell.

But after the discussions on the cover letter, let's first see if a
vendor-specific driver is needed at all.

Thomas