On 22-01-03 13:19:22, Kieran Bingham wrote: > Hi Petko, > > Quoting Petko Manolov (2022-01-03 11:09:17) > > v2: Removes an unused function (ovm6211_set_pix_clk) and this patch series is > > now based on media/master; Didn't receive any comments about the RFC version, > > thus i assume everything is perfect... :P > > Did you see > https://lore.kernel.org/linux-media/Ya9XHiz%2FPm4CjQ13@xxxxxxxxxxxxxxxxxxxxxx/? > > Sakari provided quite a few review comments to consider. Nope, somehow his message has slipped from my attention. I'd like to thank Sakari for the thorough review. This is my first v4l2 driver and i have most likely made a lot of mistakes. I'll address all his comments in v3 of the series along with some elaboration on my part. > I don't think we need to add new sensor drivers to the staging directory which > would simplify your series quite a bit, and Sakari also stated the ovm6211 > KConfig and Makefile entry should be in the patch along with the new driver > code (not in staging). This is the exact opposite to what i've done for the netdev tree, where each change should be in a separate patch. Anyway, i'll follow the media tree rules. > So you would need to refactor this series to a single patch adding the driver > do drivers/media/i2c/, and a second patch which adds the DT-bindings > accordingly. I am not sure about how practical the DT will be in this case. The sensor was used on a custom board and a rather specific reset pin wiring. I've tried to remove this logic from the driver, but it is still reflected in the DT that we've been using so far. I've got to think about this one some more... cheers, Petko
