[PATCH v4] platform/chrome: cros_ec_trace: update generating script

[PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Tzung-Bi Shih]

The original script generates unneeded ", \" on the last line which
results in compile error if one would forget to remove them.  Update the
script to not generate ", \" on the last line.  Also add a define guard
for EC_CMDS.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
Changes from v1:
- simpler awk code
Changes from v2:
- use c style comments instead of c++ style
- use '@' delimiter in sed instead of '/' to avoid unintentional end of
  comment "*/"
Changes from v3:
- more detail commit message
- add define guard for EC_CMDS

 drivers/platform/chrome/cros_ec_trace.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_trace.c b/drivers/platform/chrome/cros_ec_trace.c
index 0a76412095a9..1412ae024435 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -5,8 +5,27 @@
 
 #define TRACE_SYMBOL(a) {a, #a}
 
-// Generate the list using the following script:
-// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' include/linux/mfd/cros_ec_commands.h
+/*
+ * Generate the list using the following script:
+ * sed -n 's@^#define \(EC_CMD_[[:alnum:]_]*\)\s.*@\tTRACE_SYMBOL(\1), \\@p' \
+ * include/linux/mfd/cros_ec_commands.h | awk '
+ * BEGIN {
+ *   print "#ifndef EC_CMDS";
+ *   print "#define EC_CMDS \\";
+ * }
+ * {
+ *   if (NR != 1)
+ *     print buf;
+ *   buf = $0;
+ * }
+ * END {
+ *   gsub(/, \\/, "", buf);
+ *   print buf;
+ *   print "#endif";
+ * }
+ * '
+ */
+#ifndef EC_CMDS
 #define EC_CMDS \
 	TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
 	TRACE_SYMBOL(EC_CMD_HELLO), \
@@ -119,6 +138,7 @@
 	TRACE_SYMBOL(EC_CMD_PD_CHARGE_PORT_OVERRIDE), \
 	TRACE_SYMBOL(EC_CMD_PD_GET_LOG_ENTRY), \
 	TRACE_SYMBOL(EC_CMD_USB_PD_MUX_INFO)
+#endif
 
 #define CREATE_TRACE_POINTS
 #include "cros_ec_trace.h"
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Tzung-Bi Shih]

Hi Enric,

I found it is error-prone to replace the EC_CMDS after updated.
Perhaps, we should introduce an intermediate file "cros_ec_trace.inc".
The generating script replaces whole ".inc" file every time and the
cros_ec_trace.c includes the "cros_ec_trace.inc".

If this proposal makes sense to you, I can send the patch after this
change landed for*-next.

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Enric Balletbo i Serra]

Hi Tzung-Bi

On 30/7/19 16:07, Tzung-Bi Shih wrote:
> Hi Enric,
> 
> I found it is error-prone to replace the EC_CMDS after updated.
> Perhaps, we should introduce an intermediate file "cros_ec_trace.inc".

I am not sure I get you here, a .inc? could you explain a bit more?

Thanks,
~ Enric

> The generating script replaces whole ".inc" file every time and the
> cros_ec_trace.c includes the "cros_ec_trace.inc".
> 
> If this proposal makes sense to you, I can send the patch after this
> change landed for*-next.
> 

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Tzung-Bi Shih]

On Thu, Aug 1, 2019 at 6:59 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Tzung-Bi
>
> On 30/7/19 16:07, Tzung-Bi Shih wrote:
> > Hi Enric,
> >
> > I found it is error-prone to replace the EC_CMDS after updated.
> > Perhaps, we should introduce an intermediate file "cros_ec_trace.inc".
>
> I am not sure I get you here, a .inc? could you explain a bit more?
>
Manually generate .inc for all EC host commands:
sed ... include/linux/mfd/cros_ec_commands.h | awk ...
>drivers/platform/chrome/cros_ec_trace.inc

In cros_ec_trace.c:
#include "cros_ec_trace.inc"

cros_ec_trace.inc:
#ifndef EC_CMDS
#define EC_CMDS \
    ...
#endif

Override the whole file instead of replacing part of file to prevent
cut-and-paste error.

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Enric Balletbo i Serra]

Hi Tzung-Bi,

On 1/8/19 15:04, Tzung-Bi Shih wrote:
> On Thu, Aug 1, 2019 at 6:59 PM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Tzung-Bi
>>
>> On 30/7/19 16:07, Tzung-Bi Shih wrote:
>>> Hi Enric,
>>>
>>> I found it is error-prone to replace the EC_CMDS after updated.
>>> Perhaps, we should introduce an intermediate file "cros_ec_trace.inc".
>>
>> I am not sure I get you here, a .inc? could you explain a bit more?
>>
> Manually generate .inc for all EC host commands:
> sed ... include/linux/mfd/cros_ec_commands.h | awk ...
>> drivers/platform/chrome/cros_ec_trace.inc
> 
> In cros_ec_trace.c:
> #include "cros_ec_trace.inc"
> 

Got it. I don't think this is a "kernel" way to do it. Also, I don't see a big
value on doing this.

> cros_ec_trace.inc:
> #ifndef EC_CMDS
> #define EC_CMDS \
>     ...
> #endif
> 
> Override the whole file instead of replacing part of file to prevent
> cut-and-paste error.
> 

The way I see all this is that bulk updates of that file "shouldn't be allowed"
or are not preferable. Ideally, _we_ (the maintainers), should take care on have
cros_ec_commands and cros_ec_trace sync. For that, when someone sends a patch
that touches the cros_ec_commands _we_ (them maintainers) should tell him to
update also the cros_ec_trace file in the same patchset, and the script, is
simply a helper to do that.

Note that the cros_ec_trace needs an update to match current cros_ec_commands
but I'm waiting to do the sync because we have a patchset that moves the
cros_ec_commands.h file from include/linux/mfd to include/linux/platform_data.
Once moved I'll sync the cros_ec_trace file with current cros_ec_commands

Note also that actually, we want:

- cros_ec_commands (kernel) sync with ec_commands (EC firmware)
- cros_ec_commands (kernel) sync with cros_ec_trace (kernel)

Hopefully we will have all sync soon.

Thanks,
~ Enric

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Raul Rangel]

On Thu, Aug 01, 2019 at 03:30:53PM +0200, Enric Balletbo i Serra wrote:
 
> Got it. I don't think this is a "kernel" way to do it. Also, I don't see a big
> value on doing this.

Code generation sounds great, but it makes finding and looking at the
source file difficult unless you have a build. It also makes Go to
definition in my editor fail to find the symbols since they don't exist
in the source tree.
> 
> Note also that actually, we want:
> 
> - cros_ec_commands (kernel) sync with ec_commands (EC firmware)
> - cros_ec_commands (kernel) sync with cros_ec_trace (kernel)
> 
> Hopefully we will have all sync soon.

That would be great if you synced the commands from the EC firmware :)

Thanks

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Enric Balletbo i Serra]

Hi Tzung-Bi,

On 29/8/19 6:19, Tzung-Bi Shih wrote:
> Hi Enric and Raul,
> 
> Do you have any further concerns on this patch?

This patch will conflict with [2] which hopefully will be merged on next merge
window through Lee's tree. As this patch is only changing the doc I'm willing to
wait after [2] lands. It's on my radar and don't need to resend, I'll do the
required changes.

Best Regards,
 Enric

Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script

[Enric Balletbo Serra]

Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del
dia dj., 29 d’ag. 2019 a les 15:44:
>
> Hi Tzung-Bi,
>
> On 29/8/19 6:19, Tzung-Bi Shih wrote:
> > Hi Enric and Raul,
> >
> > Do you have any further concerns on this patch?
>
> This patch will conflict with [2] which hopefully will be merged on next merge
> window through Lee's tree. As this patch is only changing the doc I'm willing to
> wait after [2] lands. It's on my radar and don't need to resend, I'll do the
> required changes.
>
> Best Regards,
>  Enric

I missed the patch link

[2] https://lkml.org/lkml/2019/8/23/475