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

[Enric Balletbo Serra]

Hi Lee,
Missatge de Guenter Roeck <groeck@xxxxxxxxxx> del dia dc., 6 de marÃ
2019 a les 19:58:
>
> 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 +++++++++++++++++++++-----
> > > >

Just for if I was not clear after the discussion this is an ack from my side

Acked-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>

> > > > 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.
>

In theory, I should be cc'ied if someone sends a modification to this
file, also Guenter and Benson. So we can take care that is synced with
the EC code and doesn't diverge again. I think that this is the main
purpose.

Thanks,
 Enric

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