Re: [PATCH] media: i2c: ov08d10: add get_selection support

[BRETSCHNEIDER Max]

Hi Sakari, 

thank you for the review! Sorry it took me a while to get through this. v2 follows.

> > Tested on a Hexagon/Leica CS50 (i.MX8MP, rkisp1) running 6.16.12 with the
> > equivalent of this patch applied, where it fixes configuration of the 2x2
> > binned 1632x1224 mode with libcamera's rkisp1 CAC algorithm enabled.
> > Compile-tested against media/next.
>
> Can you test on a newer kernel? That being said, there are unlikely to be
> issues without that, but it's better to be sure.

Yes I will test this on a newer kernel as well but my hardware access is currently 
a bit limited, so it will probably take me a week or two. 

> > +     .get_selection = ov08d10_get_selection,
>
> It'd be nice to have the same ordering as in the driver (and also not place
> the get_selection callback in the middle of control handling).

Moved it out of the control handling section so it now sits with the other
pad op implementations. With that the struct order already matches the 
order in the driver, so I have left the struct itself alone (apart from adding
the get_selection entry)

> > +     ret = v4l2_subdev_init_finalize(&ov08d10->sd);
>
> Can you convert this driver to fully utilise sub-device state, as you're
> initialising it here?
>
> There's probably not much to do, but in particular the active format should
> be stored in sub-device state and the control handler's lock be used as the
> state lock. Also storing the active mode in the context struct would be
> nice to avoid. These should constitute a separate patch (that also includes
> init_finalize()/cleanup() calls) before this one.

Done, but I stumbled on some things which is why this grew into a small series. v2 is:
1. A fix for a NULL pointer dereference in ov08d10_get_hwcfg(), which logs 
ov08d10->cur_mode->data_lanes before cur_mode is assigned in probe(). I found this 
while doing patch #3. It is tagged for stable and patch 3 does depend on it, but of
 course I am happy to send that seperately if you would prefer this.
2. Conversion to the subdev active state.
3. Dropping the (now-redundant) cached mode pointer and recovering the mode 
now from the state (like imx{214,355} do).
4. get_selection, as in v1.

One thing about patch 2 I was unsure about and wanted to mention: it replaces the
 driver's get_fmt() with v4l2_subdev_get_fmt(). The old get_fmt() recomputed the media
 bus code from the flip controls on every call. The new version in patch 2 now refreshes 
the stored code in set_ctrl() instead. There are other ways to deal with this which I found, 
for example ov02c10, where the readout window is shifted when flipping to avoid a Bayer
 order mismatch entirely. I am not fully certain but maybe this would be an option on the 
ov08d10 too, although whether it would be the better option I don't know. 

Also I thought it'd now make sense to split patches 2 and 3 mainly so that the media bus
code refresh part in patch 2 can be reviewed on its own. Of course I'd also be happy to 
squash them if you would prefer that.

--
Thanks again and best regards,
Max