Re: [PATCH] mfd: cros: Update EC protocol to match current EC code

[Guenter Roeck]

On Wed, Mar 6, 2019 at 10:27 AM Enric Balletbo Serra
<eballetbo@xxxxxxxxx> wrote:
>
> Hi Guenter,
>
> Missatge de Guenter Roeck <groeck@xxxxxxxxxx> del dia dc., 6 de marÃ
> 2019 a les 18:20:
> >
> > [resending in plain text mode ]
> >
> > On Wed, Mar 6, 2019 at 8:57 AM Enric Balletbo Serra <eballetbo@xxxxxxxxx> wrote:
> > >
> > > Hi Gwendal,
> > >
> > > Many thanks to send this upstream.
> > >
> > > Missatge de Gwendal Grignou <gwendal@xxxxxxxxxxxx> del dia dj., 28 de
> > > febr. 2019 a les 1:31:
> > > >
> > > > Chromebook Embedded Controller protocol is defined in the kernel at
> > > > cros_ec_commands.h.
> > > > The source of trust for the EC protocol is at
> > > > https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h
> > > >
> > > > Only needed changes have been picked up from this file to the kernel
> > > > include file leading to gaps between the upstream version and what the
> > > > latest ECs can do.
> > > >
> > > > Fill the gaps to ease future integrations. Changes from the original
> > > > files is header/footer for license and include files for alignment.
> > > >
> > > > Check this include file works on ChomeOS kernel 4.14 and 4.19 on eve.
> > > >
> > > > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > > > ---
> > > >  include/linux/mfd/cros_ec_commands.h | 3627 +++++++++++++++++++++-----
> > >
> > > I'm wondering if we should move this file to include/uapi at some
> > > point as this file is also used as user-space API for some userspace
> > > applications.
> > >
> > > While we are here I'd suggest if we can also fix the few errors (3)
> > > and warnings (5) spotted by checkpatch. With that it's an ack from my
> > > side.
> > >
> > > Being strict, though, on most cases the variables are going to be used
> > > in code that can be seen by user-space programs so maybe we should
> > > really need to switch to __u8/__u16/etc exportable data types instead
> > > of the uint8_t/uint16_t/etc types (those are not aimed to be used
> > > within the kernel). For those types that are internal we should use
> > > in-kernel type (u8/u16/etc)
> > >
> > > There is also the use of the BIT macro instead of the (1 << x), I know
> > > that this is a maintainer preference.
> > >
> > Is all that even possible ?
>
> Sorry, if I wasn't clear from my side, I should have marked those as
> nit, as in this case, I don't really mind, but I remember Lee
> requesting some of those changes for this file.
>
> What I want to avoid is the desynchronization again. As this file
> belongs to the MFD subsystem Lee needs to be happy with it or at least
> know that we're not following the kernel style because is an imported
> file (i.e not using the BIT macro, doing this example because I know
> he takes care of this).
>
> So I think that the main question here is if an imported file like
> this is acceptable in the include/linux/mfd?
>

I find it highly (and more and more) confusing about what is
acceptable in the kernel and where, and what isn't. There are imported
files all over the place, and they seem to be widely acceptable and
accepted. It is difficult to keep track of what is acceptable and what
isn't on a per subsystem basis. I'll step back and keep my confusion
to myself.

> > After all, this is an imported file, and
> > we don't usually expect that imported files meet the Linux kernel
> > coding style.
> >
>
> I remember seeing somewhere that the EC code was following the Linux
> kernel style? Now I am not able to find where.
>

You are correct: https://www.chromium.org/chromium-os/ec-development

Running checkpatch over the code, it doesn't look like that is
followed, though, much less enforced.

Guenter

> Thanks,
>  Enric
>
> > Thanks,
> > Guenter
> >
> > > [snip]
> > >
> > > Thanks,
> > >  Enric