Re: [PATCH v2 0/6] Camera Sensor Drivers

["Krasteva, Martina" <martinax.krasteva@linux.intel.com>]

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