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=-7.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED 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 E9E05C43467 for ; Tue, 20 Oct 2020 13:00:44 +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 914CF22475 for ; Tue, 20 Oct 2020 13:00:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ojcAQHEA" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 914CF22475 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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: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=DZCE6IASy3iH3S2yrT3lsbWzunuqotsviPUIPTFQ7Uw=; b=ojcAQHEAua/LjbenKurb59Ohc cIphp3/3X5xX1PIK3l4IdwrROFe1HHnOxwRtkJRSSMwvrxZTRcGGsCAXAdKhiatWKP9x6yXm63OVu Gu75uCQZBkFF5AzR0S/p88OWcvoWg3D48FoPXbmSRGsHvIzgZ5poQJaZXIB+q84NzugIS9XLkdYD8 QHWT+OUuHPCFoFWyvQfk4+sRMIy7CFUzeRmg2/KmDMYeg9RRgIazKYTdWLuHyXNKkzHZTEV5l8/lm ynzFp06e6D3VbWw1Y4r7v7Mw4HPd3x9ZcU33BwyWOMNkoyAucoGNdXG4CSWWGwp0m1CzhJ6T6M+EV rGazu71TQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kUrEC-00072t-0M; Tue, 20 Oct 2020 12:59:00 +0000 Received: from mail-wm1-f66.google.com ([209.85.128.66]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kUrE9-000729-U8 for linux-arm-kernel@lists.infradead.org; Tue, 20 Oct 2020 12:58:58 +0000 Received: by mail-wm1-f66.google.com with SMTP id c16so1767153wmd.2 for ; Tue, 20 Oct 2020 05:58:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=g7yvvTboaHlvdbTqO2pCkQl4isMWIbbbVcGvTuQsmWM=; b=hVWZDi2PDqKGtmmdfOQjcmho3+IPBgdnGkwRw7KwpGNEWoTfnCsRqDnqiZ6/X3msj8 6037bLOS7nYi1kHwnmSXWUtnRVS8S//fSSccjghkJRQzPJccudF8XrUc0GnTzvjsdGfy aLC2+q46csuKNpd1CyK+cYKxhM+UfJUiQ1U4awIbpeprCOuJkV6GMMdsBWYVkylZPVXO ZT+3XDEVBRYF1NiQupGlzEKshs5JwQXy0OULrv3a8a63SXoqYwFMl9kDsQU8WwV3QpuT LhX7YdrwIYKR6jm58zOJK+TXDcelsTuoDvts1oN+kogjBGORqzdIugh6W2N3gLGo9oOm BCVQ== X-Gm-Message-State: AOAM533OqSxMeCDHZQGxLN2Vmq3YJLrKlMBM35FXE6gp5xyIxGDLWzIk FnJVdvwU1QdCF6ek8kBo/BI= X-Google-Smtp-Source: ABdhPJwYYXGPrNe0h5uGAOlzxwO/tvQ90MAO6A661TiJ2Ns4XUOEBdYCNvFTKWxhEJMdZv1HTzhTzA== X-Received: by 2002:a1c:2b05:: with SMTP id r5mr2767516wmr.179.1603198736809; Tue, 20 Oct 2020 05:58:56 -0700 (PDT) Received: from kozik-lap ([194.230.155.171]) by smtp.googlemail.com with ESMTPSA id u195sm2603572wmu.18.2020.10.20.05.58.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Oct 2020 05:58:55 -0700 (PDT) Date: Tue, 20 Oct 2020 14:58:52 +0200 From: Krzysztof Kozlowski To: Sakari Ailus Subject: Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Message-ID: <20201020125852.GA128323@kozik-lap> References: <20201019170247.92002-1-krzk@kernel.org> <20201020103833.GT13341@paasikivi.fi.intel.com> <20201020120058.GU13341@paasikivi.fi.intel.com> <20201020122621.GA126891@kozik-lap> <20201020124654.GX13341@paasikivi.fi.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201020124654.GX13341@paasikivi.fi.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201020_085857_973092_3ECA42BF X-CRM114-Status: GOOD ( 46.41 ) 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, Rob Herring , Stephen Boyd , Shawn Guo , Sascha Hauer , "linux-kernel@vger.kernel.org" , linux-clk@vger.kernel.org, Rob Herring , NXP Linux Team , Pengutronix Kernel Team , Mauro Carvalho Chehab , Fabio Estevam , Michael Turquette , 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 On Tue, Oct 20, 2020 at 03:46:54PM +0300, Sakari Ailus wrote: > On Tue, Oct 20, 2020 at 02:26:21PM +0200, Krzysztof Kozlowski wrote: > > On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote: > > > Hi Krzysztof, > > > > > > On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote: > > > > On Tue, 20 Oct 2020 at 12:38, Sakari Ailus wrote: > > > > > > > > > > Hi Krzysztof, > > > > > > > > > > On Mon, Oct 19, 2020 at 07:02:44PM +0200, Krzysztof Kozlowski wrote: > > > > > > Add bindings for the IMX258 camera sensor. The bindings, just like the > > > > > > driver, are quite limited, e.g. do not support regulator supplies. > > > > > > > > > > > > Signed-off-by: Krzysztof Kozlowski > > > > > > Reviewed-by: Rob Herring > > > > > > > > > > > > --- > > > > > > > > > > > > Changes since v4: > > > > > > 1. Add clock-lanes, > > > > > > 2. Add Rob's review, > > > > > > 3. Add one more example and extend existing one, > > > > > > 4. Add common clock properties (assigned-*). > > > > > > > > > > Using the assigned-* clock properties may be workable for this driver at > > > > > the moment. But using these properties does not guarantee the external > > > > > clock frequency intended to be used on the hardware. > > > > > > > > It guarantees it. The clock frequency will be as expected (except if > > > > someone misconfigures the DTS). > > > > > > Is that guaranteed? > > > > > > I'm not saying no to the approach, but if we change how camera sensor DT > > > bindings are defined, I'd prefer an informed decision is made on the > > > matter. > > > > > > > > > > > > Using other > > > > > frequencies *is not* expected to work. That applies to this driver as well. > > > > > > > > This is the binding which is HW description. According to HW datasheet > > > > other frequencies from described range are accepted and expected to > > > > work. > > > > > > As per datasheet, yes, different external clock frequencies can be used. > > > But the link frequency is still not independent of the external clock > > > frequency. > > > > > > The properties of the sensor's PLL tree determines what can be achieved > > > given a certain external clock frequency. So picking a wrong external clock > > > frequency quite possibly means that none of the designated link frequencies > > > are available, rendering the sensor inoperable. > > > > The driver then controls the HW and knows exactly what is needed. If > > link frequency (which has its own DT property) requires some clock > > frequency, the driver will configure the clock to that value. The same > > Well it doesn't if it doesn't get that information from DT. It will get it - via clk_get_rate(). You do not need DT for this. > The frequency is usually a range, and looking at these bindings, it's from > 6 MHz to 27 MHz. That'd be a lot of frequencies for a driver to try. It does not have to try all of them. Assuming link frequency is fixed, just use any matching (or hard-coded) input clock frequency. Since the input clock frequency most likely will be set with assigned-clock-rates, there will be no job to do for the driver at all. Unless the driver wants to do something more, of course. > > > going other direction. Driver has the knowledge about both its input > > clock and link frequency, therefore it can make the best decision. > > Again you're assuming a particular driver implementation. Actually not, I am talking about bindings as far away from the driver implementation as possible. This is why some specific frequency *is not* part of the bindings. > > Typically only a few frequencies are really available on platforms, so a in > practice a driver would not be able to get any requested frequency. I > wouldn't start hard-coding every possible frequency to camera sensor > drivers If the driver cannot get requested frequency which it apparently requires, there is nothing more to do. It's broken HW implementation. The input clock must be matching requirements, regardless of what property you put in DT. You can add "clock-frequency" property, you can even add "really-i-require-clock-frequency" but if the real HW input clock does not have, it won't work. IOW, adding "clock-frequency" property does not change the reality - the board (HW) must provide given frequency so the entire system works. > > > > > > This, instead of the clock-frequency property, effectively removes the > > > > > ability to set the correct frequency from the driver, at least with current > > > > > set of the used APIs. > > > > > > > > It seems you confuse DT bindings with some specific driver > > > > implementation. Bindings do not describe the driver behavior but the > > > > HW. The ability to set the correct frequency from the driver is not > > > > removed. It was never part of the bindings and never should. It is > > > > part of the driver. > > > > > > > > > > > > > > I suppose you could add a function to set the assigned clock frequency and > > > > > keep it, just as clk_set_rate_exclusive does? > > > > I did not reply to this comment, so let me know. Of course, one could > > add such functions. It's not a job for DT bindings, though. > > I'm not suggesting to add it to DT binding patch. What I'm saying that with > this approach is looks like it may well be needed. New properties can always be added to DT. However existing properties cannot be removed. Their meaning or values cannot be changed. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel