From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver Date: Mon, 26 Aug 2019 15:54:25 +0900 Message-ID: References: <20190819034331.13098-1-dongchun.zhu@mediatek.com> <20190819034331.13098-3-dongchun.zhu@mediatek.com> <20190821103038.GA148543@chromium.org> <20190821110542.GD31967@paasikivi.fi.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190821110542.GD31967-z7MJbOB4PBP+e+fPlCVrcFDQ4js95KgL@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Sakari Ailus Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Boichat , srv_heupstream , shengnan.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Louis Kuo , Sj Huang , Rob Herring , "moderated list:ARM/Mediatek SoC support" , dongchun.zhu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Matthias Brugger , Cao Bing Bu , Mauro Carvalho Chehab , "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS , Joerg Roedel , " , Linux Media Mailing List List-Id: devicetree@vger.kernel.org On Wed, Aug 21, 2019 at 8:05 PM Sakari Ailus wrote: > > Hi Tomasz, > > On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote: [snip] > > Is it really correct to enable the clock before the regulators? > > > > According to the datasheet, it should be: > > - PD pin HIGH, > > - nRST pin LOW, > > - DVDDIO and AVDD28 power up and stabilize, > > - clock enabled, > > - min 5 ms delay, > > - PD pin LOW, > > - min 4 ms delay, > > - nRST pin HIGH, > > - min 5 ms delay, > > - I2C interface ready. > > > > > + > > > + /* Note: set 0 is high, set 1 is low */ > > > > Why is that? If there is some inverter on the way that should be handled > > outside of this driver. (GPIO DT bindings have flags for this purpose. > > > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > > call them like this in the driver too (+/- the lowercase and underscore > > convention). > > > > According to the datasheet, the reset pin is called RST and inverted, so we should > > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > > would be the right name. > > For what it's worth sensors generally have xshutdown (or reset) pin that is > active high. Looking at the code, it is not the case here. It's a bit odd > since the usual arrangement saves power when the camera is not in use; it's > not a lot but still. Oh well. > I guess we could drive powerdown low after disabling the regulators and clocks, but that wouldn't work for the cases where the regulators are actually shared with something else, especially if that is not related to the same camera module. > ... > > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > > Please use of_match_ptr() wrapper. > > Not really needed; the driver does expect regulators, GPIOs etc., but by > leaving out of_match_ptr(), the driver will also probe on ACPI based > systems. Good point, I always keep forgetting about the ability to probe OF drivers from ACPI. Then we also need to remove the #if IS_ENABLED(CONFIG_OF) from ov02a10_of_match. Best regards, Tomasz 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=-0.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 B76BAC3A59E for ; Mon, 26 Aug 2019 07:00:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8A62322CF5 for ; Mon, 26 Aug 2019 07:00:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="WRcwatvA" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729847AbfHZHAZ (ORCPT ); Mon, 26 Aug 2019 03:00:25 -0400 Received: from mail-ed1-f68.google.com ([209.85.208.68]:33680 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729437AbfHZHAZ (ORCPT ); Mon, 26 Aug 2019 03:00:25 -0400 Received: by mail-ed1-f68.google.com with SMTP id s15so25026676edx.0 for ; Mon, 26 Aug 2019 00:00:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nTkNb4qhu4LWrFg09llWi3oBh88pK95w54xsWrsdjT0=; b=WRcwatvAI0WEvdFUv/sT0oyCxKW9JAWDmzw43+hawcVPvkP4MC+sOxq613oe7+u4MD bjj8wG02Jk6cwVwZCt36tlQmALBdiNQObpjmpl0NVmcWhLPGKFfJioYsChUsQ2SvXU+Z +1RwYI0ZvOyR/CpVy0n1vVMkrI8RqOW2IPN3E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nTkNb4qhu4LWrFg09llWi3oBh88pK95w54xsWrsdjT0=; b=fHDex3u1dFNtcSifPoEUqR5uMRuQzM1PGOa7lwpos1nrJEmIeFQALVAPyc1j+ykcQC KGZ2zSRihq4HCvpqB+vZPrQRNtczUbJ3OQuIo/jdOyoWbxI6WWeE+VnWFxHu0H3Op619 pg8ZrZ96Riwy2xJ3NcdRPi7u0no3bRJiF1I6o0Z4U2OSDHu1XFN75AFWA84uU7ixbm8h yKLMMR0D/pYzontsszo6p9cIcPZMQ0Q/QexbvSVfNHZhFLY0JmQsBxVV76HFRG8pNoUa QcvkxrKv3CQhPTjdZBjKE4PKBU4VBytKVQni8Od0/r5X60YfRpFAdsGCW/gCEd6IxY9M TZoA== X-Gm-Message-State: APjAAAUvyo4TSDGFFR5vYfarDcyAb+a+BTrlrdOg9s9Ak/TkAT4pRku2 wJUxdCta3QLL2G9aiIuKh1P1sTTX7Rjb8g== X-Google-Smtp-Source: APXvYqz3TNS37+1cl+fXdnaoMAOwxFxemQQpQC5HGPGhkHDM1VJ1RJbt2/gGoSb1/9VW2Y8adYKoog== X-Received: by 2002:a05:6402:170f:: with SMTP id y15mr17651378edu.55.1566802824037; Mon, 26 Aug 2019 00:00:24 -0700 (PDT) Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com. [209.85.221.49]) by smtp.gmail.com with ESMTPSA id c1sm1097105edn.62.2019.08.26.00.00.23 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 26 Aug 2019 00:00:23 -0700 (PDT) Received: by mail-wr1-f49.google.com with SMTP id g17so14197948wrr.5 for ; Mon, 26 Aug 2019 00:00:23 -0700 (PDT) X-Received: by 2002:adf:fc03:: with SMTP id i3mr19059644wrr.48.1566802476916; Sun, 25 Aug 2019 23:54:36 -0700 (PDT) MIME-Version: 1.0 References: <20190819034331.13098-1-dongchun.zhu@mediatek.com> <20190819034331.13098-3-dongchun.zhu@mediatek.com> <20190821103038.GA148543@chromium.org> <20190821110542.GD31967@paasikivi.fi.intel.com> In-Reply-To: <20190821110542.GD31967@paasikivi.fi.intel.com> From: Tomasz Figa Date: Mon, 26 Aug 2019 15:54:25 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver To: Sakari Ailus Cc: dongchun.zhu@mediatek.com, Mauro Carvalho Chehab , 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Wed, Aug 21, 2019 at 8:05 PM Sakari Ailus wrote: > > Hi Tomasz, > > On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote: [snip] > > Is it really correct to enable the clock before the regulators? > > > > According to the datasheet, it should be: > > - PD pin HIGH, > > - nRST pin LOW, > > - DVDDIO and AVDD28 power up and stabilize, > > - clock enabled, > > - min 5 ms delay, > > - PD pin LOW, > > - min 4 ms delay, > > - nRST pin HIGH, > > - min 5 ms delay, > > - I2C interface ready. > > > > > + > > > + /* Note: set 0 is high, set 1 is low */ > > > > Why is that? If there is some inverter on the way that should be handled > > outside of this driver. (GPIO DT bindings have flags for this purpose. > > > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > > call them like this in the driver too (+/- the lowercase and underscore > > convention). > > > > According to the datasheet, the reset pin is called RST and inverted, so we should > > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > > would be the right name. > > For what it's worth sensors generally have xshutdown (or reset) pin that is > active high. Looking at the code, it is not the case here. It's a bit odd > since the usual arrangement saves power when the camera is not in use; it's > not a lot but still. Oh well. > I guess we could drive powerdown low after disabling the regulators and clocks, but that wouldn't work for the cases where the regulators are actually shared with something else, especially if that is not related to the same camera module. > ... > > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > > Please use of_match_ptr() wrapper. > > Not really needed; the driver does expect regulators, GPIOs etc., but by > leaving out of_match_ptr(), the driver will also probe on ACPI based > systems. Good point, I always keep forgetting about the ability to probe OF drivers from ACPI. Then we also need to remove the #if IS_ENABLED(CONFIG_OF) from ov02a10_of_match. Best regards, Tomasz 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=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 DF798C3A59E for ; Mon, 26 Aug 2019 07:02:02 +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 AD13A217F4 for ; Mon, 26 Aug 2019 07:02:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="gxqyTOnz"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="WRcwatvA" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AD13A217F4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org 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:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=IQ71d4WpbUjmkfDGcooGNvsSSMdscr+Gz1+Klw3zZGo=; b=gxqyTOnzDeeTmb IAfNY1qnKobmstrP5bvjDhtimeEdsqGRdPAEoX1FlDgor8lfzaiYGNxFy++ebUfxGZ7PDZoSmTb/x XetQyRftxDU7T1eYAhMtOFfimFn6j1qIADAmw3y7PCORO6BosiyNfhHV9AJ6W9me7Gi35eMlHfEap fHMPnRvVAcBk0NBV4d3Y/0WrJiC/F+icfNlMMGR/XYdS30OHoFhQMsKsoWQ8HYNWeQru+hsXeRDMJ 4CWfJWf/UHly6XVLLvfIW7eue8WfXmTYdt241d5BQn4VLTjfLk+DcKq8zes3LU41B5U0X6Zu2SloQ yA3KaF4lvjy61dl3bfdw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1i290m-0008Bo-9O; Mon, 26 Aug 2019 07:01:56 +0000 Received: from mail-ed1-x542.google.com ([2a00:1450:4864:20::542]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1i290f-0008BR-I3 for linux-arm-kernel@lists.infradead.org; Mon, 26 Aug 2019 07:01:52 +0000 Received: by mail-ed1-x542.google.com with SMTP id a21so24985978edt.11 for ; Mon, 26 Aug 2019 00:01:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nTkNb4qhu4LWrFg09llWi3oBh88pK95w54xsWrsdjT0=; b=WRcwatvAI0WEvdFUv/sT0oyCxKW9JAWDmzw43+hawcVPvkP4MC+sOxq613oe7+u4MD bjj8wG02Jk6cwVwZCt36tlQmALBdiNQObpjmpl0NVmcWhLPGKFfJioYsChUsQ2SvXU+Z +1RwYI0ZvOyR/CpVy0n1vVMkrI8RqOW2IPN3E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nTkNb4qhu4LWrFg09llWi3oBh88pK95w54xsWrsdjT0=; b=dj7yuqQ8ZIJcIifwEFZ8agN6e9itVW7q69RMuzc3Pzb3AzCdItIyv6jXb/W22K8sDw yWwownHqXUu/WNmIiWE6/gbb+/aMhDCQ9fOKT7N/7fRGNJU5oBtcYwywOrl6QNWEvUot Jh7MdMZkEaFQBjDh2uBhI5LVBTCcfLFulsl2jV2Xb0zaPTaLyF+Sx4jyhjq8MHM6uAUq 7OsRsxuS7OJvRvrz/3BSIqpSh43q8eYB87DkIpJepuyMU2hNtZPxCze2QtZiKF1+WwZC zug7votv0IdXLqYVIPZfJaMrPy2Y+cSUevQ2jwEUY6Gt8RWkxUQJRy2mRygXd0mpFoZx OSxA== X-Gm-Message-State: APjAAAWAN/l6QxO+taGW5nmAY/SRjX5npCB+5F5RZwc7FgAdNlCCkB3I 4o4BTKb1NKuRcwk8/7gpXvMakO6KMMsKpQ== X-Google-Smtp-Source: APXvYqxIU6SC1OH88bVgXKxOtxH/cn2cUOwPFSOQyX8TLWHUMCkh8dY1G0T7BiVzsJB85iwgBQ5srw== X-Received: by 2002:aa7:d30a:: with SMTP id p10mr16718192edq.79.1566802907693; Mon, 26 Aug 2019 00:01:47 -0700 (PDT) Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com. [209.85.221.54]) by smtp.gmail.com with ESMTPSA id 59sm1114528edg.44.2019.08.26.00.01.47 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 26 Aug 2019 00:01:47 -0700 (PDT) Received: by mail-wr1-f54.google.com with SMTP id k2so14191272wrq.2 for ; Mon, 26 Aug 2019 00:01:47 -0700 (PDT) X-Received: by 2002:adf:fc03:: with SMTP id i3mr19059644wrr.48.1566802476916; Sun, 25 Aug 2019 23:54:36 -0700 (PDT) MIME-Version: 1.0 References: <20190819034331.13098-1-dongchun.zhu@mediatek.com> <20190819034331.13098-3-dongchun.zhu@mediatek.com> <20190821103038.GA148543@chromium.org> <20190821110542.GD31967@paasikivi.fi.intel.com> In-Reply-To: <20190821110542.GD31967@paasikivi.fi.intel.com> From: Tomasz Figa Date: Mon, 26 Aug 2019 15:54:25 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver To: Sakari Ailus X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190826_000149_975015_16C2C35A X-CRM114-Status: GOOD ( 24.84 ) 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 , devicetree@vger.kernel.org, Nicolas Boichat , srv_heupstream , shengnan.wang@mediatek.com, Louis Kuo , Sj Huang , Rob Herring , "moderated list:ARM/Mediatek SoC support" , dongchun.zhu@mediatek.com, 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 Wed, Aug 21, 2019 at 8:05 PM Sakari Ailus wrote: > > Hi Tomasz, > > On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote: [snip] > > Is it really correct to enable the clock before the regulators? > > > > According to the datasheet, it should be: > > - PD pin HIGH, > > - nRST pin LOW, > > - DVDDIO and AVDD28 power up and stabilize, > > - clock enabled, > > - min 5 ms delay, > > - PD pin LOW, > > - min 4 ms delay, > > - nRST pin HIGH, > > - min 5 ms delay, > > - I2C interface ready. > > > > > + > > > + /* Note: set 0 is high, set 1 is low */ > > > > Why is that? If there is some inverter on the way that should be handled > > outside of this driver. (GPIO DT bindings have flags for this purpose. > > > > If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should > > call them like this in the driver too (+/- the lowercase and underscore > > convention). > > > > According to the datasheet, the reset pin is called RST and inverted, so we should > > call it n_rst, but the powerdown signal, called PD, is not inverted, so pd > > would be the right name. > > For what it's worth sensors generally have xshutdown (or reset) pin that is > active high. Looking at the code, it is not the case here. It's a bit odd > since the usual arrangement saves power when the camera is not in use; it's > not a lot but still. Oh well. > I guess we could drive powerdown low after disabling the regulators and clocks, but that wouldn't work for the cases where the regulators are actually shared with something else, especially if that is not related to the same camera module. > ... > > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > > Please use of_match_ptr() wrapper. > > Not really needed; the driver does expect regulators, GPIOs etc., but by > leaving out of_match_ptr(), the driver will also probe on ACPI based > systems. Good point, I always keep forgetting about the ability to probe OF drivers from ACPI. Then we also need to remove the #if IS_ENABLED(CONFIG_OF) from ov02a10_of_match. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel