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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Jul 18, 2025 at 07:06:10PM +0300, Dan Carpenter wrote:
> 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.

...

> >  		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.

Still, it's good to have it to avoid any additional churn in case it
might be extended. We can argue if this needs to be a separate commit
from the main topic of this patch.

> 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.

Here we touched the line, and adding trailing comma just reduces a potential
churn in the future. I can show you plenty of changes when patch touches
unrelated line just for the sake of adding a new one after the affected.

...

> 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

With the trailing comma it will be better for the consistency in this case.
Otherwise I like your approach.

>  	},
> -	{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.

-- 
With Best Regards,
Andy Shevchenko