[v2,0/5] adds ovm6211 driver to staging

  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
  

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