Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources

[Paul Bolle <pebolle@tiscali.nl>]

Doug,

On Fri, 2014-06-13 at 08:22 -0700, Doug Anderson wrote:
> On Fri, Jun 13, 2014 at 1:08 AM, Paul Bolle <pebolle@tiscali.nl> wrote:
> > On Wed, 2014-06-11 at 08:11 -0700, Doug Anderson wrote:
> >> This is a config option on the ChromeOS EC
> >> <https://chromium.googlesource.com/chromiumos/platform/ec>.  Doing a
> >> grep there:
> >>
> >> board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> driver/battery/samus.c:#endif   /* CONFIG_CHARGER_PROFILE_OVERRIDE */
> >> include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE
> >> include/ec_commands.h:  /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */
> >> test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
> >
> > I see. So this is not a Kconfig macro but a general macro with a CONFIG_
> > prefix. There are quite a bit of those in the tree already, but still,
> > would another prefix also do?
> 
> Given that it's an entirely separate project and this is a valid
> CONFIG option in that project, it seems a lot to ask them not to use
> the CONFIG_ prefix.  Also: the part you are objecting to is only a
> comment, right?

So all the hits you quoted above are actually from code that's never
going to be included in the kernel tree, right? If so, then yes, we're
only discussing a single comment.

> We could certainly add extra wording in the comment to make it obvious
> that this is a CONFIG option for the EC and not the kernel.  Would
> that be enough?  ...or are you trying to use some scripts to
> automatically process files to look for CONFIG options?

Yes, I'm using a script to check for Kconfig macros, among other things.
It doesn't care about comments (because every now and then mistakes are
made in comments too, and some of those can get surprisingly confusing).

Anyhow, the CONFIG_ prefix used in the kernel tree is quite generic, but
we're stuck with it. Would it be bothersome to drop it in that comment?
Mentioning a preprocessor macro from a separate project is a bit
confusing to begin with. How is one supposed to know that this is a
reference to something out of tree?

So, in summary, while we're apparently only discussing a single comment,
I would appreciate it if it could be reworded, preferably by dropping
that the CONFIG_ prefix. But other people might care very little, as
they don't share this particular pet peeve.

Thanks,


Paul Bolle