From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B291EC5ACAE for ; Wed, 11 Sep 2019 11:43:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8FDA221479 for ; Wed, 11 Sep 2019 11:43:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727834AbfIKLnK (ORCPT ); Wed, 11 Sep 2019 07:43:10 -0400 Received: from mga17.intel.com ([192.55.52.151]:14773 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727817AbfIKLnI (ORCPT ); Wed, 11 Sep 2019 07:43:08 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2019 04:43:07 -0700 X-IronPort-AV: E=Sophos;i="5.64,489,1559545200"; d="scan'208";a="360110894" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2019 04:43:03 -0700 Received: by paasikivi.fi.intel.com (Postfix, from userid 1000) id B2DE220B6B; Wed, 11 Sep 2019 14:43:00 +0300 (EEST) Date: Wed, 11 Sep 2019 14:43:00 +0300 From: Sakari Ailus To: Tomasz Figa Cc: Dongchun Zhu , Mauro Carvalho Chehab , andriy.shevchenko@linux.intel.com, Rob Herring , Mark Rutland , Nicolas Boichat , Matthias Brugger , Cao Bing Bu , srv_heupstream , "moderated list:ARM/Mediatek SoC support" , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , Sj Huang , Linux Media Mailing List , devicetree@vger.kernel.org, Louis Kuo , shengnan.wang@mediatek.com Subject: Re: [V2, 2/2] media: i2c: Add more sensor modes for ov8856 camera sensor Message-ID: <20190911114300.GI5781@paasikivi.fi.intel.com> References: <20190910130446.26413-1-dongchun.zhu@mediatek.com> <20190910130446.26413-3-dongchun.zhu@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 wrote: > > > > From: Dongchun Zhu > > > > 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 > > --- > > 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@linux.intel.com