| Message ID | 1422516022-27161-4-git-send-email-danindrey@mail.ru |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
Am Donnerstag, 29. Januar 2015, 10:20:22 schrieb Andrey Danin: > NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings > for NVEC node. > > Signed-off-by: Andrey Danin <danindrey@mail.ru> > --- > .../devicetree/bindings/nvec/nvidia,nvec.txt | 19 ++----------------- > arch/arm/boot/dts/tegra20-paz00.dts | 22 > +++++++++------------- 2 files changed, 11 insertions(+), 30 deletions(-) > > diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt > b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt index > 5ae601e..d82c125 100644 > --- a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt > +++ b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt > @@ -2,20 +2,5 @@ NVIDIA compliant embedded controller > > Required properties: > - compatible : should be "nvidia,nvec". based on the earlier discussion with Wolfram, this should be "nvidia,nvec- slave" to distunguish it from a possible nvec master driver. > -- 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 > diff --git a/arch/arm/boot/dts/tegra20-paz00.dts > b/arch/arm/boot/dts/tegra20-paz00.dts index ed7e100..65e247b 100644 > --- a/arch/arm/boot/dts/tegra20-paz00.dts > +++ b/arch/arm/boot/dts/tegra20-paz00.dts > @@ -288,20 +288,16 @@ > clock-frequency = <100000>; > }; > > - 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 { > + compatible = "nvidia,nvec"; > + request-gpios = <&gpio TEGRA_GPIO(V, 2) > + GPIO_ACTIVE_HIGH>; > + reg = <0x45>; > + }; > }; > > i2c@7000d000 {
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. > + }; > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >> + }; >> }; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/31/2015 12:40 AM, Andrey Danin wrote: > 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 ? No, I don't think so. The I2C binding model is that each child of an I2C controller represents a device attached to the bus. which SW will communicate with using the I2C controller as master and the device as a slave. If there's no explicit representation of child-vs-slave in the DT, how does the I2C core know whether a particular node is intended to be accessed as a master or slave? In other words, without an explicit "communicate with this device" or "implement this device as a slave" flag, how could DT contain: i2c-controller { ... master@1a { compatible = "foo,device"; reg = <0x1a 1>; }; slave@1a { compatible = "foo,device-slave"; reg = <0x1a 1>; }; }; where: - "foo,device" means: instantiate a driver to communicate with a device of this type. - "foo,device-slave" means: instantiate a driver to act as this I2C device. Sure it's possible for the drivers for those two nodes to simply use the I2C subsystem's master or slave APIs, but I suspect DT content would confuse the I2C core into thinking that two I2C devices with the same address had been represented in DT, and the I2C core would refuse to instantiate one of them. The solution here is for the reg value to encode a "master" vs. "slave" flag, so the I2C core can allow both a master and a slave for each address. I'm pretty sure this is the nth time I've explained this. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31.03.2015 17:09, Stephen Warren wrote: > On 03/31/2015 12:40 AM, Andrey Danin wrote: >> 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 ? > > No, I don't think so. > > The I2C binding model is that each child of an I2C controller represents > a device attached to the bus. which SW will communicate with using the > I2C controller as master and the device as a slave. If there's no > explicit representation of child-vs-slave in the DT, how does the I2C > core know whether a particular node is intended to be accessed as a > master or slave? Device driver registers itself via slave API. Bus driver calls appropriate callback function when needed. If device driver decides to access hardware via master API, then it can do it. Am I missing something ? > > In other words, without an explicit "communicate with this device" or > "implement this device as a slave" flag, how could DT contain: > > i2c-controller { > ... > master@1a { > compatible = "foo,device"; > reg = <0x1a 1>; > }; > slave@1a { > compatible = "foo,device-slave"; > reg = <0x1a 1>; > }; > }; > > where: > > - "foo,device" means: instantiate a driver to communicate with a device > of this type. > > - "foo,device-slave" means: instantiate a driver to act as this I2C device. > > Sure it's possible for the drivers for those two nodes to simply use the > I2C subsystem's master or slave APIs, but I suspect DT content would > confuse the I2C core into thinking that two I2C devices with the same > address had been represented in DT, and the I2C core would refuse to > instantiate one of them. The solution here is for the reg value to > encode a "master" vs. "slave" flag, so the I2C core can allow both a > master and a slave for each address. If there is one device, then it must be one node. If there is two devices then it looks incorrect to me to have two devices with the same address. Does I2C allow two devices with same address ? I can imagine this: - we have hardware with I2C device. This device can act as master or as slave - we have device driver, that can work in one, other or both modes. If we want to force master or slave mode, we can use flags (for combined mode we can use two nodes, but it looks weird). If we want to let driver decide (preferred mode, arbitration, something else), we can use current rules. > > I'm pretty sure this is the nth time I've explained this. Sorry. I don't understand why you still suggest to use flags. We can use existing infrastructure in this case. There is already similar case in arch/arm/boot/dts/r8a7790-lager.dts (see i2c1 and eeprom). Do we *really* need this extra rules at this moment ? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Added Wolfram Sang and linux-i2c ML On 31.03.2015 18:46, Andrey Danin wrote: > On 31.03.2015 17:09, Stephen Warren wrote: >> On 03/31/2015 12:40 AM, Andrey Danin wrote: >>> 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 ? >> >> No, I don't think so. >> >> The I2C binding model is that each child of an I2C controller represents >> a device attached to the bus. which SW will communicate with using the >> I2C controller as master and the device as a slave. If there's no >> explicit representation of child-vs-slave in the DT, how does the I2C >> core know whether a particular node is intended to be accessed as a >> master or slave? > > Device driver registers itself via slave API. Bus driver calls > appropriate callback function when needed. > If device driver decides to access hardware via master API, then it can > do it. > > Am I missing something ? > >> >> In other words, without an explicit "communicate with this device" or >> "implement this device as a slave" flag, how could DT contain: >> >> i2c-controller { >> ... >> master@1a { >> compatible = "foo,device"; >> reg = <0x1a 1>; >> }; >> slave@1a { >> compatible = "foo,device-slave"; >> reg = <0x1a 1>; >> }; >> }; >> >> where: >> >> - "foo,device" means: instantiate a driver to communicate with a device >> of this type. >> >> - "foo,device-slave" means: instantiate a driver to act as this I2C >> device. >> >> Sure it's possible for the drivers for those two nodes to simply use the >> I2C subsystem's master or slave APIs, but I suspect DT content would >> confuse the I2C core into thinking that two I2C devices with the same >> address had been represented in DT, and the I2C core would refuse to >> instantiate one of them. The solution here is for the reg value to >> encode a "master" vs. "slave" flag, so the I2C core can allow both a >> master and a slave for each address. > > If there is one device, then it must be one node. If there is two > devices then it looks incorrect to me to have two devices with the same > address. Does I2C allow two devices with same address ? > > I can imagine this: > - we have hardware with I2C device. This device can act as master or as > slave > - we have device driver, that can work in one, other or both modes. > > If we want to force master or slave mode, we can use flags (for combined > mode we can use two nodes, but it looks weird). > If we want to let driver decide (preferred mode, arbitration, something > else), we can use current rules. > >> >> I'm pretty sure this is the nth time I've explained this. > > Sorry. I don't understand why you still suggest to use flags. We can use > existing infrastructure in this case. There is already similar case in > arch/arm/boot/dts/r8a7790-lager.dts (see i2c1 and eeprom). > > Do we *really* need this extra rules at this moment ? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/31/2015 09:46 AM, Andrey Danin wrote: > On 31.03.2015 17:09, Stephen Warren wrote: >> On 03/31/2015 12:40 AM, Andrey Danin wrote: >>> 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 ? >> >> No, I don't think so. >> >> The I2C binding model is that each child of an I2C controller represents >> a device attached to the bus. which SW will communicate with using the >> I2C controller as master and the device as a slave. If there's no >> explicit representation of child-vs-slave in the DT, how does the I2C >> core know whether a particular node is intended to be accessed as a >> master or slave? > > Device driver registers itself via slave API. Bus driver calls > appropriate callback function when needed. > If device driver decides to access hardware via master API, then it can > do it. > > Am I missing something ? > >> >> In other words, without an explicit "communicate with this device" or >> "implement this device as a slave" flag, how could DT contain: >> >> i2c-controller { >> ... >> master@1a { >> compatible = "foo,device"; >> reg = <0x1a 1>; >> }; >> slave@1a { >> compatible = "foo,device-slave"; >> reg = <0x1a 1>; >> }; >> }; >> >> where: >> >> - "foo,device" means: instantiate a driver to communicate with a device >> of this type. >> >> - "foo,device-slave" means: instantiate a driver to act as this I2C >> device. >> >> Sure it's possible for the drivers for those two nodes to simply use the >> I2C subsystem's master or slave APIs, but I suspect DT content would >> confuse the I2C core into thinking that two I2C devices with the same >> address had been represented in DT, and the I2C core would refuse to >> instantiate one of them. The solution here is for the reg value to >> encode a "master" vs. "slave" flag, so the I2C core can allow both a >> master and a slave for each address. > > If there is one device, then it must be one node. If there is two > devices then it looks incorrect to me to have two devices with the same > address. Does I2C allow two devices with same address ? One of the nodes is to indicate that the kernel should implement the slave mode device and one is to indicate that the kernel should implement the master mode device. Those two devices/nodes have completely different semantics, so while they share the I2C bus address they don't represent the same thing. Admittedly it would be uncommon to do this, since it'd be using the I2C bus in loopback mode. However, I don't see why we should set out to prevent that. > I can imagine this: > - we have hardware with I2C device. This device can act as master or as > slave > - we have device driver, that can work in one, other or both modes. > > If we want to force master or slave mode, we can use flags (for combined > mode we can use two nodes, but it looks weird). > If we want to let driver decide (preferred mode, arbitration, something > else), we can use current rules. > >> >> I'm pretty sure this is the nth time I've explained this. > > Sorry. I don't understand why you still suggest to use flags. We can use > existing infrastructure in this case. There is already similar case in > arch/arm/boot/dts/r8a7790-lager.dts (see i2c1 and eeprom). > > Do we *really* need this extra rules at this moment ? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 1. April 2015, 11:28:32 schrieb Stephen Warren: > On 03/31/2015 09:46 AM, Andrey Danin wrote: > > On 31.03.2015 17:09, Stephen Warren wrote: > >> On 03/31/2015 12:40 AM, Andrey Danin wrote: > >>> Hi, > >>> > >>> Thanks for the review. > >>> > >>> On 03.02.2015 0:20, Stephen Warren wrote: [ snipped old patch parts ] > >>>>> 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. Stephen, we haven't used your suggestion because Wolfram disliked the idea in e.g. http://lkml.iu.edu/hypermail/linux/kernel/1409.1/03446.html > >>> Driver (nvec in this case) can decide what mode should it use according > >>> to compatible value. Is it not enough ? > >> > >> No, I don't think so. > >> > >> The I2C binding model is that each child of an I2C controller represents > >> a device attached to the bus. which SW will communicate with using the > >> I2C controller as master and the device as a slave. If there's no > >> explicit representation of child-vs-slave in the DT, how does the I2C > >> core know whether a particular node is intended to be accessed as a > >> master or slave? > > > > Device driver registers itself via slave API. Bus driver calls > > appropriate callback function when needed. > > If device driver decides to access hardware via master API, then it can > > do it. > > > > Am I missing something ? > > > >> In other words, without an explicit "communicate with this device" or > >> "implement this device as a slave" flag, how could DT contain: > >> > >> i2c-controller { > >> > >> ... > >> master@1a { > >> > >> compatible = "foo,device"; > >> reg = <0x1a 1>; > >> > >> }; > >> slave@1a { > >> > >> compatible = "foo,device-slave"; > >> reg = <0x1a 1>; > >> > >> }; > >> > >> }; > >> > >> where: > >> > >> - "foo,device" means: instantiate a driver to communicate with a device > >> of this type. > >> > >> - "foo,device-slave" means: instantiate a driver to act as this I2C > >> device. > >> > >> Sure it's possible for the drivers for those two nodes to simply use the > >> I2C subsystem's master or slave APIs, but I suspect DT content would > >> confuse the I2C core into thinking that two I2C devices with the same > >> address had been represented in DT, and the I2C core would refuse to > >> instantiate one of them. The solution here is for the reg value to > >> encode a "master" vs. "slave" flag, so the I2C core can allow both a > >> master and a slave for each address. > > > > If there is one device, then it must be one node. If there is two > > devices then it looks incorrect to me to have two devices with the same > > address. Does I2C allow two devices with same address ? > > One of the nodes is to indicate that the kernel should implement the > slave mode device and one is to indicate that the kernel should > implement the master mode device. Those two devices/nodes have > completely different semantics, so while they share the I2C bus address > they don't represent the same thing. > > Admittedly it would be uncommon to do this, since it'd be using the I2C > bus in loopback mode. However, I don't see why we should set out to > prevent that. We are sitting between the chairs currently. I hope Wolfram can further comment on this. Having a generic loopback slave driver which just echos all messages it received back to the master (on the same controller or a different one) would be nice IMHO. > > I can imagine this: > > - we have hardware with I2C device. This device can act as master or as > > slave > > - we have device driver, that can work in one, other or both modes. > > > > If we want to force master or slave mode, we can use flags (for combined > > mode we can use two nodes, but it looks weird). > > If we want to let driver decide (preferred mode, arbitration, something > > else), we can use current rules. > > > >> I'm pretty sure this is the nth time I've explained this. > > > > Sorry. I don't understand why you still suggest to use flags. We can use > > existing infrastructure in this case. There is already similar case in > > arch/arm/boot/dts/r8a7790-lager.dts (see i2c1 and eeprom). > > > > Do we *really* need this extra rules at this moment ?
On 04/02/2015 03:37 AM, Marc Dietrich wrote: > > Am Mittwoch, 1. April 2015, 11:28:32 schrieb Stephen Warren: >> On 03/31/2015 09:46 AM, Andrey Danin wrote: >>> On 31.03.2015 17:09, Stephen Warren wrote: >>>> On 03/31/2015 12:40 AM, Andrey Danin wrote: >>>>> Hi, >>>>> >>>>> Thanks for the review. >>>>> >>>>> On 03.02.2015 0:20, Stephen Warren wrote: > > [ snipped old patch parts ] > >>>>>>> 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. > > Stephen, we haven't used your suggestion because Wolfram disliked the idea in > e.g. http://lkml.iu.edu/hypermail/linux/kernel/1409.1/03446.html As you said in the response you linked to, the objection is invalid since it won't break any DTs. The driver for a node is responsible for defining the meaning of its own reg properties. It should be pretty trivial to allow the Tegra I2C controller driver (or indeed any driver at all) to handle either #address-cells=<1> (the current setting) or #address-cells=<2> (a new value which enables adding a new flag cell) or even #address-cells=<1> with some of the upper bits of the reg value used as flags (which would default to 0 in all current DTs, so e.g. using the MSB==1 as a slave flag), all at run-time with complete backwards-compatibility. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt index 5ae601e..d82c125 100644 --- a/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt +++ b/Documentation/devicetree/bindings/nvec/nvidia,nvec.txt @@ -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 diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts index ed7e100..65e247b 100644 --- a/arch/arm/boot/dts/tegra20-paz00.dts +++ b/arch/arm/boot/dts/tegra20-paz00.dts @@ -288,20 +288,16 @@ clock-frequency = <100000>; }; - 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 { + compatible = "nvidia,nvec"; + request-gpios = <&gpio TEGRA_GPIO(V, 2) + GPIO_ACTIVE_HIGH>; + reg = <0x45>; + }; }; i2c@7000d000 {
NVEC driver was reimplemented to use tegra i2c. Use common i2c bindings for NVEC node. Signed-off-by: Andrey Danin <danindrey@mail.ru> --- .../devicetree/bindings/nvec/nvidia,nvec.txt | 19 ++----------------- arch/arm/boot/dts/tegra20-paz00.dts | 22 +++++++++------------- 2 files changed, 11 insertions(+), 30 deletions(-)