[PATCH v1 0/2] Style fixes for EDAC/bluefield driver

[PATCH v1 0/2] Style fixes for EDAC/bluefield driver

[David Thompson]

This patch series provides two style fixes for the BlueField EDAC driver.

David Thompson (2):
  EDAC/bluefield: fix white space in bluefield_edac_mc_probe
  EDAC/bluefield: fix prefix for GET_DIMM_INFO defines

 drivers/edac/bluefield_edac.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.30.1

[PATCH v1 1/2] EDAC/bluefield: fix white space in bluefield_edac_mc_probe

[David Thompson]

This patch removes an empty line in bluefield_edac_mc_probe().

Signed-off-by: David Thompson <davthompson@nvidia.com>
Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
---
 drivers/edac/bluefield_edac.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
index 5b3164560648..1f6f39a7dbf3 100644
--- a/drivers/edac/bluefield_edac.c
+++ b/drivers/edac/bluefield_edac.c
@@ -320,7 +320,6 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
 	edac_mc_free(mci);
 
 	return ret;
-
 }
 
 static void bluefield_edac_mc_remove(struct platform_device *pdev)
-- 
2.30.1

Re: [PATCH v1 1/2] EDAC/bluefield: fix white space in bluefield_edac_mc_probe

[Borislav Petkov]

On Wed, Jun 12, 2024 at 03:38:30PM -0400, David Thompson wrote:
> This patch removes an empty line in bluefield_edac_mc_probe().

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

Also, feel free to peruse that whole directory.

> Signed-off-by: David Thompson <davthompson@nvidia.com>
> Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> ---
>  drivers/edac/bluefield_edac.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
> index 5b3164560648..1f6f39a7dbf3 100644
> --- a/drivers/edac/bluefield_edac.c
> +++ b/drivers/edac/bluefield_edac.c
> @@ -320,7 +320,6 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
>  	edac_mc_free(mci);
>  
>  	return ret;
> -
>  }
>  
>  static void bluefield_edac_mc_remove(struct platform_device *pdev)
> -- 

So just the effort to create a whole patch just for that is an overkill.

Please do not do that. If you notice very minor style issues like that, you can
do them when touching this code as part of a change with more substance. Or you
can simply ignore such minor issues.

Whitespace cleanup like that gets in the way of real work and pretty much all
maintainers are overworked already.

So I'd appreciate it if you concentrate on real fixes and improvements.

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v1 1/2] EDAC/bluefield: fix white space in bluefield_edac_mc_probe

[David Thompson]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Wednesday, June 12, 2024 4:01 PM
> To: David Thompson <davthompson@nvidia.com>
> Cc: tony.luck@intel.com; james.morse@arm.com; mchehab@kernel.org;
> rric@kernel.org; Shravan Ramani <shravankr@nvidia.com>; linux-
> edac@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] EDAC/bluefield: fix white space in
> bluefield_edac_mc_probe
> 
> On Wed, Jun 12, 2024 at 03:38:30PM -0400, David Thompson wrote:
> > This patch removes an empty line in bluefield_edac_mc_probe().
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
> 

I did review this section of the kernel documentation and then
went on to create a v2 with the updated commit message.
I apologize but I didn't notice your comment below about 
your preference to not have this type of patch at all.  I have been
told in the past to have separate patches for style cleanups, and
not to bundle them with features.  But I can do as you recommend
for next version.  I will squash the two style fixes into a patch that
contains some new functionality for the BlueField EDAC driver.

> Also, feel free to peruse that whole directory.
> 
> > Signed-off-by: David Thompson <davthompson@nvidia.com>
> > Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> > ---
> >  drivers/edac/bluefield_edac.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/edac/bluefield_edac.c
> > b/drivers/edac/bluefield_edac.c index 5b3164560648..1f6f39a7dbf3
> > 100644
> > --- a/drivers/edac/bluefield_edac.c
> > +++ b/drivers/edac/bluefield_edac.c
> > @@ -320,7 +320,6 @@ static int bluefield_edac_mc_probe(struct
> platform_device *pdev)
> >  	edac_mc_free(mci);
> >
> >  	return ret;
> > -
> >  }
> >
> >  static void bluefield_edac_mc_remove(struct platform_device *pdev)
> > --
> 
> So just the effort to create a whole patch just for that is an overkill.
> 
> Please do not do that. If you notice very minor style issues like that, you can do
> them when touching this code as part of a change with more substance. Or you
> can simply ignore such minor issues.
> 
> Whitespace cleanup like that gets in the way of real work and pretty much all
> maintainers are overworked already.
> 
> So I'd appreciate it if you concentrate on real fixes and improvements.
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v1 1/2] EDAC/bluefield: fix white space in bluefield_edac_mc_probe

[Borislav Petkov]

On Thu, Jun 13, 2024 at 03:06:38PM +0000, David Thompson wrote:
> I did review this section of the kernel documentation and then
> went on to create a v2 with the updated commit message.
> I apologize but I didn't notice your comment below about 
> your preference to not have this type of patch at all.  I have been
> told in the past to have separate patches for style cleanups, and

Yeah, it all depends on the maintainer. I, personally, do not encourage style
cleanups only because they interfere with git archeology and teach new
submitters not to chase real bugs but do silly patches only.

> not to bundle them with features.  But I can do as you recommend for next
> version.  I will squash the two style fixes into a patch that contains some
> new functionality for the BlueField EDAC driver.

Thanks.

Simply use checkpatch on your patches only but not on the .c files directly. 

-- 
Regards/Gruss,
    Boris.

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

[PATCH v1 2/2] EDAC/bluefield: fix prefix for GET_DIMM_INFO defines

[David Thompson]

This patch updates the prefix used in the "get DIMM info"
definitions to align with other #defines used by driver.

Signed-off-by: David Thompson <davthompson@nvidia.com>
Reviewed-by: Shravan Kumar Ramani <shravankr@nvidia.com>
---
 drivers/edac/bluefield_edac.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
index 1f6f39a7dbf3..8f1cc029606f 100644
--- a/drivers/edac/bluefield_edac.c
+++ b/drivers/edac/bluefield_edac.c
@@ -48,12 +48,12 @@
 #define MLXBF_EDAC_ERROR_GRAIN		8
 
 /*
- * Request MLNX_SIP_GET_DIMM_INFO
+ * Request MLXBF_SIP_GET_DIMM_INFO
  *
  * Retrieve information about DIMM on a certain slot.
  *
  * Call register usage:
- * a0: MLNX_SIP_GET_DIMM_INFO
+ * a0: MLXBF_SIP_GET_DIMM_INFO
  * a1: (Memory controller index) << 16 | (Dimm index in memory controller)
  * a2-7: not used.
  *
@@ -61,7 +61,7 @@
  * a0: MLXBF_DIMM_INFO defined below describing the DIMM.
  * a1-3: not used.
  */
-#define MLNX_SIP_GET_DIMM_INFO		0x82000008
+#define MLXBF_SIP_GET_DIMM_INFO		0x82000008
 
 /* Format for the SMC response about the memory information */
 #define MLXBF_DIMM_INFO__SIZE_GB GENMASK_ULL(15, 0)
@@ -189,7 +189,7 @@ static void bluefield_edac_init_dimms(struct mem_ctl_info *mci)
 		dimm = mci->dimms[i];
 
 		smc_arg = mem_ctrl_idx << 16 | i;
-		smc_info = smc_call1(MLNX_SIP_GET_DIMM_INFO, smc_arg);
+		smc_info = smc_call1(MLXBF_SIP_GET_DIMM_INFO, smc_arg);
 
 		if (!FIELD_GET(MLXBF_DIMM_INFO__SIZE_GB, smc_info)) {
 			dimm->mtype = MEM_EMPTY;
-- 
2.30.1