LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Warren <swarren@wwwdotorg.org>
To: Andrey Danin <danindrey@mail.ru>,
	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: Mon, 02 Feb 2015 14:20:47 -0700	[thread overview]
Message-ID: <54CFEA2F.8040701@wwwdotorg.org> (raw)
In-Reply-To: <1422516022-27161-4-git-send-email-danindrey@mail.ru>

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.

> 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.

> +			compatible = "nvidia,nvec";
> +			request-gpios = <&gpio TEGRA_GPIO(V, 2)
> +				GPIO_ACTIVE_HIGH>;
> +			reg = <0x45>;

The order is typically compatible, reg, other properties.

> +		};
>   	};


  parent reply	other threads:[~2015-02-02 21:20 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 [this message]
2015-03-31  6:40     ` Andrey Danin
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=54CFEA2F.8040701@wwwdotorg.org \
    --to=swarren@wwwdotorg.org \
    --cc=ac100@lists.launchpad.net \
    --cc=danindrey@mail.ru \
    --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=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).