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=-12.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 AB63BC4361B for ; Thu, 10 Dec 2020 20:53:33 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 58D2123E21 for ; Thu, 10 Dec 2020 20:53:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 58D2123E21 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=seznam.cz Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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=merlin.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:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rIZc4LwsYemPgUEv0pgpfWaIo9oOXEvwNUkQP0F93Gw=; b=yjYfoaOSCtFve5gxJyYiMwdC/ iLGzTaYLiqcwFShs1SKGwOK+l0aHehHlYMbtJZ+n9vnDxh5Fft3leD9m9Bi0NQKiYR4s4yV1QvoNT 3g7cfm10G/OijwL+e13JbM8d4w+/qthx95GNTQvrdBfxcuC5kSWmBcLpeamQOIH4Jj9AO3WwjDVMc ebjfFbUOqIP7alQqUiCzOz4dXlI/pBvzCgYmE8KtAY39HGowf3XOdju5KCsdDvk8ZMkSu/x49qwlj BUhxuNcGbjBB9l7W0PoIZgUEaSmXAeUuVhOx1EnKfJ1MfF9c06U1DRqUPH9aMrNpl3QrqVnBM1sDp VnmM4mfgQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1knSuc-0006cX-7Q; Thu, 10 Dec 2020 20:51:42 +0000 Received: from mxf2.seznam.cz ([2a02:598:2::123]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1knSuY-0006br-3Q for linux-arm-kernel@lists.infradead.org; Thu, 10 Dec 2020 20:51:39 +0000 Received: from email.seznam.cz by email-smtpc25a.ng.seznam.cz (email-smtpc25a.ng.seznam.cz [10.23.18.34]) id 14be179e4fb043f71517dbc0; Thu, 10 Dec 2020 21:51:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seznam.cz; s=beta; t=1607633484; bh=uVI8DKBCKgq5GbOjTJGcesmmho5CB7T5xPqxJqxGoBE=; h=Received:Subject:To:Cc:References:From:Message-ID:Date:User-Agent: MIME-Version:In-Reply-To:Content-Type:Content-Transfer-Encoding: Content-Language; b=DOYrZsTuj6+Aw2wzWcKSqSn9uJkohGDyC9xOPjcz+C5aW4+WFNJOczbkqq7yO41ty z/KL1uT5kID1YIXNgJBcKw3DoQS/76hQt971mvjJvBdT2CoGTqus6RkqB9qTRVUlwv Qlb3UI9DdCTtMFsKQMk3lupp1vtDU05HpOrxs97M= Received: from [192.168.1.213] (ip-228-128.dynamic.ccinternet.cz [212.69.128.228]) by email-relay6.ng.seznam.cz (Seznam SMTPD 1.3.122) with ESMTP; Thu, 10 Dec 2020 21:51:22 +0100 (CET) Subject: Re: [PATCH v2 1/3] media: i2c: imx219: add support for specifying clock-frequencies To: Sascha Hauer References: <20201206172720.9406-1-michael.srba@seznam.cz> <20201207055952.GB14307@pengutronix.de> From: Michael Srba Message-ID: <222f5118-72ac-d291-f8d9-743d5c45c4ea@seznam.cz> Date: Thu, 10 Dec 2020 21:51:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <20201207055952.GB14307@pengutronix.de> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201210_155138_290869_812E93FE X-CRM114-Status: GOOD ( 33.64 ) 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: devicetree@vger.kernel.org, Geert Uytterhoeven , Dave Stevenson , Shawn Guo , Magnus Damm , linux-renesas-soc@vger.kernel.org, Rob Herring , NXP Linux Team , Pengutronix Kernel Team , Mauro Carvalho Chehab , Fabio Estevam , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, sorry for late reply. I copied this approach from looking at other camera sensor drivers, and it seemed less "ugly" to me than using assigned-rates (I will be upstreaming required dts changes for Samsung Galaxy A3 (2015), so the dts feeling "proper" is important to me). I however am not qualified to make that decision, so if you believe that the assigned-rates approach is cleaner and more suitable for mainline, I will try to adjust my internal filter for what is "more proper" :) As for rounding, the issue is that it seems to like to round up, instead of trying to find the closest possible value. I *guess* trying to set the lower barrier might work out in practice, but it seems kind of ugly. All in all, what I did seemed like the cleanest option to me, and it was an approach that other drivers also use. But if you believe there is a cleaner approach, I will be more than happy to do something else, though I would appreciate an explanation of why it is cleaner so that I can make better decisions in the future. Best regards, Michael On 07. 12. 20 6:59, Sascha Hauer wrote: > Hi Michael, > > On Sun, Dec 06, 2020 at 06:27:18PM +0100, michael.srba@seznam.cz wrote: >> From: Michael Srba >> >> This patch adds 1% tolerance on input clock, similar to other camera sensor >> drivers. It also allows for specifying the actual clock in the device tree, >> instead of relying on it being already set to the right frequency (which is >> often not the case). >> >> Signed-off-by: Michael Srba >> >> --- >> >> changes since v1: default to exactly 24MHz when `clock-frequency` is not present >> >> --- >> drivers/media/i2c/imx219.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c >> index f64c0ef7a897..b6500e2ab19e 100644 >> --- a/drivers/media/i2c/imx219.c >> +++ b/drivers/media/i2c/imx219.c >> @@ -1443,13 +1443,28 @@ static int imx219_probe(struct i2c_client *client) >> return PTR_ERR(imx219->xclk); >> } >> >> - imx219->xclk_freq = clk_get_rate(imx219->xclk); >> - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { >> + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq); >> + if (ret) { >> + dev_warn(dev, "could not get xclk frequency\n"); >> + >> + /* default to 24MHz */ >> + imx219->xclk_freq = 24000000; >> + } >> + >> + /* this driver currently expects 24MHz; allow 1% tolerance */ >> + if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) { >> dev_err(dev, "xclk frequency not supported: %d Hz\n", >> imx219->xclk_freq); >> return -EINVAL; >> } >> >> + ret = clk_set_rate(imx219->xclk, imx219->xclk_freq); >> + if (ret) { >> + dev_err(dev, "could not set xclk frequency\n"); >> + return ret; >> + } > clk_set_rate() returns successfully when the rate change has succeeded. > It tells you nothing about the actual rate that has been set. The rate > could be very different from what you want to get, depending on what the > hardware is able to archieve. There's clk_round_rate() that tells you > which rate you'll get when you call clk_set_rate() with that value. > You would have to call clk_round_rate() first and see if you are happy > with the result, afterwards set the rate. From that view it doesn't make > much sense to check the device tree if a number between 23760000 and > 24240000 is specified there, the clk api will do rounding anyway. > > Also there's the assigned-clocks device tree binding, see > Documentation/devicetree/bindings/clock/clock-bindings.txt. This allows > you to set the desired clock rate directly in the device tree. All > that's left to do in the driver is to replace the check for the exact > rate with a check which allows a certain tolerance. > > Sascha > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel