linux-kernel - Re: [PATCH v10] staging: media: atomisp: fix indentation in aa, anr, and bh modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f7db034-6b38-44c3-b841-ef4bc1db3973@suswa.mountain>
Date: Fri, 18 Jul 2025 19:06:10 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: LiangCheng Wang <zaq14760@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>, Hans de Goede <hansg@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
	llvm@...ts.linux.dev
Subject: Re: [PATCH v10] staging: media: atomisp: fix indentation in aa, anr,
 and bh modules

On Fri, Jul 18, 2025 at 11:02:14PM +0800, LiangCheng Wang wrote:
> Fix tab/space indentation and move a standalone kernel-doc
> comment of the 'strength' field of the struct ia_css_aa_config
> to the whole-structure one.
> Align with kernel coding style guidelines.

There are too many changes all at once and some of the changes are not
described in the commit message.

> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
> index 899d566234b9d3a35401666dcf0c7b1b80fd5b31..488807a161b9a6ba9ebc4a557221cd21bd1df108 100644
> --- a/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
> @@ -16,25 +16,21 @@ const struct ia_css_anr_config default_anr_config = {
>  		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
>  		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
>  		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
> -		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4
> +		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,

No need to add a comma to this line.  The comma at the end of the line
is useful when we might add another element to an array.  But here the
length is fixed.

If someone were to add a comma here and it was new code, then that's
fine.  But I don't want to have to review a separate patch which only
adds a unnecessary comma.

>  	},
> -	{10, 20, 30}
> +	{ 10, 20, 30 },

Same here.  This comma serves no purpose.  We can't actually add
anything to this struct.  What would be actually helpful would be to
use designated initializers.

diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
index 899d566234b9..3de7ebea3d6e 100644
--- a/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
+++ b/drivers/staging/media/atomisp/pci/isp/kernels/anr/anr_1.0/ia_css_anr.host.c
@@ -11,14 +11,14 @@
 #include "ia_css_anr.host.h"

 const struct ia_css_anr_config default_anr_config = {
-	10,
-	{
+	.threshold = 10,
+	.thresholds = {
 		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
 		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
 		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4,
 		0, 3, 1, 2, 3, 6, 4, 5, 1, 4, 2, 3, 2, 5, 3, 4
 	},
-	{10, 20, 30}
+	.factors = {10, 20, 30},
 };

 void

I added a comma to the end of .factors because there is a 1% change we
will add a new member to the struct and it's the right thing to do.  I
was already changing that line, so I'm allowed to make tiny white space
changes like this.

But notice how I left off the comma after the numbers.  That array is a
fixed size and nothing can be added.  Leaving off the comma communicates
that.  Also there was no need to change that line.  It's unrelated to
using desgnated initializers.  If you added a comma, you would need to
send a separate patch for that with a commit message to describe and
justify it.  As a reviewer, I would need to go through the line
carefully and verify that none of the other numbers had been changed.

The commit message for the above patch would say, "Use a designated
initializer for default_anr_config.  It helps readability."  There would
be no need to mention that "I added a comma" to the end of the .factors
line because it's a minor thing that we're not really stressed about.

regards,
dan carpenter