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:

quoted

Hi Martina,

On Thu, May 27, 2021 at 03:21:39PM +0100, Martina Krasteva wrote:

quoted

From: Martina Krasteva <redacted>

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