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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,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 9693EC433E0 for ; Thu, 11 Jun 2020 10:03:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7AAE9207C3 for ; Thu, 11 Jun 2020 10:03:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727784AbgFKKDb (ORCPT ); Thu, 11 Jun 2020 06:03:31 -0400 Received: from mga03.intel.com ([134.134.136.65]:9863 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727782AbgFKKDb (ORCPT ); Thu, 11 Jun 2020 06:03:31 -0400 IronPort-SDR: L5cfRmxTVdj2l411y4r9KWlyl33VYHq4ccL6pO/tSS8VdibyLKErtWAIMUoSn1WO+OiwuGY0LO 1tCc/qYHxfrQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:30 -0700 IronPort-SDR: Pn15RnbO6jIDhtSwfC1dpWA2jynSfJXqvEvOYQg42lVNOehGHk53YpEDtcsPI9Se0S/k2dok6n ZpkT0LhhwrMw== X-IronPort-AV: E=Sophos;i="5.73,499,1583222400"; d="scan'208";a="260660099" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:25 -0700 Received: by paasikivi.fi.intel.com (Postfix, from userid 1000) id A935D20680; Thu, 11 Jun 2020 13:03:22 +0300 (EEST) Date: Thu, 11 Jun 2020 13:03:22 +0300 From: Sakari Ailus To: Tomasz Figa Cc: Dongchun Zhu , Linus Walleij , Bartosz Golaszewski , Mauro Carvalho Chehab , Andy Shevchenko , 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 , linux-devicetree , Louis Kuo , Shengnan Wang =?utf-8?B?KOeOi+Wco+eUtyk=?= Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Message-ID: <20200611100322.GL16711@paasikivi.fi.intel.com> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> <20200611095333.GK16711@paasikivi.fi.intel.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: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > +}; > > > [snip] > > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > which is used for trying the format, but not applying it to the hardware? > > > > Yes. > > > > > > > > > + if (ov02a10->streaming) { > > > > + mutex_unlock(&ov02a10->mutex); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + /* Only one sensor mode supported */ > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + ov02a10->fmt = fmt->format; > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > + fmt->format = ov02a10->fmt; > > > > > > Ditto. > > > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + > > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > > + return -EINVAL; > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > codes. Should this just perhaps be "!= 0"? > > > > > > > + > > > > + code->code = ov02a10->fmt.code; > > > > + > > > > + return 0; > > > > +} > > > [snip] > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg) > > > > +{ > > > > + struct v4l2_subdev_format fmt = { > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > + .format = { > > > > + .width = 1600, > > > > + .height = 1200, > > > > + } > > > > + }; > > > > + > > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > sparse about it. Sakari, is this a correct implementation? > > > > The purpose is to initialise a pad configuration (format and selection > > rectangles) to the device defaults. As there seem to be no selection > > rectangles, this seems fine to me. > > Thanks. I traced the code and could only see one place where the > callback is being called and that was with cfg != NULL. Still, the > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > should be passed to which. Is that also correct? It could be used in setting the active format in probe. That would probably be cleaner than what it currently does. But apart from that, the framework always calls init_cfg with cfg non-NULL. -- Sakari Ailus 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=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 4828BC433E0 for ; Thu, 11 Jun 2020 10:03:55 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1396120835 for ; Thu, 11 Jun 2020 10:03:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dLnfdupu" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1396120835 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7ZREs/xa6YJn83PLxFlEW/KKVfR+DbDamRwAGeKhUao=; b=dLnfdupu5bXD6g 0RoAIUha9DPlNMTLmld1WWeqp7l7ftKrTlPvZSu7ubHmfSaVCusq4Ke9XkVqm+QGt9JUvYw6wkHZj ZkV+5y0xdBBBAz9+A6WCMyYLfUBPYFdf10QvgIf9/u34celA24qVPpd8Nk4coFzcRu1lRXdKEC06M Wvemt/1pLnMULoCPvXL36qvNypNldVG/OlZKWcJibi3kltTS28ZyhfkodQLoRgp2DYQc5040FS6x9 KoNzn1D/fHt0jOsOjhkw59f9AzJJbH1M/EtVyvgXHdsyfih0Or9zCVciqUwaqoUbYjqngIQcAvL7X eIaHIeWo1kkR0dNchAmA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjK3o-0006V9-08; Thu, 11 Jun 2020 10:03:48 +0000 Received: from mga01.intel.com ([192.55.52.88]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjK3W-0005z1-MF; Thu, 11 Jun 2020 10:03:32 +0000 IronPort-SDR: 3fKCufSo77Nx+gIBTAjFWRkf74PwFdKHG7tTFf8SL72rFlbFbjrY+z8VJQr1ZbHXMSja8J5ovy jx2lbpXXFbOQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:30 -0700 IronPort-SDR: Pn15RnbO6jIDhtSwfC1dpWA2jynSfJXqvEvOYQg42lVNOehGHk53YpEDtcsPI9Se0S/k2dok6n ZpkT0LhhwrMw== X-IronPort-AV: E=Sophos;i="5.73,499,1583222400"; d="scan'208";a="260660099" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:25 -0700 Received: by paasikivi.fi.intel.com (Postfix, from userid 1000) id A935D20680; Thu, 11 Jun 2020 13:03:22 +0300 (EEST) Date: Thu, 11 Jun 2020 13:03:22 +0300 From: Sakari Ailus To: Tomasz Figa Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Message-ID: <20200611100322.GL16711@paasikivi.fi.intel.com> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> <20200611095333.GK16711@paasikivi.fi.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200611_030330_738373_946068A7 X-CRM114-Status: GOOD ( 29.87 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Nicolas Boichat , Andy Shevchenko , srv_heupstream , linux-devicetree , Linus Walleij , Shengnan Wang =?utf-8?B?KOeOi+Wco+eUtyk=?= , Bartosz Golaszewski , Sj Huang , Rob Herring , "moderated list:ARM/Mediatek SoC support" , Dongchun Zhu , Louis Kuo , Matthias Brugger , Cao Bing Bu , Mauro Carvalho Chehab , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > +}; > > > [snip] > > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > which is used for trying the format, but not applying it to the hardware? > > > > Yes. > > > > > > > > > + if (ov02a10->streaming) { > > > > + mutex_unlock(&ov02a10->mutex); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + /* Only one sensor mode supported */ > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + ov02a10->fmt = fmt->format; > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > + fmt->format = ov02a10->fmt; > > > > > > Ditto. > > > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + > > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > > + return -EINVAL; > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > codes. Should this just perhaps be "!= 0"? > > > > > > > + > > > > + code->code = ov02a10->fmt.code; > > > > + > > > > + return 0; > > > > +} > > > [snip] > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg) > > > > +{ > > > > + struct v4l2_subdev_format fmt = { > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > + .format = { > > > > + .width = 1600, > > > > + .height = 1200, > > > > + } > > > > + }; > > > > + > > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > sparse about it. Sakari, is this a correct implementation? > > > > The purpose is to initialise a pad configuration (format and selection > > rectangles) to the device defaults. As there seem to be no selection > > rectangles, this seems fine to me. > > Thanks. I traced the code and could only see one place where the > callback is being called and that was with cfg != NULL. Still, the > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > should be passed to which. Is that also correct? It could be used in setting the active format in probe. That would probably be cleaner than what it currently does. But apart from that, the framework always calls init_cfg with cfg non-NULL. -- Sakari Ailus _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek 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=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 9AA9CC433DF for ; Thu, 11 Jun 2020 10:03:41 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3073F20835 for ; Thu, 11 Jun 2020 10:03:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="HgBCxkXX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3073F20835 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Io7M37SYbGHvhGCDs8TkbeaaGAWL1BJwlgqmCaDd5xI=; b=HgBCxkXXsKLhfJ +eSPuEa3sNFe+9WGa9jMFuaIhvINbIjsMi6XBub1+Vw5puie1uXz3XdMJOUCj1Mfu18zeKk73v1C3 yv1gAGcPSPaDD4pLQwE5jzYcFG7himUy2TKls/0S/560FnrGWn5A/KTKdFpFiy26o5qN9PSVBGHLL SHigX4nQRFWr/H2//X8hCmCZFI2ciZh6Ba5dFpaiGGPN/4//GvJys0SI0XOMjvPK6CxHmaz02aNRR simPulpOjR0JLjyMNJLrBi56bWCpesoohHQojAJ0Y+U4qEgtj/vDcIkT5Kg3Q9p24NN1MIB7G3Atb GKPtv5n0nrFd34bav5yg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjK3Z-000636-S0; Thu, 11 Jun 2020 10:03:33 +0000 Received: from mga01.intel.com ([192.55.52.88]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjK3W-0005z1-MF; Thu, 11 Jun 2020 10:03:32 +0000 IronPort-SDR: 3fKCufSo77Nx+gIBTAjFWRkf74PwFdKHG7tTFf8SL72rFlbFbjrY+z8VJQr1ZbHXMSja8J5ovy jx2lbpXXFbOQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:30 -0700 IronPort-SDR: Pn15RnbO6jIDhtSwfC1dpWA2jynSfJXqvEvOYQg42lVNOehGHk53YpEDtcsPI9Se0S/k2dok6n ZpkT0LhhwrMw== X-IronPort-AV: E=Sophos;i="5.73,499,1583222400"; d="scan'208";a="260660099" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 03:03:25 -0700 Received: by paasikivi.fi.intel.com (Postfix, from userid 1000) id A935D20680; Thu, 11 Jun 2020 13:03:22 +0300 (EEST) Date: Thu, 11 Jun 2020 13:03:22 +0300 From: Sakari Ailus To: Tomasz Figa Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Message-ID: <20200611100322.GL16711@paasikivi.fi.intel.com> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> <20200611095333.GK16711@paasikivi.fi.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200611_030330_738373_946068A7 X-CRM114-Status: GOOD ( 29.87 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Nicolas Boichat , Andy Shevchenko , srv_heupstream , linux-devicetree , Linus Walleij , Shengnan Wang =?utf-8?B?KOeOi+Wco+eUtyk=?= , Bartosz Golaszewski , Sj Huang , Rob Herring , "moderated list:ARM/Mediatek SoC support" , Dongchun Zhu , Louis Kuo , Matthias Brugger , Cao Bing Bu , Mauro Carvalho Chehab , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > +}; > > > [snip] > > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > which is used for trying the format, but not applying it to the hardware? > > > > Yes. > > > > > > > > > + if (ov02a10->streaming) { > > > > + mutex_unlock(&ov02a10->mutex); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + /* Only one sensor mode supported */ > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + ov02a10->fmt = fmt->format; > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > + fmt->format = ov02a10->fmt; > > > > > > Ditto. > > > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + > > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > > + return -EINVAL; > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > codes. Should this just perhaps be "!= 0"? > > > > > > > + > > > > + code->code = ov02a10->fmt.code; > > > > + > > > > + return 0; > > > > +} > > > [snip] > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg) > > > > +{ > > > > + struct v4l2_subdev_format fmt = { > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > + .format = { > > > > + .width = 1600, > > > > + .height = 1200, > > > > + } > > > > + }; > > > > + > > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > sparse about it. Sakari, is this a correct implementation? > > > > The purpose is to initialise a pad configuration (format and selection > > rectangles) to the device defaults. As there seem to be no selection > > rectangles, this seems fine to me. > > Thanks. I traced the code and could only see one place where the > callback is being called and that was with cfg != NULL. Still, the > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > should be passed to which. Is that also correct? It could be used in setting the active format in probe. That would probably be cleaner than what it currently does. But apart from that, the framework always calls init_cfg with cfg non-NULL. -- Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel