Re: [PATCH v1 1/9] media: vimc: Don't iterate over single pad

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, May 30, 2024 at 02:18:05PM -0600, Shuah Khan wrote:
> On 5/30/24 13:45, Laurent Pinchart wrote:
> > On Thu, May 30, 2024 at 01:27:53PM -0600, Shuah Khan wrote:
> >> On 4/24/24 17:57, Laurent Pinchart wrote:
> >>> The .init_state() operations of the debayer and sensor entities iterate
> >>> over the entity's pads. In practice, the iteration covers a single pad
> >>> only. Access the pad directly and remove the loops.
> >>
> >> I am not seeing much of a reason to do this. This code is good
> >> even when num_pads change.
> >>
> >> Don't change the loops.
> > 
> > Why so ? Beside the fact that the loop wastes some CPU cycles, the
> > current code implies that there would be multiple source pads, which is
> > confusing for the reader. I think the result of this patch is both
> > improved efficiency and improved readability.
> 
> It is currently flexible and if and when more pads get added,
> there is no need to change it. I am not concerned about the
> efficiency on this test driver. Also people use the test
> driver as a sample.

If pad gets added, we don't know yet if the code will work as-is.
Chances are it will need to change anyway. I don't think it's a good
idea to prepare for a situation that we can't foresee without having
good reasons to make assumptions.

I have plans to refactor the vimc driver exteensively, changing how the
different entities behave, to bring it closer to how a real inline ISP
is architectured. The .init_state() functions will likely be rewritten
completely.

I agree with the sample argument, and that's one more reason why I think
this patch does the right thing :-)

> Please leave it the way it is.

-- 
Regards,

Laurent Pinchart