| Message ID | 20200630024942.20891-1-dongchun.zhu@mediatek.com |
|---|---|
| Headers | show |
| Series | media: i2c: Add support for 0V02A10 sensor | expand |
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.
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
On Tue, Jun 30, 2020 at 4:19 PM Tomasz Figa <tfiga@chromium.org> wrote: > > 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. Ah, I actually recall that the reset pin was physically active low, so we would indeed better keep it at HIGH. Sorry for the noise. Best regards, Tomasz
On Tue, Jun 30, 2020 at 04:21:31PM +0200, Tomasz Figa wrote: > On Tue, Jun 30, 2020 at 4:19 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Tue, Jun 30, 2020 at 11:54 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: ... > > > > + 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. > > Ah, I actually recall that the reset pin was physically active low, so > we would indeed better keep it at HIGH. Sorry for the noise. Here HIGH means "asserted", so in the code above it's LOW, means "deasserted", i.o.w. non-reset state. I dunno what is better, it depends to the firmware / driver expectations of the device configuration (from the power point of view, HIGH makes sense here, and not LOW, i.o.w. asserted reset line). And nice of the logical state that it doesn't depend to the active low / high (the latter just transparent to the user).
On Tue, Jun 30, 2020 at 4:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Jun 30, 2020 at 04:21:31PM +0200, Tomasz Figa wrote: > > On Tue, Jun 30, 2020 at 4:19 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > On Tue, Jun 30, 2020 at 11:54 AM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > ... > > > > > > + 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. > > > > Ah, I actually recall that the reset pin was physically active low, so > > we would indeed better keep it at HIGH. Sorry for the noise. > > Here HIGH means "asserted", so in the code above it's LOW, means "deasserted", > i.o.w. non-reset state. I dunno what is better, it depends to the firmware / > driver expectations of the device configuration (from the power point of view, > HIGH makes sense here, and not LOW, i.o.w. asserted reset line). > > And nice of the logical state that it doesn't depend to the active low / high > (the latter just transparent to the user). Yeah, after reading through the GPIO subsystem documentation and discussing with a bunch of people on how to interpret it, we concluded that the driver needs to deal only with the logical state of the signal. Actually, I later realized that the problem of leakage should likely be solved by pinctrl suspend settings, based on given hardware conditions, rather than the driver explicitly. Best regards, Tomasz
Hi Dongchun, 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 Thank you for the patch. Please see my comments inline. [snip] > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg) > +{ > + struct v4l2_subdev_format fmt = { > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, As we discussed before, this function is never called with cfg == NULL. Perhaps what we need here is to call ov02a10_set_fmt() twice, once for V4L2_SUBDEV_FORMAT_ACTIVE and then for V4L2_SUBDEV_FORMAT_TRY? Sakari, would that be a proper implementation of this function? > + .format = { > + .width = 1600, > + .height = 1200, > + } > + }; > + > + ov02a10_set_fmt(sd, cfg, &fmt); > + > + return 0; [snip] With this and Sakari's comment about the initial state of the reset pin fixed, feel free to add my Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
On Tue, Jun 30, 2020 at 05:07:46PM +0000, Tomasz Figa wrote: > Hi Dongchun, > > 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 > > Thank you for the patch. Please see my comments inline. > > [snip] > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > As we discussed before, this function is never called with cfg == NULL. > Perhaps what we need here is to call ov02a10_set_fmt() twice, once for > V4L2_SUBDEV_FORMAT_ACTIVE and then for V4L2_SUBDEV_FORMAT_TRY? > > Sakari, would that be a proper implementation of this function? It's fine to test fmt, but it should be only done if the driver calls the function with ACTIVE format. I.e. it can be removed here, and always use TRY. > > > + .format = { > > + .width = 1600, > > + .height = 1200, > > + } > > + }; > > + > > + ov02a10_set_fmt(sd, cfg, &fmt); > > + > > + return 0; > [snip] > > With this and Sakari's comment about the initial state of the reset pin > fixed, feel free to add my > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > Best regards, > Tomasz
On Tue, 2020-06-30 at 16:40 +0200, Tomasz Figa wrote: > On Tue, Jun 30, 2020 at 4:37 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Jun 30, 2020 at 04:21:31PM +0200, Tomasz Figa wrote: > > > On Tue, Jun 30, 2020 at 4:19 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > > On Tue, Jun 30, 2020 at 11:54 AM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > ... > > > > > > > > + 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. > > > > > > Ah, I actually recall that the reset pin was physically active low, so > > > we would indeed better keep it at HIGH. Sorry for the noise. > > > > Here HIGH means "asserted", so in the code above it's LOW, means "deasserted", > > i.o.w. non-reset state. I dunno what is better, it depends to the firmware / > > driver expectations of the device configuration (from the power point of view, > > HIGH makes sense here, and not LOW, i.o.w. asserted reset line). > > > > And nice of the logical state that it doesn't depend to the active low / high > > (the latter just transparent to the user). > > Yeah, after reading through the GPIO subsystem documentation and > discussing with a bunch of people on how to interpret it, we concluded > that the driver needs to deal only with the logical state of the > signal. > > Actually, I later realized that the problem of leakage should likely > be solved by pinctrl suspend settings, based on given hardware > conditions, rather than the driver explicitly. > Thank you for all your explanation. Fixed in next release. > Best regards, > Tomasz
Hi Tomasz, Sakari, Thanks for the review. On Tue, 2020-06-30 at 21:47 +0300, Sakari Ailus wrote: > On Tue, Jun 30, 2020 at 05:07:46PM +0000, Tomasz Figa wrote: > > Hi Dongchun, > > > > 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 > > > > Thank you for the patch. Please see my comments inline. > > > > [snip] > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_pad_config *cfg) > > > +{ > > > + struct v4l2_subdev_format fmt = { > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > As we discussed before, this function is never called with cfg == NULL. > > Perhaps what we need here is to call ov02a10_set_fmt() twice, once for > > V4L2_SUBDEV_FORMAT_ACTIVE and then for V4L2_SUBDEV_FORMAT_TRY? > > > > Sakari, would that be a proper implementation of this function? > > It's fine to test fmt, but it should be only done if the driver calls the > function with ACTIVE format. I.e. it can be removed here, and always use > TRY. > Sorry for my late coming. The implementation of this function should be common(similar to OV2680/OV5645). If it needs to update to be more proper or clear, as user always sets format.which to ACTIVE when calling set_fmt, we could only reserve the TRY format in init_cfg like this: struct v4l2_subdev_format fmt = { which = V4L2_SUBDEV_FORMAT_TRY, > > > > > + .format = { > > > + .width = 1600, > > > + .height = 1200, > > > + } > > > + }; > > > + > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > + > > > + return 0; > > [snip] > > > > With this and Sakari's comment about the initial state of the reset pin > > fixed, feel free to add my > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > Best regards, > > Tomasz >
On Wed, Jul 01, 2020 at 04:47:22PM +0800, Dongchun Zhu wrote: > Hi Tomasz, Sakari, > > Thanks for the review. > > On Tue, 2020-06-30 at 21:47 +0300, Sakari Ailus wrote: > > On Tue, Jun 30, 2020 at 05:07:46PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > 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 > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg) > > > > +{ > > > > + struct v4l2_subdev_format fmt = { > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > > > As we discussed before, this function is never called with cfg == NULL. > > > Perhaps what we need here is to call ov02a10_set_fmt() twice, once for > > > V4L2_SUBDEV_FORMAT_ACTIVE and then for V4L2_SUBDEV_FORMAT_TRY? > > > > > > Sakari, would that be a proper implementation of this function? > > > > It's fine to test fmt, but it should be only done if the driver calls the > > function with ACTIVE format. I.e. it can be removed here, and always use > > TRY. > > > > Sorry for my late coming. > The implementation of this function should be common(similar to > OV2680/OV5645). > If it needs to update to be more proper or clear, as user always sets > format.which to ACTIVE when calling set_fmt, we could only reserve the > TRY format in init_cfg like this: > struct v4l2_subdev_format fmt = { > which = V4L2_SUBDEV_FORMAT_TRY, Yes, please.