On 30/05/2024 22:21, Laurent Pinchart wrote: > 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 :-) I agree with Laurent on this. Sensor and debayer subdev devices have hardwired pads determined by the hardware, it is not something that is flexible. Since this is also serves as an example of such a driver, it makes sense to hardcode it, as that is how it is done in practice. It would be nice though to use defines rather than hardcoding it to 1 in a line like this: mf = v4l2_subdev_state_get_format(sd_state, 1); It would make it easier to read and see which pad index is source and which is sink. The sensor has only one pad, so there you can use the index 0, creating PAD defines doesn't add anything for a sensor. Regards, Hans > >> Please leave it the way it is. >
