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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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