| Message ID | 20210527142145.173-1-martinax.krasteva@linux.intel.com |
|---|---|
| Headers | show |
| Series | Camera Sensor Drivers | expand |
Hi Martina, On Thu, May 27, 2021 at 03:21:39PM +0100, Martina Krasteva wrote: > From: Martina Krasteva <martinax.krasteva@intel.com> > > Patch series contains Sony IMX335, Sony IMX412 and OmniVision OV9282 > camera sensor drivers and respective binding documentation. > > v1->v2: > - define maxItems for reset-gpios in dt binding document > - make sure the device is powered off on remove > - use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() > > v1: https://patchwork.kernel.org/project/linux-media/list/?series=458115 Thanks for the update. The patches seem fine, but I noticed one problem: the analogue gain is only updated when exposure is set. This is a bug. Most drivers do not try to synchronise setting analogue gain and exposure to the same frame. Do you need that? Alternatively the control framework would probably need to be amended a little --- something that would have other benefits, too. I don't think that blocks merging the drivers though. But this needs to be addressed. -- Kind regards, Sakari Ailus
Hi Sakari, Thanks for the review On 6/10/2021 10:27 PM, Sakari Ailus wrote: > Hi Martina, > > On Thu, May 27, 2021 at 03:21:39PM +0100, Martina Krasteva wrote: >> From: Martina Krasteva <martinax.krasteva@intel.com> >> >> Patch series contains Sony IMX335, Sony IMX412 and OmniVision OV9282 >> camera sensor drivers and respective binding documentation. >> >> v1->v2: >> - define maxItems for reset-gpios in dt binding document >> - make sure the device is powered off on remove >> - use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() >> >> v1: https://patchwork.kernel.org/project/linux-media/list/?series=458115 > Thanks for the update. > > The patches seem fine, but I noticed one problem: the analogue gain is only > updated when exposure is set. This is a bug. > > Most drivers do not try to synchronise setting analogue gain and exposure > to the same frame. Do you need that? Alternatively the control framework > would probably need to be amended a little --- something that would have > other benefits, too. Analog gain and exposure are "clustered". If I understand correctly, when several controls are in a cluster and one/several of them are set/get from userspace only the first control ops is called - V4L2_CID_EXPOSURE in my case. Analog gain can be set explicitly, exposure control ops will be called with analog gain new value and current exp. value. Then I could have checked the is_new flag, so I can write the reg value just to the updated control, not to both. In my case the userspace provides gain and exposure settings in sync so cluster is used to mirror that behavior. > > I don't think that blocks merging the drivers though. But this needs to be > addressed. Best Regards, Martina
Hi Martina, On Fri, Jun 11, 2021 at 10:06:00AM +0100, Krasteva, Martina wrote: > Hi Sakari, > > Thanks for the review > > On 6/10/2021 10:27 PM, Sakari Ailus wrote: > > Hi Martina, > > > > On Thu, May 27, 2021 at 03:21:39PM +0100, Martina Krasteva wrote: > > > From: Martina Krasteva <martinax.krasteva@intel.com> > > > > > > Patch series contains Sony IMX335, Sony IMX412 and OmniVision OV9282 > > > camera sensor drivers and respective binding documentation. > > > > > > v1->v2: > > > - define maxItems for reset-gpios in dt binding document > > > - make sure the device is powered off on remove > > > - use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() > > > > > > v1: https://patchwork.kernel.org/project/linux-media/list/?series=458115 > > Thanks for the update. > > > > The patches seem fine, but I noticed one problem: the analogue gain is only > > updated when exposure is set. This is a bug. > > > > Most drivers do not try to synchronise setting analogue gain and exposure > > to the same frame. Do you need that? Alternatively the control framework > > would probably need to be amended a little --- something that would have > > other benefits, too. > > Analog gain and exposure are "clustered". If I understand correctly, when > several controls are in a cluster and one/several of them are set/get from > userspace only the first control ops is called - V4L2_CID_EXPOSURE in my > case. > > Analog gain can be set explicitly, exposure control ops will be called with > analog gain new value and current exp. value. > Then I could have checked the is_new flag, so I can write the reg value just > to the updated control, not to both. > > In my case the userspace provides gain and exposure settings in sync so > cluster is used to mirror that behavior. I missed that, indeed that's fine then. -- Regards, Sakari Ailus
From: Martina Krasteva <martinax.krasteva@intel.com> Patch series contains Sony IMX335, Sony IMX412 and OmniVision OV9282 camera sensor drivers and respective binding documentation. v1->v2: - define maxItems for reset-gpios in dt binding document - make sure the device is powered off on remove - use pm_runtime_resume_and_get() instead of pm_runtime_get_sync() v1: https://patchwork.kernel.org/project/linux-media/list/?series=458115 Martina Krasteva (6): dt-bindings: media: Add bindings for imx335 media: i2c: Add imx335 camera sensor driver dt-bindings: media: Add bindings for imx412 media: i2c: Add imx412 camera sensor driver dt-bindings: media: Add bindings for ov9282 media: i2c: Add ov9282 camera sensor driver .../devicetree/bindings/media/i2c/ovti,ov9282.yaml | 91 ++ .../devicetree/bindings/media/i2c/sony,imx335.yaml | 91 ++ .../devicetree/bindings/media/i2c/sony,imx412.yaml | 91 ++ MAINTAINERS | 27 + drivers/media/i2c/Kconfig | 42 + drivers/media/i2c/Makefile | 4 +- drivers/media/i2c/imx335.c | 1129 +++++++++++++++++ drivers/media/i2c/imx412.c | 1272 ++++++++++++++++++++ drivers/media/i2c/ov9282.c | 1137 +++++++++++++++++ 9 files changed, 3883 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml create mode 100644 drivers/media/i2c/imx335.c create mode 100644 drivers/media/i2c/imx412.c create mode 100644 drivers/media/i2c/ov9282.c base-commit: f6b46ef27317b3441138b902689bd89e4f82c6f4