From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752428AbbCaGko (ORCPT ); Tue, 31 Mar 2015 02:40:44 -0400 Received: from smtp40.i.mail.ru ([94.100.177.100]:41575 "EHLO smtp40.i.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751125AbbCaGkl (ORCPT ); Tue, 31 Mar 2015 02:40:41 -0400 X-Greylist: delayed 38374 seconds by postgrey-1.27 at vger.kernel.org; Tue, 31 Mar 2015 02:40:40 EDT Message-ID: <551A415B.4040506@mail.ru> Date: Tue, 31 Mar 2015 09:40:27 +0300 From: Andrey Danin User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Stephen Warren , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, ac100@lists.launchpad.net CC: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Thierry Reding , Alexandre Courbot , Marc Dietrich Subject: Re: [PATCH 3/3] dt: paz00: define nvec as child of i2c bus References: <1422516022-27161-1-git-send-email-danindrey@mail.ru> <1422516022-27161-4-git-send-email-danindrey@mail.ru> <54CFEA2F.8040701@wwwdotorg.org> In-Reply-To: <54CFEA2F.8040701@wwwdotorg.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Spam: Not detected X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Thanks for the review. On 03.02.2015 0:20, Stephen Warren wrote: > On 01/29/2015 12:20 AM, Andrey Danin wrote: >> NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings >> for NVEC node. > >> diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt >> b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt > > The changes to this file make more sense either as a standalone patch > 1/4, or as part of the driver changes. > >> @@ -2,20 +2,5 @@ NVIDIA compliant embedded controller >> >> Required properties: >> - compatible : should be "nvidia,nvec". >> -- reg : the iomem of the i2c slave controller >> -- interrupts : the interrupt line of the i2c slave controller >> -- clock-frequency : the frequency of the i2c bus >> -- gpios : the gpio used for ec request >> -- slave-addr: the i2c address of the slave controller >> -- clocks : Must contain an entry for each entry in clock-names. >> - See ../clocks/clock-bindings.txt for details. >> -- clock-names : Must include the following entries: >> - Tegra20/Tegra30: >> - - div-clk >> - - fast-clk >> - Tegra114: >> - - div-clk >> -- resets : Must contain an entry for each entry in reset-names. >> - See ../reset/reset.txt for details. >> -- reset-names : Must include the following entries: >> - - i2c >> +- request-gpios : the gpio used for ec request >> +- reg: the i2c address of the slave controller > > This change breaks ABI. > > Instead of modifying the definition of the existing compatible value, I > think you should introduce a new compatible value to describe the > external NVEC chip. I changed compatible value to nvec-slave in v2. > >> diff --git a/arch/arm/boot/dts/tegra20-paz00.dts >> b/arch/arm/boot/dts/tegra20-paz00.dts > >> - nvec@7000c500 { >> - compatible = "nvidia,nvec"; >> - reg = <0x7000c500 0x100>; >> - interrupts = ; >> - #address-cells = <1>; >> - #size-cells = <0>; >> + i2c@7000c500 { >> + status = "okay"; >> clock-frequency = <80000>; >> - request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>; >> - slave-addr = <138>; >> - clocks = <&tegra_car TEGRA20_CLK_I2C3>, >> - <&tegra_car TEGRA20_CLK_PLL_P_OUT3>; >> - clock-names = "div-clk", "fast-clk"; >> - resets = <&tegra_car 67>; >> - reset-names = "i2c"; >> + >> + nvec: nvec@45 { > > This doesn't feel correct. There's nothing here to indicate that this > child device is a slave that is implemented by the host SoC rather than > something external attached to the I2C bus. > > Perhaps you can get away with this, since the driver for nvidia,nvec > only calls I2C APIs suitable for internal slaves rather than external > slaves? Even so though, I think the distinction needs to be clearly > marked in the DT so that any generic code outside the NVEC driver that > parses the DT can determine the difference. > > I would recommend the I2C controller having #address-cells=<2> with cell > 0 being 0==master,1==slave, cell 1 being the I2C address. The I2C driver > would need to support #address-cells=<1> for backwards-compatibility. > Driver (nvec in this case) can decide what mode should it use according to compatible value. Is it not enough ? >> + compatible = "nvidia,nvec"; >> + request-gpios = <&gpio TEGRA_GPIO(V, 2) >> + GPIO_ACTIVE_HIGH>; >> + reg = <0x45>; > > The order is typically compatible, reg, other properties. Ok, thanks. > >> + }; >> }; >