LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrey Danin <danindrey@mail.ru>
To: Stephen Warren <swarren@wwwdotorg.org>,
	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 <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	Russell King <linux@arm.linux.org.uk>,
	Thierry Reding <thierry.reding@gmail.com>,
	Alexandre Courbot <gnurou@gmail.com>,
	Marc Dietrich <marvin24@gmx.de>
Subject: Re: [PATCH 3/3] dt: paz00: define nvec as child of i2c bus
Date: Tue, 31 Mar 2015 09:40:27 +0300	[thread overview]
Message-ID: <551A415B.4040506@mail.ru> (raw)
In-Reply-To: <54CFEA2F.8040701@wwwdotorg.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 = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
>> -        #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.
>
>> +        };
>>       };
>


  reply	other threads:[~2015-03-31  6:40 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-29  7:20 [PATCH 0/3] arm: tegra: implement NVEC driver using tegra i2c Andrey Danin
2015-01-29  7:20 ` [PATCH 1/3] i2c: tegra: implement slave mode Andrey Danin
2015-01-29  9:40   ` Marc Dietrich
2015-01-29 11:41   ` Wolfram Sang
2015-03-31  6:25     ` Andrey Danin
2015-01-29  7:20 ` [PATCH 2/3] staging/nvec: reimplement on top of tegra i2c driver Andrey Danin
2015-01-29  9:53   ` Marc Dietrich
2015-01-29  7:20 ` [PATCH 3/3] dt: paz00: define nvec as child of i2c bus Andrey Danin
2015-01-29 10:01   ` Marc Dietrich
2015-02-02 21:20   ` Stephen Warren
2015-03-31  6:40     ` Andrey Danin [this message]
2015-03-31 14:09       ` Stephen Warren
2015-03-31 15:46         ` Andrey Danin
2015-03-31 16:04           ` Andrey Danin
2015-04-01 17:28           ` Stephen Warren
2015-04-02  9:37             ` Marc Dietrich
2015-04-02 14:50               ` Stephen Warren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=551A415B.4040506@mail.ru \
    --to=danindrey@mail.ru \
    --cc=ac100@lists.launchpad.net \
    --cc=devicetree@vger.kernel.org \
    --cc=galak@codeaurora.org \
    --cc=gnurou@gmail.com \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=marvin24@gmx.de \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=swarren@wwwdotorg.org \
    --cc=thierry.reding@gmail.com \
    --subject='Re: [PATCH 3/3] dt: paz00: define nvec as child of i2c bus' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).