Hi Tomasz, On Wed, Sep 11, 2019 at 07:12:02PM +0900, Tomasz Figa wrote: > Hi Sakari, > > On Tue, Sep 10, 2019 at 10:05 PM <dongchun.zhu@xxxxxxxxxxxx> wrote: > > > > From: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx> > > > > This patch mainly adds two more sensor modes for OV8856 CMOS image sensor. > > That is, the resolution of 1632*1224 and 3264*2448, corresponding to the bayer order of BGGR. > > The sensor revision also differs in some OTP register. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx> > > --- > > drivers/media/i2c/ov8856.c | 654 +++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 639 insertions(+), 15 deletions(-) > > > > What do you think about the approach taken by this patch? > > My understanding is that the register arrays being added by it can be > only used with 24MHz input clock, while the existing ones are for > 19.2MHz. That means that this patch makes the driver expose completely > different modes (resolutions, mbus formats) depending on the input > clock. Are we okay with this? These register list based drivers only support a tiny subset of configurations a sensor can support, and the number of those configurations may be amended over time. I don't see a problem in choosing a different set of available configurations based on the external clock frequency; that may, after all, cause that some of the configurations, at a particular frame rate, are not even achievable --- albeit this is perhaps unlikely in this case. In practice, it's often the case that the sensor vendor provides these configurations and the vendor may provide different configurations (including output resolutions etc.) to different parties. So it may well be the submitter of the patch would also not have access to similar configurations (output size, cropping etc.) that now exist in the driver. I'll review the patch itself soonish. -- Regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
