Re: [PATCH V11 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Tomasz Figa <tfiga@chromium.org>]

On Tue, Jun 30, 2020 at 11:54 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Dongchun,
>
> Thanks for the update.
>
> On Tue, Jun 30, 2020 at 10:49:42AM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for OV02A10 image sensor.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  MAINTAINERS                 |    1 +
> >  drivers/media/i2c/Kconfig   |   13 +
> >  drivers/media/i2c/Makefile  |    1 +
> >  drivers/media/i2c/ov02a10.c | 1052 +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1067 insertions(+)
> >  create mode 100644 drivers/media/i2c/ov02a10.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 378c961..a6a2f8b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12566,6 +12566,7 @@ L:    linux-media@vger.kernel.org
> >  S:   Maintained
> >  T:   git git://linuxtv.org/media_tree.git
> >  F:   Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > +F:   drivers/media/i2c/ov02a10.c
> >
> >  OMNIVISION OV13858 SENSOR DRIVER
> >  M:   Sakari Ailus <sakari.ailus@linux.intel.com>
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index da11036..65519cf 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -812,6 +812,19 @@ config VIDEO_IMX355
> >         To compile this driver as a module, choose M here: the
> >         module will be called imx355.
> >
> > +config VIDEO_OV02A10
> > +     tristate "OmniVision OV02A10 sensor support"
> > +     depends on I2C && VIDEO_V4L2
> > +     select MEDIA_CONTROLLER
> > +     select VIDEO_V4L2_SUBDEV_API
> > +     select V4L2_FWNODE
> > +     help
> > +       This is a Video4Linux2 sensor driver for the OmniVision
> > +       OV02A10 camera.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called ov02a10.
> > +
> >  config VIDEO_OV2640
> >       tristate "OmniVision OV2640 sensor support"
> >       depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 993acab..384e676 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
> >  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
> >  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
> >  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
> > +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
> >  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> >  obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
> >  obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
> > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
> > new file mode 100644
> > index 0000000..f7fd329
> > --- /dev/null
> > +++ b/drivers/media/i2c/ov02a10.c
>
> ...
>
> > +     ov02a10->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>
>
> Shouldn't this be GPIOD_OUT_HIGH? That's the state it'll be after powering
> the sensor off the first time. Alternatively make reset signal high in
> runtime suspend callback.

We might want to keep the signals low when the regulators are powered
down, to reduce the leakage.

Best regards,
Tomasz