On 15/12/2023 00:28, dean@xxxxxxxxxxxx wrote: > On 2023-12-13 04:03, Hans Verkuil wrote: >> Hi Dean, >> >> A few more comments below. >> > ... >> I never noticed this before, but there is a call to >> v4l2_ctrl_handler_setup() missing >> in this driver! That call ensures that s2255_s_ctrl() is called with >> the initial control >> values. >> >> It's probably something that should be added. > > If this is added, it needs done very carefully. Adding it to probe will 100% break the firmware load. s2255_s_ctrl sends a USB command to the board and the asynchronous firmware load shares the > endpoint for commands. > > The initial brightness, contrast, hue, and saturation default values match what the firmware sets when first loaded. It's redundant to set them again immediately after the firmware load succeeds. I > think at a minimum this belongs in a separate patch. I agree with that. If you are absolutely certain that the default values match that of the firmware, then you can omit the setup call, but that has to be documented. I see that the fw is loaded at open(), so v4l2_ctrl_handler_setup() should probably be called there once the fw is successfully loaded. Regards, Hans > > Regards, > > Dean > >> >> Regards, >> >> Hans
