| Message ID | 1396547967-19260-1-git-send-email-soren.brinkmann@xilinx.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Apr 3, 2014 at 12:59 PM, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > Add device tree binding documentation for the Cadence I2C controller. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > > Changes in v4: > - moved adding DT docs into this dedicated patch > > Changes in v3: None > Changes in v2: None > > --- > .../devicetree/bindings/i2c/i2c-cadence.txt | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cadence.txt > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > new file mode 100644 > index 000000000000..685adf513111 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > @@ -0,0 +1,21 @@ > +Binding for the Cadence I2C controller > + > +Required properties: > + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'. > + clocks: From common clock bindings. Phandle to input clock. > + > +Optional properties: > + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may > + be lower). Defaults to 400000 if not specified. Since not all devices support 400kHz, I would prefer that the default be 100kHz. Also, it would be good if what speed the default is consistent across i2c drivers. Rob -- 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 Thu, 2014-04-03 at 02:51PM -0500, Rob Herring wrote: > On Thu, Apr 3, 2014 at 12:59 PM, Soren Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > Add device tree binding documentation for the Cadence I2C controller. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > > > Changes in v4: > > - moved adding DT docs into this dedicated patch > > > > Changes in v3: None > > Changes in v2: None > > > > --- > > .../devicetree/bindings/i2c/i2c-cadence.txt | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cadence.txt > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > > new file mode 100644 > > index 000000000000..685adf513111 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > > @@ -0,0 +1,21 @@ > > +Binding for the Cadence I2C controller > > + > > +Required properties: > > + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'. > > + clocks: From common clock bindings. Phandle to input clock. > > + > > +Optional properties: > > + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may > > + be lower). Defaults to 400000 if not specified. > > Since not all devices support 400kHz, I would prefer that the default > be 100kHz. Also, it would be good if what speed the default is > consistent across i2c drivers. Fine with me. I'll change this. Thanks, Sören -- 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 Thu, Apr 03, 2014 at 10:59:26AM -0700, Soren Brinkmann wrote: > Add a driver for the Cadence I2C controller. This controller is for > example found in Xilinx Zynq. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Tested-by: Michal Simek <michal.simek@xilinx.com> > Reviewed-by: Harini Katakam <harinik@xilinx.com> Only minor stuff, but since you resend anyhow because of 400kHz issue... > +static void cdns_i2c_mrecv(struct cdns_i2c *id) > +{ > + unsigned int ctrl_reg; > + unsigned int isr_status; > + > + id->p_recv_buf = id->p_msg->buf; > + id->recv_count = id->p_msg->len; > + > + /* Put the controller in master receive mode and clear the FIFO */ > + ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET); > + ctrl_reg |= CDNS_I2C_CR_RW | CDNS_I2C_CR_CLR_FIFO; > + > + if ((id->p_msg->flags & I2C_M_RECV_LEN) == I2C_M_RECV_LEN) 'if (id->p_msg->flags & I2C_M_RECV_LEN)' is enough. > + /* Report the other error interrupts to application as EIO */ > + if (id->err_status & (CDNS_I2C_IXR_RX_UNF | > + CDNS_I2C_IXR_TX_OVF | > + CDNS_I2C_IXR_RX_OVF | > + CDNS_I2C_IXR_NACK)) { > + cdns_i2c_master_reset(adap); > + return -EIO; > + } NACK is returned as -ENXIO. Check 'Documentation/i2c/fault-codes' for standard patterns. I'd think the rest is EIO, though.
On Thu, 2014-04-03 at 10:59 -0700, Soren Brinkmann wrote: > > Add device tree binding documentation for the Cadence I2C controller. > > [ ... ] > > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > @@ -0,0 +1,21 @@ > +Binding for the Cadence I2C controller > + > +Required properties: > + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'. > + clocks: From common clock bindings. Phandle to input clock. the usual complaint: 'clocks' items are not just phandles (your example even suggests this); either adjust the description to something correct, or just refer to the common clock bindings to not duplicate their description but I guess the I2C controller's binding should explicitly state which clocks are required, and you might want to consider 'clock-names', too > + > +Optional properties: > + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may > + be lower). Defaults to 400000 if not specified. is the value default a feature of the Linux implementation, or consciously designed to be in the binding spec? and I agree that the default should be the standard I2C speed instead of fast mode > + > +Example: > + > + i2c@e0004000 { > + compatible = "cdns,i2c-r1p10"; > + clocks = <&clkc 38>; > + interrupts = <0 25 4>; > + reg = <0xE0004000 0x1000>; > + clock-frequency = <400000>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; use lower case hex digits, consider symbolic identifiers for clocks and interrupt flags the example has many more properties than the binding describes, the additional items aren't even optional -- you might want to refer to a few more common or general bindings virtually yours Gerhard Sittig
On Fri, 2014-04-04 at 09:17PM +0200, Gerhard Sittig wrote: > On Thu, 2014-04-03 at 10:59 -0700, Soren Brinkmann wrote: > > > > Add device tree binding documentation for the Cadence I2C controller. > > > > [ ... ] > > > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt > > @@ -0,0 +1,21 @@ > > +Binding for the Cadence I2C controller > > + > > +Required properties: > > + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'. > > + clocks: From common clock bindings. Phandle to input clock. > > the usual complaint: 'clocks' items are not just phandles (your > example even suggests this); either adjust the description to > something correct, or just refer to the common clock bindings to > not duplicate their description I'll figure out something. > > but I guess the I2C controller's binding should explicitly state > which clocks are required, and you might want to consider > 'clock-names', too There is only one clock input. There is no need to overcomplicate things by adding clock-names. > > > + > > +Optional properties: > > + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may > > + be lower). Defaults to 400000 if not specified. > > is the value default a feature of the Linux implementation, or > consciously designed to be in the binding spec? and I agree that > the default should be the standard I2C speed instead of fast mode I remove the note regarding the default. It's implementation. > > > + > > +Example: > > + > > + i2c@e0004000 { > > + compatible = "cdns,i2c-r1p10"; > > + clocks = <&clkc 38>; > > + interrupts = <0 25 4>; > > + reg = <0xE0004000 0x1000>; > > + clock-frequency = <400000>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > use lower case hex digits, consider symbolic identifiers for > clocks and interrupt flags I don't care about the case of those hex digits, but where does it say that they have to be lower case? Will somebody complain about usage of lower case chars next? > > the example has many more properties than the binding describes, > the additional items aren't even optional -- you might want to > refer to a few more common or general bindings Well, this is common across binding documentation. Can we please get consistency in this? I see plenty of binding docs that are documented this way, not mentioning much regarding such common properties. Sören -- 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/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt new file mode 100644 index 000000000000..685adf513111 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt @@ -0,0 +1,21 @@ +Binding for the Cadence I2C controller + +Required properties: + compatible: Compatibility string. Must be 'cdns,i2c-r1p10'. + clocks: From common clock bindings. Phandle to input clock. + +Optional properties: + clock-frequency: Desired operating frequency, in Hz, of the bus (actual may + be lower). Defaults to 400000 if not specified. + +Example: + + i2c@e0004000 { + compatible = "cdns,i2c-r1p10"; + clocks = <&clkc 38>; + interrupts = <0 25 4>; + reg = <0xE0004000 0x1000>; + clock-frequency = <400000>; + #address-cells = <1>; + #size-cells = <0>; + };
Add device tree binding documentation for the Cadence I2C controller. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- Changes in v4: - moved adding DT docs into this dedicated patch Changes in v3: None Changes in v2: None --- .../devicetree/bindings/i2c/i2c-cadence.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cadence.txt