i2c: npcm7xx: add NPCM i2c controller driver
Tali Perry
maintainerv9 cover i2c: npcm7xx: add NPCM i2c controller driver
This patch set adds i2c controller support
for the Nuvoton NPCM Baseboard Management Controller (BMC).
NPCM7xx includes 16 I2C controllers. This driver operates the controller. This module also includes a slave mode.
---
v9 -> v8:
- Fix according to maintainer comments.
- Split lines of iowrite..(ioread..) to separate lines.
- Use readx_poll_timeout_atomic
- resolve various style issues.
v8 -> v7:
- Split to two commits, one for master, one for slave.
- Rename smb to i2c.
- Remove global vars.
v7 -> v6:
- Rebased on Linux 5.4-rc8 (was Linux 5.4-rc7).
- Fix issue found by kbuild test robot (redundant include).
- Note: left a warning related to fall through. This fall through is intentional.
v6 -> v5:
- Update documentation
v5 -> v4:
- support recovery
- master-slave switch support needed for IPMB
v4 -> v3:
- typo on cover letter.
v3 -> v2:
- fix dt binding: compatible name: omit "bus"
v2 -> v1:
- run check patch in strict mode.
- use linux crc.
- define regs in constant offset without base.
- remove debug prints.
- no declarations for local functions.
v1: initial version
---
Tali Perry (3):
dt-bindings: i2c: npcm7xx: add NPCM I2C controller documentation
i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
i2c: npcm7xx: Add support for slave mode for Nuvoton NPCM BMC I2C
controller driver.
.../devicetree/bindings/i2c/i2c-npcm7xx.txt | 29 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-npcm7xx.c | 2434 +++++++++++++++++ 4 files changed, 2473 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
base-commit: af42d3466bdc8f39806b26f593604fdc54140bcb
Signature · 1 line
2.22.0
Tali Perry
maintainerPATCH v9 1/3 dt-bindings: i2c: npcm7xx: add NPCM I2C controller
Added device tree binding documentation for Nuvoton BMC NPCM I2C controller.
--- .../devicetree/bindings/i2c/i2c-npcm7xx.txt | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt
Documentation/devicetree/bindings/i2c/i2c-npcm7xx.txt new file +29 -0
@@ -0,0 +1,29 @@ +Nuvoton NPCM7XX I2C bus + +The NPCM750x includes sixteen I2C bus controllers. All Controllers support +both master and slave mode. Each controller has two 16 byte HW FIFO for TX and +RX. + +Required properties: +- compatible : must be "nuvoton,npcm750-i2c" +- reg : Offset and length of the register set for the device. +- interrupts : Contain the I2C interrupt with flags for falling edge. +- clocks : phandle of I2C reference clock. + +Optional: +- bus-frequency : Contain the I2C bus frequency, + the default I2C bus frequency is 100000. +- pinctrl-0 : for example <&smbX_pins>, X is module number + (on NPCM7XX it's 0 to 15) +- pinctrl-names : should be set to "default" +Example: + + i2c0: i2c@80000 { + compatible = "nuvoton,npcm750-i2c"; + reg = <0x80000 0x1000>; + clocks = <&clk NPCM7XX_CLK_APB2>; + bus-frequency = <100000>; + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&smb0_pins>; + };
Signature · 1 line
2.22.0
Re: [PATCH v9 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller
Quoted 7 lines · Tali Perry
Added device tree binding documentation for Nuvoton BMC NPCM I2C controller.
Signed-off-by Tali Perry <[email protected]>Reviewed-by Rob Herring <[email protected]>--- .../devicetree/bindings/i2c/i2c-npcm7xx.txt | 29 +++++++++++++++++++
Wouldn't be better to have it in YAML from the day 1?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Tali Perry
maintainerPATCH v9 2/3 i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
In reply to Tali PerryAdd Nuvoton NPCM BMC I2C controller driver.
--- drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-npcm7xx.c | 1814 ++++++++++++++++++++++++++++++ 3 files changed, 1821 insertions(+) create mode 100644 drivers/i2c/busses/i2c-npcm7xx.c
drivers/i2c/busses/Kconfig +9 -0
@@ -786,6 +786,15 @@ config I2C_NOMADIK I2C interface from ST-Ericsson's Nomadik and Ux500 architectures, as well as the STA2X11 PCIe I/O HUB. +config I2C_NPCM7XX + tristate "Nuvoton I2C Controller" + depends on ARCH_NPCM7XX || COMPILE_TEST + help + If you say yes to this option, support will be included for the + Nuvoton I2C controller, which is available on the NPCM7xx BMC + controller. + Driver can also support slave mode (select I2C_SLAVE). + config I2C_OCORES tristate "OpenCores I2C Controller" help
drivers/i2c/busses/Makefile +1 -0
@@ -81,6 +81,7 @@ obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o obj-$(CONFIG_I2C_MXS) += i2c-mxs.o obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o +obj-$(CONFIG_I2C_NPCM7XX) += i2c-npcm7xx.o obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o obj-$(CONFIG_I2C_OMAP) += i2c-omap.o obj-$(CONFIG_I2C_OWL) += i2c-owl.o
drivers/i2c/busses/i2c-npcm7xx.c new file +1811 -0
1,811 lines not on this page · raw message
Signature · 1 line
2.22.0
Re: [PATCH v9 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
Quoted 1 line · Tali Perry
Add Nuvoton NPCM BMC I2C controller driver.
...
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/errno.h> +#include <linux/i2c.h>
drivers/i2c/busses/i2c-npcm7xx.c line 20
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10
-0+#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/jiffies.h> +#include <linux/iopoll.h>
Perhaps ordered?
drivers/i2c/busses/i2c-npcm7xx.c line 24
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+enum i2c_mode { +I2C_MASTER , +I2C_SLAVE
+ Comma.
Quoted 1 line
+};
...
Quoted 1 line
+#define
NPCM_I2CSEGCTL0xE4
Indentation issue, see the below lines.
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000 + +// Common regs +#define NPCM_I2CSDA 0x00
...
drivers/i2c/busses/i2c-npcm7xx.c line 237
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+#define I2C_FREQ_100KHZ 100 +#define I2C_FREQ_400KHZ 400 +#define I2C_FREQ_1MHZ 1000
Can you rather use standard definitions in Hz?
...
Quoted 1 line
+#define
NPCM_I2C_EVENT_LOG(event)(bus->event_log|= event)
Useless macro, and even harmful to some extend - it hides one of the parameter (proper should take bus and event, but it will be quite longer than simple
conditional). Use in-place the conditional.
...
drivers/i2c/busses/i2c-npcm7xx.c line 311
Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+struct npcm_i2c { +u32 xmits; +
Unnecessary blank line.
I have noticed other places with the same issue. Fix 'em all.
Quoted 1 line
+};
...
drivers/i2c/busses/i2c-npcm7xx.c line 372
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5
-0+static inline u16 npcm_i2c_get_index (struct npcm_i2c *bus) +{ +if (bus->operation ==I2C_READ_OPER ) +return bus->rd_ind; +else if (bus->operation ==I2C_WRITE_OPER )
Redundant 'else'
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+return bus->wr_ind; +return 0 ; +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 385
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+static void npcm_i2c_disable (struct npcm_i2c *bus) +{ +u8 i2cctl2;
+ blank line.
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7
-0+// Disable module. + i2cctl2 =ioread8 (bus->reg +NPCM_I2CCTL2 ); + i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE ; +iowrite8 (i2cctl2, bus->reg +NPCM_I2CCTL2 ); + + bus->state =I2C_DISABLE ; +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 396
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+static void npcm_i2c_enable (struct npcm_i2c *bus) +{ +u8 i2cctl2 =ioread8 (bus->reg +NPCM_I2CCTL2 );
In this case direct assignment like above is okay...
drivers/i2c/busses/i2c-npcm7xx.c line 406
Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10
-0+ + i2cctl2 = i2cctl2 |I2CCTL2_ENABLE ; +iowrite8 (i2cctl2, bus->reg +NPCM_I2CCTL2 ); + bus->state =I2C_IDLE ; +} + +// enable\disable end of busy (EOB) interrupt +static inline void npcm_i2c_eob_int (struct npcm_i2c *bus,bool enable) +{ +u8 val =ioread8 (bus->reg +NPCM_I2CCST3 );
...but not here. Better...
drivers/i2c/busses/i2c-npcm7xx.c line 408
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ +// Clear EO_BUSY pending bit:
...to perform it explicitly here.
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10
-0+ val = val |NPCM_I2CCST3_EO_BUSY ; +iowrite8 (val, bus->reg +NPCM_I2CCST3 ); + + val =ioread8 (bus->reg +NPCM_I2CCTL1 ); +if (enable) + val = (val |NPCM_I2CCTL1_EOBINTE ) & ~NPCM_I2CCTL1_RWS ; +else + val = (val & ~NPCM_I2CCTL1_EOBINTE ) & ~NPCM_I2CCTL1_RWS ; +iowrite8 (val, bus->reg +NPCM_I2CCTL1 ); +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 476
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+static void npcm_i2c_int_enable (struct npcm_i2c *bus,bool enable) +{ +u8 val =ioread8 (bus->reg +NPCM_I2CCTL1 ); + +if (enable) + val = (val |NPCM_I2CCTL1_INTEN ) & ~NPCM_I2CCTL1_RWS ; +else + val = (val & ~NPCM_I2CCTL1_INTEN ) & ~NPCM_I2CCTL1_RWS ; +iowrite8 (val, bus->reg +NPCM_I2CCTL1 );
if (enable) val |= NPCM_I2CCTL1_INTEN; else val &= ~NPCM_I2CCTL1_INTEN; iowrite8(val & ~NPCM_I2CCTL1_RWS, bus->reg + NPCM_I2CCTL1);
Ditto for the rest similar cases.
Quoted 1 line
+}
...
drivers/i2c/busses/i2c-npcm7xx.c line 486
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ val = (val |NPCM_I2CCTL1_START ) & + ~(NPCM_I2CCTL1_STOP |NPCM_I2CCTL1_ACK );
val |= NPCM_I2CCTL1_START; val &= ~(NPCM_I2CCTL1_STOP | NPCM_I2CCTL1_ACK);
Ditto for other similar cases.
...
drivers/i2c/busses/i2c-npcm7xx.c line 506
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+static inline void npcm_i2c_master_stop (struct npcm_i2c *bus) +{ +if (bus->fifo_use) {
if (...) return;
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12
-0+npcm_i2c_select_bank (bus,I2C_BANK_1 ); + +if (bus->operation ==I2C_READ_OPER ) +npcm_i2c_clear_rx_fifo (bus); +else +npcm_i2c_clear_tx_fifo (bus); + +npcm_i2c_clear_fifo_int (bus); + +iowrite8 (0 , bus->reg +NPCM_I2CTXF_CTL ); + } +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 582
Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1
-0+NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_CB );
Some magic happens here...
...
drivers/i2c/busses/i2c-npcm7xx.c line 642
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+static u32 npcm_i2c_get_fifo_fullness (struct npcm_i2c *bus) +{ +if (bus->operation ==I2C_WRITE_OPER ) +return FIELD_GET (NPCM_I2CTXF_STS_TX_BYTES , +ioread8 (bus->reg +NPCM_I2CTXF_STS )); +else if (bus->operation ==I2C_READ_OPER )
Redundant 'else'
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+return FIELD_GET (NPCM_I2CRXF_STS_RX_BYTES , +ioread8 (bus->reg +NPCM_I2CRXF_STS )); +return 0 ; +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 663
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+// configure the FIFO before using it. If nread is -1 RX FIFO will not be +// configured. same for nwrite
TABs/spaces mix.
...
drivers/i2c/busses/i2c-npcm7xx.c line 717
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11
-0+static void npcm_i2c_read_from_fifo (struct npcm_i2c *bus,u8 bytes_in_fifo) +{ +u8 data; + +while (bytes_in_fifo--) { + data =npcm_i2c_rd_byte (bus); + +if (bus->master_or_slave ==I2C_MASTER ) { +if (bus->rd_ind < bus->rd_size) + bus->rd_buf[bus->rd_ind++] = data; + }else {// I2C_SLAVE:
Redundant (at least in this patch).
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+ } + } +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 755
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+int rcount; +int fifo_bytes; +if (rcount < (2 *I2C_HW_FIFO_SIZE ) && rcount >I2C_HW_FIFO_SIZE ) + fifo_bytes = (u8)(rcount -I2C_HW_FIFO_SIZE );
Why explicit casting?
drivers/i2c/busses/i2c-npcm7xx.c line 757
Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1
-0+if ((rcount - fifo_bytes) <=0 ) {
Why not positive conditional and drop those parentheses?
Quoted 16 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +16
-0+// last bytes are about to be read - end of transaction. +// Stop should be set before reading last byte. +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_READ4 ); + + bus->state =I2C_STOP_PENDING ; + bus->stop_ind = ind; + +npcm_i2c_eob_int (bus, true); +npcm_i2c_master_stop (bus); +npcm_i2c_read_from_fifo (bus, fifo_bytes); + }else { +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_READ3 ); +npcm_i2c_read_from_fifo (bus, fifo_bytes); + rcount = bus->rd_size - bus->rd_ind; +npcm_i2c_set_fifo (bus, rcount,-1 ); + }
...
drivers/i2c/busses/i2c-npcm7xx.c line 850
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +10
-0+static void npcm_i2c_int_master_handler_read (struct npcm_i2c *bus) +{ +u16 block_extra_bytes_size; +u8 data; + +// Master read operation (pure read or following a write operation). +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_READ ); + +// added bytes to the packet: + block_extra_bytes_size = (u8 )bus->read_block_use + (u8 )bus->PEC_use;
Why explicit castings?
drivers/i2c/busses/i2c-npcm7xx.c line 863
Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12
-0+ +// Perform master read, distinguishing between last byte and the rest of +// the bytes. The last byte should be read when the clock is stopped +if (bus->rd_ind ==0 ) {//first byte handling: +// in block protocol first byte is the size +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_READ1 ); +if (bus->read_block_use) { +// first byte in block protocol is the size: + data =npcm_i2c_rd_byte (bus); +// if slave returned illegal size. read up to 32 bytes. +if (data >=I2C_SMBUS_BLOCK_MAX ) + data =I2C_SMBUS_BLOCK_MAX ;
min() / min_t() ? See below...
drivers/i2c/busses/i2c-npcm7xx.c line 867
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+ +// is data is 0 -> not supported. read at least one byte +if (data ==0 ) + data =1 ;
...actually clamp_val().
drivers/i2c/busses/i2c-npcm7xx.c line 880
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11
-0+ bus->rd_size = data + block_extra_bytes_size; + + bus->rd_buf[bus->rd_ind++] = data; + +// clear RX FIFO interrupt status: +if (bus->fifo_use) { + data =ioread8 (bus->reg +NPCM_I2CFIF_CTS ); + data = data |NPCM_I2CFIF_CTS_RXF_TXE ; +iowrite8 (data, bus->reg +NPCM_I2CFIF_CTS ); + } +npcm_i2c_set_fifo (bus, (bus->rd_size -1 ),-1 );
Too many parentheses.
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7
-0+npcm_i2c_stall_after_start (bus, false); + }else { +npcm_i2c_clear_tx_fifo (bus); +npcm_i2c_master_fifo_read (bus); + } + }else { + if (bus->rd_size == block_extra_bytes_size &&
drivers/i2c/busses/i2c-npcm7xx.c line 896
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+ bus->read_block_use) { + bus->state =I2C_STOP_PENDING ; + bus->stop_ind =I2C_BLOCK_BYTES_ERR_IND ; + bus->cmd_err = -EIO ; +npcm_i2c_eob_int (bus, true); +npcm_i2c_master_stop (bus); +npcm_i2c_read_from_fifo (bus, +npcm_i2c_get_fifo_fullness (bus) + );
Bad style. Combine last two lines together.
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+ }else { +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_READ2 ); +npcm_i2c_master_fifo_read (bus); + } + } +}
...
drivers/i2c/busses/i2c-npcm7xx.c line 940
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+static irqreturn_t npcm_i2c_int_master_handler (struct npcm_i2c *bus) +{ +npcm_i2c_eob_int (bus, false);
Extra spaces.
I have noticed more places with the same issue, fix 'em all.
Review
Quoted 2 lines
(quoted diff) +2
-0+ val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | + NPCM_I2CST_STASTR;
Make it temporary variable and use here and below...
Review
Quoted 2 lines
(quoted diff) +2
-0+ val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | + NPCM_I2CST_STASTR;
...here.
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; +}
Refactoring of such a big function will be benefit to all. And you may decrease indentation level at the same time.
...
drivers/i2c/busses/i2c-npcm7xx.c line 1157
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ fif_cts = (fif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR ) | +NPCM_I2CFIF_CTS_CLR_FIFO ;
fif_cts &= ~NPCM_I2CFIF_CTS_SLVRSTR; fif_cts |= NPCM_I2CFIF_CTS_CLR_FIFO;
...
drivers/i2c/busses/i2c-npcm7xx.c line 1174
Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12
-0+if (npcm_i2c_get_SDA (_adap) ==0 ) { +// Repeat the following sequence until SDA is released + do { + // Issue a single SCL cycle + iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST); + retries = 10; + while (retries != 0 && + FIELD_GET(NPCM_I2CCST_TGSCL, + ioread8(bus->reg + NPCM_I2CCST))) { + udelay(20); + retries--; + }
timeout loops more natural in do {} while form. But here is readx_poll_timeout() NIH.
drivers/i2c/busses/i2c-npcm7xx.c line 1178
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+// tgclk failed to toggle +if (retries ==0 ) +dev_err (bus->dev,"recovery toggle timeout" );
If it's an error, why we are still continuing?
drivers/i2c/busses/i2c-npcm7xx.c line 1182
Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+// If SDA line is inactive (high), stop +if (npcm_i2c_get_SDA (_adap)) + done = true; + }while ((!done) && (--iter !=0 ));
Too many parentheses. done can be dropped (below you may use iter in condition).
drivers/i2c/busses/i2c-npcm7xx.c line 1193
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+// If SDA line is released: send start-addr-stop, to re-sync. +if (done) { +npcm_i2c_master_start (bus); +// Wait until START condition is sent, or RETRIES_NUM + retries =RETRIES_NUM ; +while (retries && !npcm_i2c_is_master (bus)) { +udelay (20 ); + retries--; + }
do {} while ().
Quoted 11 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11
-0+// If START condition was sent +if (retries >0 ) { +// Send an address byte in write direction: +npcm_i2c_wr_byte (bus, bus->dest_addr); +udelay (200 ); +npcm_i2c_master_stop (bus); +udelay (200 ); + status =0 ; + } + } + }
...
drivers/i2c/busses/i2c-npcm7xx.c line 1215
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8
-0+if (unlikely (npcm_i2c_get_SDA (_adap) ==0 )) { +// Generate a START, to synchronize Master and Slave +npcm_i2c_master_start (bus); + +// Wait until START condition is sent, or RETRIES_NUM + retries =RETRIES_NUM ; +while (retries && !npcm_i2c_is_master (bus)) + retries--;
do {} while (). Fix them all.
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+// set SCL low for a long time (note: this is unlikely) +usleep_range (25000 ,35000 ); +npcm_i2c_master_stop (bus); +udelay (200 ); + status =0 ; + }
...
drivers/i2c/busses/i2c-npcm7xx.c line 1263
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+static bool npcm_i2c_init_clk (struct npcm_i2c *bus,u32 bus_freq) +{ +u32 k1 =0 ; +u32 k2 =0 ; +u8 dbnct =0 ; +u32 sclfrq =0 ; +u8 hldt =7 ; +bool fast_mode = false; +u32 src_clk_freq;// in KHz
This is very cryptic.
The function deserve a good comment above which shows all formulas in use with reference to datasheet pages, etc.
drivers/i2c/busses/i2c-npcm7xx.c line 1305
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+// Master or Slave with frequency > 25 MHZ + else if (src_clk_freq > 25000) { + hldt = (u8 )__KERNEL_DIV_ROUND_UP (src_clk_freq *300 , +1000000 ) +7 ; + + k1 =__KERNEL_DIV_ROUND_UP (src_clk_freq *1600 , +1000000 ); + k2 =__KERNEL_DIV_ROUND_UP (src_clk_freq *900 , +1000000 );
Why casting? Why __ special macro? Isn't DIV_ROUND_UP() enough? Ditto for other similar cases.
drivers/i2c/busses/i2c-npcm7xx.c line 1349
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8
-0+ k1 =round_up (k1,2 ); + k2 =round_up (k2 +1 ,2 ); +if (k1 <SCLFRQ_MIN || k1 >SCLFRQ_MAX || + k2 <SCLFRQ_MIN || k2 >SCLFRQ_MAX ) +return false; + } +// After clock parameters calculation update reg (ENABLE should be 0): + iowrite8(FIELD_PREP(I2CCTL2_SCLFRQ6_0, sclfrq & 0x7F),
Magic number.
drivers/i2c/busses/i2c-npcm7xx.c line 1365
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +11
-0+ bus->reg + NPCM_I2CCTL2); + +// force to bank 0, set SCL and fast mode +iowrite8 (FIELD_PREP (I2CCTL3_400K_MODE , fast_mode) | +FIELD_PREP (I2CCTL3_SCLFRQ8_7 , (sclfrq >>7 ) &0x3 ), + bus->reg +NPCM_I2CCTL3 ); + +// Select Bank 0 to access NPCM_I2CCTL4/NPCM_I2CCTL5 +npcm_i2c_select_bank (bus,I2C_BANK_0 ); +iowrite8 ((u8 )k1 /2 , bus->reg +NPCM_I2CSCLLT ); +iowrite8 ((u8 )k2 /2 , bus->reg +NPCM_I2CSCLHT );
Why casting?
...
drivers/i2c/busses/i2c-npcm7xx.c line 1396
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+if (FIELD_GET (I2C_VER_FIFO_EN ,ioread8 (bus->reg +I2C_VER ))) { + bus->fifo_use = true; +npcm_i2c_select_bank (bus,I2C_BANK_0 ); + val =ioread8 (bus->reg +NPCM_I2CFIF_CTL ); +iowrite8 (val |NPCM_I2CFIF_CTL_FIFO_EN , + bus->reg +NPCM_I2CFIF_CTL );
Do it in two operations.
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+npcm_i2c_select_bank (bus,I2C_BANK_1 ); + }else { + bus->fifo_use = false; + } + +// Configure I2C module clock frequency +if (!npcm_i2c_init_clk (bus, bus_freq)) { +dev_err (bus->dev,"npcm_i2c_init_clk failed\n" ); +return false;
drivers/i2c/busses/i2c-npcm7xx.c line 1414
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +9
-0+ } + +// Enable module (before configuring CTL1) +npcm_i2c_enable (bus); + bus->state =I2C_IDLE ; + +// Enable I2C int and New Address Match int source + val =ioread8 (bus->reg +NPCM_I2CCTL1 ); + val = (val |NPCM_I2CCTL1_NMINTE ) & ~NPCM_I2CCTL1_RWS ;
Usually we mask first, then disjunct with new bits. Actually this applies to all code where I also suggest to split to two operations.
Quoted 1 line
(quoted diff) +1
-0+ iowrite8(val, bus->reg + NPCM_I2CCTL1);
...
drivers/i2c/busses/i2c-npcm7xx.c line 1436
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ ret =of_property_read_u32 (pdev->dev.of_node, +"bus-frequency" , &clk_freq);
With ugly ifdefery this seems suspicious. Probably you imply device_property_read_u32()?
drivers/i2c/busses/i2c-npcm7xx.c line 1443
Quoted 8 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7
-0+if (ret <0 ) { +dev_err (&pdev->dev,"Could not read bus-frequency property\n" ); + clk_freq =100000 ; + } + + ret =npcm_i2c_init_module (bus,I2C_MASTER , clk_freq /1000 ); +if (!ret) {
0 is error condition?!
drivers/i2c/busses/i2c-npcm7xx.c line 1445
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+dev_err (&pdev->dev,"npcm_i2c_init_module() failed\n" ); +return -1 ;
Use proper error coded.
Quoted 1 line
(quoted diff) +1
-0+ }
...
drivers/i2c/busses/i2c-npcm7xx.c line 1482
Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1
-0+ bus->dest_addr = (u8)(slave_addr <<1 );// Translate 7bit to 8bit format
Awful formatting (comments) and casting.
drivers/i2c/busses/i2c-npcm7xx.c line 1512
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ i2cfif_cts = (i2cfif_cts & ~NPCM_I2CFIF_CTS_SLVRSTR ) | +NPCM_I2CFIF_CTS_CLR_FIFO ;
Do it in two operations.
...
drivers/i2c/busses/i2c-npcm7xx.c line 1540
Quoted 1 line
drivers/i2c/busses/i2c-npcm7xx.c new file +1
-0+if (unlikely (bus->state ==I2C_DISABLE )) {
Why unlikely() is in use? Any performance improvement? Show numbers.
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+dev_err (bus->dev,"I2C%d module is disabled" , bus->num); +return -EINVAL ; + }
...
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +4
-0+ time_left = jiffies + +msecs_to_jiffies (DEFAULT_STALL_COUNT ) +1 ; + do { + /* we must clear slave address immediately when the bus is not
drivers/i2c/busses/i2c-npcm7xx.c line 1619
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8
-0+* busy, so we spinlock it, but we don't keep the lock for the +* entire while since it is too long. +*/ +spin_lock_irqsave (&bus->lock, flags); + bus_busy =ioread8 (bus->reg +NPCM_I2CCST ) &NPCM_I2CCST_BB ; +spin_unlock_irqrestore (&bus->lock, flags); +if (!bus_busy) +break ;
This can be part of below condition.
Quoted 2 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ + }while (time_is_after_jiffies (time_left));
...
drivers/i2c/busses/i2c-npcm7xx.c line 1643
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+if (!npcm_i2c_master_start_xmit (bus, slave_addr, nwrite, nread, + write_data, read_data, read_PEC, + read_block)) + ret = -(EBUSY ); + +if (ret != -(EBUSY )) {
Too many parentheses. Fix them all in entire driver.
drivers/i2c/busses/i2c-npcm7xx.c line 1651
Quoted 9 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +8
-0+ time_left =wait_for_completion_timeout (&bus->cmd_complete, + timeout); + +if (time_left ==0 ) { +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_TO ); +if (bus->master_or_slave ==I2C_MASTER ) { +dev_dbg (bus->dev, +"i2c%d TO = %d\n" , bus->num, timeout);
Why not first line fit enough?
drivers/i2c/busses/i2c-npcm7xx.c line 1668
Quoted 13 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +12
-0+i2c_recover_bus (adap); + bus->cmd_err = -EIO ; + bus->state =I2C_IDLE ; + } + } + } + ret = bus->cmd_err; +// If nothing went wrong, return number of messages x-ferred. +if (ret >=0 ) +return num; + +return ret;
Why not traditional pattern, i.e.
if (ret < 0) return ret;
?
Quoted 1 line
+}
...
drivers/i2c/busses/i2c-npcm7xx.c line 1677
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +7
-0+static u32 npcm_i2c_functionality (struct i2c_adapter *adap) +{ +return I2C_FUNC_I2C | +I2C_FUNC_SMBUS_EMUL | +I2C_FUNC_SMBUS_BLOCK_DATA | +I2C_FUNC_SMBUS_PEC + ;
Awful indentation.
Quoted 1 line
+}
...
drivers/i2c/busses/i2c-npcm7xx.c line 1684
Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5
-0+static const struct i2c_adapter_quirks npcm_i2c_quirks = { + .max_read_len =32768 , + .max_write_len =32768 , + .max_num_msgs =2 , + .flags =I2C_AQ_COMB_WRITE_THEN_READ
Missed comma.
Quoted 1 line
+};
...
drivers/i2c/busses/i2c-npcm7xx.c line 1708
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+static int npcm_i2c_probe_bus (struct platform_device *pdev) +{ +#ifdef CONFIG_OF
Why this ugly ifdefery?
drivers/i2c/busses/i2c-npcm7xx.c line 1711
Quoted 4 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +3
-0+ num =of_alias_get_id (pdev->dev.of_node,"i2c" ); + bus->num = num; + i2c_clk =devm_clk_get (&pdev->dev,NULL );
Can't be optional?
drivers/i2c/busses/i2c-npcm7xx.c line 1713
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+if (IS_ERR (i2c_clk)) +return -EPROBE_DEFER ;
Why shadow an error code? Fix them all.
drivers/i2c/busses/i2c-npcm7xx.c line 1747
Quoted 7 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +5
-0+ bus->apb_clk =clk_get_rate (i2c_clk); +#endif // CONFIG_OF + bus->irq =platform_get_irq (pdev,0 ); +if (bus->irq <0 ) +return -ENODEV ;
Why shadowed error code?
drivers/i2c/busses/i2c-npcm7xx.c line 1768
Quoted 3 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +2
-0+ ret =i2c_add_numbered_adapter (&bus->adap); +if (ret <0 )
What about positive? Do they ever happen?
Hint: drop ' < 0' part in each case you have in this driver where it's not needed.
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c new file +6
-0+return ret; + +platform_set_drvdata (pdev, bus); + +return 0 ; +}
...
Quoted 1 line
+MODULE_VERSION("0.1.1");
Does it make any sense?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Tali Perry
maintainerPATCH v9 3/3 i2c: npcm7xx: Add support for slave mode for Nuvoton
In reply to Tali PerryAdd support for slave mode for Nuvoton
NPCM BMC I2C controller driver.
--- drivers/i2c/busses/i2c-npcm7xx.c | 623 +++++++++++++++++++++++++++++++ 1 file changed, 623 insertions(+)
drivers/i2c/busses/i2c-npcm7xx.c +623 -0
@@ -68,6 +68,24 @@ enum i2c_state { I2C_STOP_PENDING, }; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +// Module supports setting multiple own slave addresses +enum i2c_addr { +I2C_SLAVE_ADDR1 =0 , +I2C_SLAVE_ADDR2 , +I2C_SLAVE_ADDR3 , +I2C_SLAVE_ADDR4 , +I2C_SLAVE_ADDR5 , +I2C_SLAVE_ADDR6 , +I2C_SLAVE_ADDR7 , +I2C_SLAVE_ADDR8 , +I2C_SLAVE_ADDR9 , +I2C_SLAVE_ADDR10 , +I2C_GC_ADDR , +I2C_ARP_ADDR , +}; +#endif +// init register and default value required to enable module #define NPCM_I2CSEGCTL 0xE4#define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
@@ -95,6 +113,14 @@ enum i2c_state {#define NPCM_I2CADDR6 0x16#define NPCM_I2CADDR10 0x17 +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +// I2CADDR array: because the addr regs are sprinkled all over the address space +const int npcm_i2caddr[10 ] = {NPCM_I2CADDR1 ,NPCM_I2CADDR2 ,NPCM_I2CADDR3 , +NPCM_I2CADDR4 ,NPCM_I2CADDR5 ,NPCM_I2CADDR6 , +NPCM_I2CADDR7 ,NPCM_I2CADDR8 ,NPCM_I2CADDR9 , +NPCM_I2CADDR10 }; +#endif +#define NPCM_I2CCTL4 0x1A#define NPCM_I2CCTL5 0x1B#define NPCM_I2CSCLLT 0x1C // SCL Low Time
@@ -309,6 +335,16 @@ struct npcm_i2c {unsigned long bus_freq;// in kHz u32 xmits; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +u8 own_slave_addr; +struct i2c_client *slave; +int slv_rd_size; +int slv_rd_ind; +int slv_wr_size; +int slv_wr_ind; +u8 slv_rd_buf[I2C_HW_FIFO_SIZE ]; +u8 slv_wr_buf[I2C_HW_FIFO_SIZE ]; +#endif }; static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -337,6 +373,10 @@ static void npcm_i2c_init_params(struct npcm_i2c *bus) bus->int_time_stamp =0 ; bus->PEC_use = false; bus->PEC_mask =0 ; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) + bus->master_or_slave =I2C_SLAVE ; +#endif }static inline void npcm_i2c_wr_byte (struct npcm_i2c *bus,u8 data)
@@ -383,6 +423,18 @@ static inline bool npcm_i2c_is_quick(struct npcm_i2c *bus)static void npcm_i2c_disable (struct npcm_i2c *bus) {u8 i2cctl2; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +int i; + +// select bank 0 for I2C addresses +npcm_i2c_select_bank (bus,I2C_BANK_0 ); + +// Slave addresses removal +for (i =I2C_SLAVE_ADDR1 ; i <I2C_NUM_OF_ADDR ; i++) +iowrite8 (0 , bus->reg + npcm_i2caddr[i]); + +npcm_i2c_select_bank (bus,I2C_BANK_1 ); +#endif // Disable module. i2cctl2 =ioread8 (bus->reg +NPCM_I2CCTL2 ); i2cctl2 = i2cctl2 & ~I2CCTL2_ENABLE ;
@@ -537,6 +589,51 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)iowrite8 (val, bus->reg +NPCM_I2CCTL1 ); } +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +static int npcm_i2c_slave_enable_l (struct npcm_i2c *bus, +enum i2c_addr addr_type,u8 addr, +bool enable) +{ +u8 i2cctl1; +u8 i2cctl3; +u8 slave_addr_reg =FIELD_PREP (NPCM_I2CADDR_A , addr) | +FIELD_PREP (NPCM_I2CADDR_SAEN , enable); + +if (addr_type ==I2C_GC_ADDR ) { + i2cctl1 =ioread8 (bus->reg +NPCM_I2CCTL1 ); + i2cctl1 = (i2cctl1 & ~NPCM_I2CCTL1_GCMEN ) | +FIELD_PREP (NPCM_I2CCTL1_GCMEN , enable); +iowrite8 (i2cctl1, bus->reg +NPCM_I2CCTL1 ); +return 0 ; + } +if (addr_type ==I2C_ARP_ADDR ) { + i2cctl3 =ioread8 (bus->reg +NPCM_I2CCTL3 ); + i2cctl3 = (i2cctl3 & ~I2CCTL3_ARPMEN ) | +FIELD_PREP (I2CCTL3_ARPMEN , enable); +iowrite8 (i2cctl3, bus->reg +NPCM_I2CCTL3 ); +return 0 ; + } +if (addr_type >=I2C_ARP_ADDR ) +return -EFAULT ; + +// select bank 0 for address 3 to 10 +if (addr_type >I2C_SLAVE_ADDR2 ) +npcm_i2c_select_bank (bus,I2C_BANK_0 ); + +// Set and enable the address +iowrite8 (slave_addr_reg, bus->reg + npcm_i2caddr[(int )addr_type]); + +// enable interrupt on slave match: + i2cctl1 =ioread8 (bus->reg +NPCM_I2CCTL1 ); + i2cctl1 = (i2cctl1 |NPCM_I2CCTL1_NMINTE ) & ~NPCM_I2CCTL1_RWS ; +iowrite8 (i2cctl1, bus->reg +NPCM_I2CCTL1 ); + +if (addr_type >I2C_SLAVE_ADDR2 ) +npcm_i2c_select_bank (bus,I2C_BANK_1 ); +return 0 ; +} +#endif +static void npcm_i2c_reset (struct npcm_i2c *bus) {// Save NPCM_I2CCTL1 relevant bits. It is being cleared when the
@@ -565,6 +662,13 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)// Clear all fifo bits: iowrite8 (NPCM_I2CFIF_CTS_CLR_FIFO , bus->reg +NPCM_I2CFIF_CTS ); +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) { +npcm_i2c_slave_enable_l (bus,I2C_SLAVE_ADDR1 , bus->slave->addr, + true); + } +#endif + bus->state =I2C_IDLE ; }
@@ -632,6 +736,10 @@ static void npcm_i2c_callback(struct npcm_i2c *bus, } bus->operation =I2C_NO_OPER ; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) + bus->master_or_slave =I2C_SLAVE ; +#endif }static u32 npcm_i2c_get_fifo_fullness (struct npcm_i2c *bus)
@@ -659,6 +767,33 @@ static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus, } } +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +static void npcm_i2c_write_to_fifo_slave (struct npcm_i2c *bus, +u16 max_bytes_to_send) +{ +// Fill the FIFO, while the FIFO is not full and there are more bytes to +// write +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_tx_fifo (bus); +iowrite8 (0 , bus->reg +NPCM_I2CTXF_CTL ); + +while ((max_bytes_to_send--) && (I2C_HW_FIFO_SIZE - +npcm_i2c_get_fifo_fullness (bus))) { +if (bus->slv_wr_size >0 ) { +npcm_i2c_wr_byte (bus, + bus->slv_wr_buf[bus->slv_wr_ind % +I2C_HW_FIFO_SIZE ]); + bus->slv_wr_ind = (bus->slv_wr_ind +1 ) % +I2C_HW_FIFO_SIZE ; + bus->slv_wr_size--;// size indicates the # of bytes in +// the SW FIFO, not HW. + }else { +break ; + } + } +} +#endif +// configure the FIFO before using it. If nread is -1 RX FIFO will not be // configured. same for nwrite static void npcm_i2c_set_fifo (struct npcm_i2c *bus,int nread,int nwrite)
@@ -715,6 +850,18 @@ static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)if (bus->rd_ind < bus->rd_size) bus->rd_buf[bus->rd_ind++] = data; }else {// I2C_SLAVE: +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) { + bus->slv_rd_buf[bus->slv_rd_ind % +I2C_HW_FIFO_SIZE ] = data; + bus->slv_rd_ind++; +if (bus->slv_rd_ind ==1 && bus->read_block_use) +// 1st byte is length in block protocol + bus->slv_rd_size = data + + (u8 )bus->PEC_use + + (u8 )bus->read_block_use; + } +#endif } } }
@@ -736,6 +883,452 @@ static void npcm_i2c_master_abort(struct npcm_i2c *bus) } } +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +static u8 npcm_i2c_get_slave_addr (struct npcm_i2c *bus, +enum i2c_addr addr_type) +{ +u8 slave_add; + +// select bank 0 for address 3 to 10 +if (addr_type >I2C_SLAVE_ADDR2 ) +npcm_i2c_select_bank (bus,I2C_BANK_0 ); + + slave_add =ioread8 (bus->reg + npcm_i2caddr[(int )addr_type]); + +if (addr_type >I2C_SLAVE_ADDR2 ) +npcm_i2c_select_bank (bus,I2C_BANK_1 ); + +return slave_add; +} + +static int npcm_i2c_remove_slave_addr (struct npcm_i2c *bus,u8 slave_add) +{ +int i; + + slave_add |=0x80 ;//Set the enable bit + +npcm_i2c_select_bank (bus,I2C_BANK_0 ); + +for (i =I2C_SLAVE_ADDR1 ; i <I2C_NUM_OF_ADDR ; i++) { +if (ioread8 (bus->reg + npcm_i2caddr[i]) == slave_add) +iowrite8 (0 , bus->reg + npcm_i2caddr[i]); + } + +npcm_i2c_select_bank (bus,I2C_BANK_1 ); + +return 0 ; +} + +static int npcm_i2c_slave_get_wr_buf (struct npcm_i2c *bus) +{ +u8 value =0 ; +int ret = bus->slv_wr_ind; +int i; + +// fill a cyclic buffer +for (i =0 ; i <I2C_HW_FIFO_SIZE ; i++) { +if (bus->slv_wr_size >=I2C_HW_FIFO_SIZE ) +break ; +i2c_slave_event (bus->slave,I2C_SLAVE_READ_REQUESTED , &value); + bus->slv_wr_buf[(bus->slv_wr_ind + bus->slv_wr_size) % +I2C_HW_FIFO_SIZE ] = value; + bus->slv_wr_size++; +i2c_slave_event (bus->slave,I2C_SLAVE_READ_PROCESSED , &value); + } +return I2C_HW_FIFO_SIZE - ret; +} + +static void npcm_i2c_slave_send_rd_buf (struct npcm_i2c *bus) +{ +int i; + +for (i =0 ; i < bus->slv_rd_ind; i++) +i2c_slave_event (bus->slave,I2C_SLAVE_WRITE_RECEIVED , + &bus->slv_rd_buf[i]); + +// once we send bytes up, need to reset the counter of the wr buf +// got data from master (new offset in device), ignore wr fifo: +if (bus->slv_rd_ind) { + bus->slv_wr_size =0 ; + bus->slv_wr_ind =0 ; + } + + bus->slv_rd_ind =0 ; + bus->slv_rd_size = bus->adap.quirks->max_read_len; + +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_rx_fifo (bus); +} + +static bool npcm_i2c_slave_receive (struct npcm_i2c *bus,u16 nread, +u8 *read_data) +{ + bus->state =I2C_OPER_STARTED ; + bus->operation =I2C_READ_OPER ; + bus->slv_rd_size = nread; + bus->slv_rd_ind =0 ; + +iowrite8 (0 , bus->reg +NPCM_I2CTXF_CTL ); +iowrite8 (I2C_HW_FIFO_SIZE , bus->reg +NPCM_I2CRXF_CTL ); + +npcm_i2c_clear_tx_fifo (bus); +npcm_i2c_clear_rx_fifo (bus); + +return true; +} + +static bool npcm_i2c_slave_xmit (struct npcm_i2c *bus,u16 nwrite, +u8 *write_data) +{ +if (nwrite ==0 ) +return false; + + bus->state =I2C_OPER_STARTED ; + bus->operation =I2C_WRITE_OPER ; + +// get the next buffer +npcm_i2c_slave_get_wr_buf (bus); + +if (nwrite >0 ) +npcm_i2c_write_to_fifo_slave (bus, nwrite); + +return true; +} + +// currently slave IF only supports single byte operations. +// in order to utilyze the npcm HW FIFO, the driver will ask for 16 bytes +// at a time, pack them in buffer, and then transmit them all together +// to the FIFO and onward to the bus. +// NACK on read will be once reached to bus->adap->quirks->max_read_len. +// sending a NACK wherever the backend requests for it is not supported. +// the next two functions allow reading to local buffer before writing it all +// to the HW FIFO. +// ret val: number of bytes read form the IF: + +static int npcm_i2c_slave_wr_buf_sync (struct npcm_i2c *bus) +{ +int left_in_fifo =FIELD_GET (NPCM_I2CTXF_STS_TX_BYTES , +ioread8 (bus->reg +NPCM_I2CTXF_STS )); + +if (left_in_fifo >=I2C_HW_FIFO_SIZE ) +return left_in_fifo; + +if (bus->slv_wr_size >=I2C_HW_FIFO_SIZE ) +return left_in_fifo;// fifo already full + +// update the wr fifo ind, back to the untransmitted bytes: + bus->slv_wr_ind = bus->slv_wr_ind - left_in_fifo; + bus->slv_wr_size = bus->slv_wr_size + left_in_fifo; + +if (bus->slv_wr_ind <0 ) + bus->slv_wr_ind +=I2C_HW_FIFO_SIZE ; + +return left_in_fifo; +} + +static void npcm_i2c_slave_rd_wr (struct npcm_i2c *bus) +{ +if (FIELD_GET (NPCM_I2CST_XMIT ,ioread8 (bus->reg +NPCM_I2CST ))) { +// Slave got an address match with direction bit 1 so +// it should transmit data +// Write till the master will NACK + bus->operation =I2C_WRITE_OPER ; +npcm_i2c_slave_xmit (bus, + bus->adap.quirks->max_write_len, + bus->slv_wr_buf); + }else { +// Slave got an address match with direction bit 0 +// so it should receive data. +// this module does not support saying no to bytes. +// it will always ACK. + bus->operation =I2C_READ_OPER ; +npcm_i2c_read_from_fifo (bus,npcm_i2c_get_fifo_fullness (bus)); + bus->stop_ind =I2C_SLAVE_RCV_IND ; +npcm_i2c_slave_send_rd_buf (bus); +npcm_i2c_slave_receive (bus, + bus->adap.quirks->max_read_len, + bus->slv_rd_buf); + } +} + +static irqreturn_t npcm_i2c_int_slave_handler (struct npcm_i2c *bus) +{ +u8 val; +irqreturn_t ret =IRQ_NONE ; +u8 i2cst =ioread8 (bus->reg +NPCM_I2CST ); + +// Slave: A NACK has occurred +if (FIELD_GET (NPCM_I2CST_NEGACK , i2cst)) { +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_NACK ); + bus->stop_ind =I2C_NACK_IND ; +npcm_i2c_slave_wr_buf_sync (bus); +if (bus->fifo_use) +// clear the FIFO +iowrite8 (NPCM_I2CFIF_CTS_CLR_FIFO , + bus->reg +NPCM_I2CFIF_CTS ); + +// In slave write, NACK is OK, otherwise it is a problem + bus->stop_ind =I2C_NO_STATUS_IND ; + bus->operation =I2C_NO_OPER ; + bus->own_slave_addr =0xFF ; + +// Slave has to wait for I2C_STOP to decide this is the end +// of the transaction. +// Therefore transaction is not yet considered as done +iowrite8 (NPCM_I2CST_NEGACK , bus->reg +NPCM_I2CST ); + + ret =IRQ_HANDLED ; + } + +// Slave mode: a Bus Error (BER) has been identified +if (FIELD_GET (NPCM_I2CST_BER , i2cst)) { +// Check whether bus arbitration or Start or Stop during data +// xfer bus arbitration problem should not result in recovery +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_BER_SLV ); + bus->stop_ind =I2C_BUS_ERR_IND ; + +// wait for bus busy before clear fifo +iowrite8 (NPCM_I2CFIF_CTS_CLR_FIFO , bus->reg +NPCM_I2CFIF_CTS ); + + bus->state =I2C_IDLE ; + +// in BER case we might get 2 interrupts: one for slave one for +// master ( for a channel which is master\slave switching) +if (completion_done (&bus->cmd_complete) == false) { + bus->cmd_err = -EIO ; +complete (&bus->cmd_complete); + } + bus->own_slave_addr =0xFF ; +iowrite8 (NPCM_I2CST_BER , bus->reg +NPCM_I2CST ); + ret =IRQ_HANDLED ; + } + +// A Slave Stop Condition has been identified +if (FIELD_GET (NPCM_I2CST_SLVSTP , i2cst)) { +int bytes_in_fifo =npcm_i2c_get_fifo_fullness (bus); + + bus->stop_ind =I2C_SLAVE_DONE_IND ; + +if (bus->operation ==I2C_READ_OPER ) { +npcm_i2c_read_from_fifo (bus, bytes_in_fifo); + +// Slave done transmitting or receiving +// if the buffer is empty nothing will be sent + } + +// Slave done transmitting or receiving +// if the buffer is empty nothing will be sent +npcm_i2c_slave_send_rd_buf (bus); + + bus->stop_ind =I2C_NO_STATUS_IND ; + +// Note, just because we got here, it doesn't mean we through +// away the wr buffer. +// we keep it until the next received offset. + bus->operation =I2C_NO_OPER ; + bus->int_cnt =0 ; + bus->event_log_prev = bus->event_log; + bus->event_log =0 ; + bus->own_slave_addr =0xFF ; + +i2c_slave_event (bus->slave,I2C_SLAVE_STOP ,0 ); + +iowrite8 (NPCM_I2CST_SLVSTP , bus->reg +NPCM_I2CST ); + +if (bus->fifo_use) { +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_rx_fifo (bus); +npcm_i2c_clear_tx_fifo (bus); + +iowrite8 (NPCM_I2CFIF_CTS_CLR_FIFO , + bus->reg +NPCM_I2CFIF_CTS ); + } + + bus->state =I2C_IDLE ; + ret =IRQ_HANDLED ; + } + +// restart condition occurred and Rx-FIFO was not empty +if (bus->fifo_use &&FIELD_GET (NPCM_I2CFIF_CTS_SLVRSTR , +ioread8 (bus->reg +NPCM_I2CFIF_CTS ))) { + bus->stop_ind =I2C_SLAVE_RESTART_IND ; + + bus->master_or_slave =I2C_SLAVE ; + +if (bus->operation ==I2C_READ_OPER ) +npcm_i2c_read_from_fifo (bus, +npcm_i2c_get_fifo_fullness (bus) + ); + + bus->operation =I2C_WRITE_OPER ; + +iowrite8 (0 , bus->reg +NPCM_I2CRXF_CTL ); + + val =NPCM_I2CFIF_CTS_CLR_FIFO |NPCM_I2CFIF_CTS_SLVRSTR | +NPCM_I2CFIF_CTS_RXF_TXE ; +iowrite8 (val, bus->reg +NPCM_I2CFIF_CTS ); + +npcm_i2c_slave_rd_wr (bus); + + ret =IRQ_HANDLED ; + } + +// A Slave Address Match has been identified +if (FIELD_GET (NPCM_I2CST_NMATCH , i2cst)) { +u8 info =0 ; + +NPCM_I2C_EVENT_LOG (NPCM_I2C_EVENT_NMATCH_SLV ); +// Address match automatically implies slave mode + bus->master_or_slave =I2C_SLAVE ; + +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_rx_fifo (bus); +npcm_i2c_clear_tx_fifo (bus); +iowrite8 (0 , bus->reg +NPCM_I2CTXF_CTL ); +iowrite8 (I2C_HW_FIFO_SIZE , bus->reg +NPCM_I2CRXF_CTL ); + +if (FIELD_GET (NPCM_I2CST_XMIT , i2cst)) { + bus->operation =I2C_WRITE_OPER ; + }else { +i2c_slave_event (bus->slave,I2C_SLAVE_WRITE_REQUESTED , + &info); + bus->operation =I2C_READ_OPER ; + } + +if (bus->own_slave_addr ==0xFF ) {// unknown address +// Check which type of address match +if (FIELD_GET (NPCM_I2CCST_MATCH , +ioread8 (bus->reg +NPCM_I2CCST ))) { +u16 addr; +enum i2c_addr eaddr; + + addr = ((ioread8 (bus->reg +NPCM_I2CCST3 ) & +0x7 ) <<7 ) | + (ioread8 (bus->reg +NPCM_I2CCST2 ) & +0x7F ); + + info =ffs (addr); + eaddr = (enum i2c_addr )info; + + addr =FIELD_GET (NPCM_I2CADDR_A , +npcm_i2c_get_slave_addr (bus, + eaddr) + ); + bus->own_slave_addr = addr; + +if (bus->PEC_mask &BIT (info)) + bus->PEC_use = true; +else + bus->PEC_use = false; + }else { +if (FIELD_GET (NPCM_I2CCST_GCMATCH , +ioread8 (bus->reg +NPCM_I2CCST ))) + bus->own_slave_addr =0 ; +if (FIELD_GET (NPCM_I2CCST_ARPMATCH , +ioread8 (bus->reg +NPCM_I2CCST ))) + bus->own_slave_addr =0x61 ; + } + }else { +// Slave match can happen in two options: +// 1. Start, SA, read (slave read without further ado) +// 2. Start, SA, read, data, restart, SA, read, ... +// (slave read in fragmented mode) +// 3. Start, SA, write, data, restart, SA, read, .. +// (regular write-read mode) +if ((bus->state ==I2C_OPER_STARTED && + bus->operation ==I2C_READ_OPER && + bus->stop_ind ==I2C_SLAVE_XMIT_IND ) || + bus->stop_ind ==I2C_SLAVE_RCV_IND ) { +// slave transmit after slave receive w/o Slave +// Stop implies repeated start + bus->stop_ind =I2C_SLAVE_RESTART_IND ; + } + } + +if (FIELD_GET (NPCM_I2CST_XMIT , i2cst)) + bus->stop_ind =I2C_SLAVE_XMIT_IND ; +else + bus->stop_ind =I2C_SLAVE_RCV_IND ; + + bus->state =I2C_SLAVE_MATCH ; + +npcm_i2c_slave_rd_wr (bus); + +iowrite8 (NPCM_I2CST_NMATCH , bus->reg +NPCM_I2CST ); + ret =IRQ_HANDLED ; + } + +// Slave SDA status is set - transmit or receive, slave +if (FIELD_GET (NPCM_I2CST_SDAST , i2cst) || + (bus->fifo_use && + (npcm_i2c_tx_fifo_empty (bus) ||npcm_i2c_rx_fifo_full (bus)))) { +npcm_i2c_slave_rd_wr (bus); + +iowrite8 (NPCM_I2CST_SDAST , bus->reg +NPCM_I2CST ); + + ret =IRQ_HANDLED ; + }//SDAST + +return ret; +} + +static int npcm_i2c_reg_slave (struct i2c_client *client) +{ +unsigned long lock_flags; +struct npcm_i2c *bus =i2c_get_adapdata (client->adapter); + + bus->slave = client; + +if (!bus->slave) +return -EINVAL ; + +if (client->flags &I2C_CLIENT_TEN ) +return -EAFNOSUPPORT ; + +spin_lock_irqsave (&bus->lock, lock_flags); + +npcm_i2c_init_params (bus); + bus->slv_rd_size =0 ; + bus->slv_wr_size =0 ; + bus->slv_rd_ind =0 ; + bus->slv_wr_ind =0 ; +if (client->flags &I2C_CLIENT_PEC ) + bus->PEC_use = true; + +dev_info (bus->dev,"I2C%d register slave SA=0x%x, PEC=%d\n" , bus->num, + client->addr, bus->PEC_use); + +npcm_i2c_slave_enable_l (bus,I2C_SLAVE_ADDR1 , client->addr, true); + +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_rx_fifo (bus); +npcm_i2c_clear_tx_fifo (bus); + +spin_unlock_irqrestore (&bus->lock, lock_flags); + +return 0 ; +} + +static int npcm_i2c_unreg_slave (struct i2c_client *client) +{ +struct npcm_i2c *bus = client->adapter->algo_data; +unsigned long lock_flags; + +spin_lock_irqsave (&bus->lock, lock_flags); +if (!bus->slave) { +spin_unlock_irqrestore (&bus->lock, lock_flags); +return -EINVAL ; + } + +npcm_i2c_remove_slave_addr (bus, client->addr); + + bus->slave =NULL ; +spin_unlock_irqrestore (&bus->lock, lock_flags); + +return 0 ; +} +#endif // CONFIG_I2C_SLAVE +static void npcm_i2c_master_fifo_read (struct npcm_i2c *bus) {int rcount;
@@ -1430,6 +2023,9 @@ static int __npcm_i2c_init(struct npcm_i2c *bus, struct platform_device *pdev) bus->state =I2C_DISABLE ; bus->master_or_slave =I2C_SLAVE ; bus->int_time_stamp =0 ; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) + bus->slave =NULL ; +#endif bus->xmits =0 ; ret = of_property_read_u32(pdev->dev.of_node,
@@ -1464,6 +2060,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)if (ret ==IRQ_HANDLED )return ret; } +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) { + bus->master_or_slave =I2C_SLAVE ; + ret =npcm_i2c_int_slave_handler (bus); +if (ret ==IRQ_HANDLED ) +return ret; + } +#endif return IRQ_HANDLED ; }
@@ -1613,6 +2217,11 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,*/ spin_lock_irqsave (&bus->lock, flags); bus_busy =ioread8 (bus->reg +NPCM_I2CCST ) &NPCM_I2CCST_BB ; +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (!bus_busy && bus->slave) +iowrite8 ((bus->slave->addr &0x7F ), + bus->reg +NPCM_I2CADDR1 ); +#endif spin_unlock_irqrestore (&bus->lock, flags);if (!bus_busy)
@@ -1661,6 +2270,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,if (bus->cmd_err == -EAGAIN )i2c_recover_bus (adap); +#if IS_ENABLED (CONFIG_I2C_SLAVE ) +// reenable slave if it was enabled +if (bus->slave) +iowrite8 ((bus->slave->addr &0x7F ) |NPCM_I2CADDR_SAEN , + bus->reg +NPCM_I2CADDR1 ); +#endif +// If nothing went wrong, return number of messages x-ferred. if (ret >=0 )return num;
@@ -1674,6 +2290,9 @@ static u32 npcm_i2c_functionality(struct i2c_adapter *adap)I2C_FUNC_SMBUS_EMUL |I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PEC +#if IS_ENABLED(CONFIG_I2C_SLAVE) + |I2C_FUNC_SLAVE +#endif ; }
@@ -1687,6 +2306,10 @@ static const struct i2c_adapter_quirks npcm_i2c_quirks = {static const struct i2c_algorithm npcm_i2c_algo = { .master_xfer = npcm_i2c_master_xfer, .functionality = npcm_i2c_functionality, +#if IS_ENABLED(CONFIG_I2C_SLAVE) + .reg_slave = npcm_i2c_reg_slave, + .unreg_slave = npcm_i2c_unreg_slave, +#endif };static int npcm_i2c_probe_bus (struct platform_device *pdev)
Signature · 1 line
2.22.0
Re: [PATCH v9 3/3] i2c: npcm7xx: Add support for slave mode for Nuvoton
Quoted 2 lines · Tali Perry
Add support for slave mode for Nuvoton
NPCM BMC I2C controller driver.
Same comments apply as per previous patch.
...
Quoted 1 line
+#if
IS_ENABLED(CONFIG_I2C_SLAVE)
Perhaps create a separate module and compile it when user selects? See I²C DesignWare split.
...
drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_reset(struct npcm_i2c *bus) · line 669
Quoted 5 lines
drivers/i2c/busses/i2c-npcm7xx.c +5
-0+#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) { +npcm_i2c_slave_enable_l (bus,I2C_SLAVE_ADDR1 , bus->slave->addr, + true); + }
Put it on one line and drop {} -- it will be much better to read.
Quoted 1 line
+#endif
...
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c +12
-0+static void npcm_i2c_write_to_fifo_slave (struct npcm_i2c *bus, +u16 max_bytes_to_send) +{ +// Fill the FIFO, while the FIFO is not full and there are more bytes to +// write +npcm_i2c_clear_fifo_int (bus); +npcm_i2c_clear_tx_fifo (bus); +iowrite8 (0 , bus->reg +NPCM_I2CTXF_CTL ); + +while ((max_bytes_to_send--) && (I2C_HW_FIFO_SIZE - +npcm_i2c_get_fifo_fullness (bus))) { +if (bus->slv_wr_size >0 ) {
drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus, · line 792
Quoted 10 lines
drivers/i2c/busses/i2c-npcm7xx.c +10
-0+npcm_i2c_wr_byte (bus, + bus->slv_wr_buf[bus->slv_wr_ind % +I2C_HW_FIFO_SIZE ]); + bus->slv_wr_ind = (bus->slv_wr_ind +1 ) % +I2C_HW_FIFO_SIZE ; + bus->slv_wr_size--;// size indicates the # of bytes in +// the SW FIFO, not HW. + }else { +break ; + }
This looks ugly. Can you redo the style.
Quoted 2 lines
(quoted diff) +2
-0+ } +}
...
drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_read_from_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo) · line 864
Quoted 12 lines
drivers/i2c/busses/i2c-npcm7xx.c +12
-0+#if IS_ENABLED (CONFIG_I2C_SLAVE ) +if (bus->slave) { + bus->slv_rd_buf[bus->slv_rd_ind % +I2C_HW_FIFO_SIZE ] = data; + bus->slv_rd_ind++; +if (bus->slv_rd_ind ==1 && bus->read_block_use) +// 1st byte is length in block protocol + bus->slv_rd_size = data + + (u8 )bus->PEC_use + + (u8 )bus->read_block_use; + } +#endif
Ditto.
...
drivers/i2c/busses/i2c-npcm7xx.c static u32 npcm_i2c_functionality(struct i2c_adapter *adap) · line 2295
Quoted 6 lines
drivers/i2c/busses/i2c-npcm7xx.c +3
-0I2C_FUNC_SMBUS_EMUL |I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_PEC +#if IS_ENABLED(CONFIG_I2C_SLAVE) + |I2C_FUNC_SLAVE +#endif
Completely inconsistent style.
Quoted 1 line
;
Signature · 2 lines
With Best Regards, Andy Shevchenko