The function configure_isp_from_args() contained a duplicate call to
ia_css_output0_configure() using the same output frame index. Remove
the redundant call to simplify the configuration path.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 2904455b35f7..1612bb2fd8b0 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -796,9 +796,6 @@ static int configure_isp_from_args(const struct sh_css_sp_pipeline *pipeline,
 	if (ret)
 		return ret;
 	ret = ia_css_copy_output_configure(binary, args->copy_output);
-	if (ret)
-		return ret;
-	ret = ia_css_output0_configure(binary, ia_css_frame_get_info(args->out_frame[0]));
 	if (ret)
 		return ret;
 	ret = ia_css_iterator_configure(binary, ia_css_frame_get_info(args->in_frame));
-- 
2.53.0

On Sat, Mar 28, 2026 at 08:21:38PM +0100, Jose A. Perez de Azpillaga wrote:
> The function configure_isp_from_args() contained a duplicate call to
> ia_css_output0_configure() using the same output frame index. Remove
> the redundant call to simplify the configuration path.
> 
> Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> ---

This feels like a guess work patch.  Sure, it looks like duplicate
code but duplicate code isn't always wrong.  How do you know which
call to remove?  How do you know that it's not a copy and paste error
and the right fix is just to change the code instead of deleting it?

Patch 1 felt like an AI patch, and this patch feels even more strongly
like an AI patch.

Please don't send guess work patches or if you do add a giant comment
at the bottom saying --- "This patch is a GUESS.  Review carefully!
Untested"

regards,
dan carpenter

On Mon, Mar 30, 2026 at 12:35:34PM +0300, Dan Carpenter wrote:
> On Sat, Mar 28, 2026 at 08:21:38PM +0100, Jose A. Perez de Azpillaga wrote:
> > The function configure_isp_from_args() contained a duplicate call to
> > ia_css_output0_configure() using the same output frame index. Remove
> > the redundant call to simplify the configuration path.
> >
> > Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> > ---
>
> This feels like a guess work patch.  Sure, it looks like duplicate
> code but duplicate code isn't always wrong.  How do you know which
> call to remove?  How do you know that it's not a copy and paste error
> and the right fix is just to change the code instead of deleting it?
>

my response here would be something similar to Andy's one.

> Patch 1 felt like an AI patch, and this patch feels even more strongly
> like an AI patch.
>

ouch.

> Please don't send guess work patches or if you do add a giant comment
> at the bottom saying --- "This patch is a GUESS.  Review carefully!
> Untested"
>

okay, I wasn't aware of that. I'll try to add that comment or better
yet, not trying to fix things without the actual hardware. thanks.

...

regards,
jose a. p-a

On Sat, Mar 28, 2026 at 9:27 PM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
>
> The function configure_isp_from_args() contained a duplicate call to
> ia_css_output0_configure() using the same output frame index. Remove
> the redundant call to simplify the configuration path.

This requires more information, in particular to explain if the order
has no side effects. It might be that double configuration has side
effects and removal (wrong) one may lead to other currently hidden
issues.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Mar 30, 2026 at 12:01:21PM +0300, Andy Shevchenko wrote:
> On Sat, Mar 28, 2026 at 9:27 PM Jose A. Perez de Azpillaga
> <azpijr@gmail.com> wrote:
> >
> > The function configure_isp_from_args() contained a duplicate call to
> > ia_css_output0_configure() using the same output frame index. Remove
> > the redundant call to simplify the configuration path.
>
> This requires more information, in particular to explain if the order
> has no side effects. It might be that double configuration has side
> effects and removal (wrong) one may lead to other currently hidden
> issues.
>

mhm... my bad for that,

the ia_css_output0_configure() function acts as a configuration setter.
it populates a struct ia_css_output0_configuration from the frame info
and caches it in the binary parameters. calling it twice with the same
out_frame[0] pointer merely overwrites the exact same state with
identical values. it has no cumulative state, either does its order
matter relative to ia_css_copy_output_configure().

...

regards,
jose a. p-a

On Tue, Mar 31, 2026 at 9:03 AM Jose A. Perez de Azpillaga
<azpijr@gmail.com> wrote:
> On Mon, Mar 30, 2026 at 12:01:21PM +0300, Andy Shevchenko wrote:
> > On Sat, Mar 28, 2026 at 9:27 PM Jose A. Perez de Azpillaga
> > <azpijr@gmail.com> wrote:
> > >
> > > The function configure_isp_from_args() contained a duplicate call to
> > > ia_css_output0_configure() using the same output frame index. Remove
> > > the redundant call to simplify the configuration path.
> >
> > This requires more information, in particular to explain if the order
> > has no side effects. It might be that double configuration has side
> > effects and removal (wrong) one may lead to other currently hidden
> > issues.
>
> mhm... my bad for that,
>
> the ia_css_output0_configure() function acts as a configuration setter.
> it populates a struct ia_css_output0_configuration from the frame info
> and caches it in the binary parameters. calling it twice with the same
> out_frame[0] pointer merely overwrites the exact same state with
> identical values. it has no cumulative state, either does its order
> matter relative to ia_css_copy_output_configure().

So, no call in between accesses that parameter or relies on its state?
With this analysis in the commit message the change looks good.
Please, amend it in v2.

-- 
With Best Regards,
Andy Shevchenko