| Message ID | 20220103110922.715065-1-petko.manolov@konsulko.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | adds ovm6211 driver to staging | expand |
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@valkosipuli.retiisi.eu/? Sakari provided quite a few review comments to consider. 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). 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. -- Kieran > > This patch adds ovm6211 driver into the staging directory. It also creates > media/i2c entry, where ovm6211.c lives for now, to mimic the generic media > source tree. > > Petko Manolov (5): > adds ovm6211 driver to staging > adds ovm6211 entry to Kconfig > adds ovm6211 entry to Makefile > adds drivers/staging/media/i2c/Kconfig entry > adds i2c/ explicitly to Makefile > > drivers/staging/media/Kconfig | 2 + > drivers/staging/media/Makefile | 1 + > drivers/staging/media/i2c/Kconfig | 9 + > drivers/staging/media/i2c/Makefile | 1 + > drivers/staging/media/i2c/ovm6211.c | 1143 +++++++++++++++++++++++++++ > 5 files changed, 1156 insertions(+) > create mode 100644 drivers/staging/media/i2c/Kconfig > create mode 100644 drivers/staging/media/i2c/Makefile > create mode 100644 drivers/staging/media/i2c/ovm6211.c > > > base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c > -- > 2.30.2 >
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@valkosipuli.retiisi.eu/? > > 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
Quoting Petko Manolov (2022-01-03 21:19:21) > 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@valkosipuli.retiisi.eu/? > > > > 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... That's why the bindings are different from the DT itself. The specifics of how you connect it up belong to your device tree, but the bindings are there to state how you describe the hardware. Think of the bindings as the documentation part for how someone would write the device-tree nodes for your sensor driver to use it on their custom platform. It should be something like: Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml for example. As I understand it, any driver which adds a .compatible string must provide bindings. -- Kieran
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 This patch adds ovm6211 driver into the staging directory. It also creates media/i2c entry, where ovm6211.c lives for now, to mimic the generic media source tree. Petko Manolov (5): adds ovm6211 driver to staging adds ovm6211 entry to Kconfig adds ovm6211 entry to Makefile adds drivers/staging/media/i2c/Kconfig entry adds i2c/ explicitly to Makefile drivers/staging/media/Kconfig | 2 + drivers/staging/media/Makefile | 1 + drivers/staging/media/i2c/Kconfig | 9 + drivers/staging/media/i2c/Makefile | 1 + drivers/staging/media/i2c/ovm6211.c | 1143 +++++++++++++++++++++++++++ 5 files changed, 1156 insertions(+) create mode 100644 drivers/staging/media/i2c/Kconfig create mode 100644 drivers/staging/media/i2c/Makefile create mode 100644 drivers/staging/media/i2c/ovm6211.c base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c