Re: [PATCH 1/6] EDAC: fsl_ddr: Pass down fsl_mc_pdata in ddr_in32() and ddr_out32()

[Borislav Petkov]

On Tue, Jul 09, 2024 at 04:23:02PM -0400, Frank Li wrote:
Subject: Re: [PATCH 1/6] EDAC: fsl_ddr: Pass down fsl_mc_pdata in ddr_in32() and ddr_out32()

The subject prefixes in the EDAC subsystem have the following format:

"EDAC/driver: <Sentence starting with a capital letter"

> Pass down fsl_mc_data in help function ddr_in32() and ddr_out32() to
> prepare add iMX9 support. iMX9 have a little difference register layout.

Since everyone is using AI nowadays, you could have it verify your commit
messages. I did this:

| ChatGPT, Please verify and correct the grammar in this english text: "Pass
| down fsl_mc_data in help function ddr_in32() and ddr_out32() to prepare add
| iMX9 support. iMX9 have a little difference register layout."

It replied with:

| "Pass down fsl_mc_data in the helper functions ddr_in32() and ddr_out32() to
| prepare for adding iMX9 support. The iMX9 has a slightly different register
| layout."
| 
| Changes made:
| 1. "help function" changed to "helper functions" for clarity.
| 
| 2. "prepare add iMX9 support" changed to "prepare for adding iMX9 support" for
|    grammatical correctness.
| 
| 3. "iMX9 have a little difference register layout" changed to "The iMX9 has
|    a slightly different register layout" for subject-verb agreement and
|    smoother phrasing.

And this all looks good to me.

With all the cringe we all get from AI, I think it is very useful for
verifying commit messages.

Do that for all your commit messages pls.

Thx.

> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
>  drivers/edac/fsl_ddr_edac.c | 62 ++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)

How did you test these patches of yours?

They don't even build!

drivers/edac/fsl_ddr_edac.c: In function 'fsl_mc_err_probe':
drivers/edac/fsl_ddr_edac.c:538:21: error: too few arguments to function 'ddr_in32'
  538 |         sdram_ctl = ddr_in32(pdata->mc_vbase + FSL_MC_DDR_SDRAM_CFG);
      |                     ^~~~~~~~
drivers/edac/fsl_ddr_edac.c:38:19: note: declared here
   38 | static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)
      |                   ^~~~~~~~
make[4]: *** [scripts/Makefile.build:229: drivers/edac/fsl_ddr_edac.o] Error 1
make[3]: *** [scripts/Makefile.build:478: drivers/edac] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [scripts/Makefile.build:478: drivers] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1936: .] Error 2
make: *** [Makefile:224: __sub-make] Error 2

Before you submit next time, build-test *every* *single* patch of yours and
test the driver with all of them.

This should not ever happen in submission.

Stopping review here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette