i2c: new bus driver for efm32

PATCH Superseded Uwe Kleine-König 3 participants 8 messages Mar 21, 2014 – Mar 25, 2014 1 message not in the archive

Subsystems (2)

Device tree bindings I2C subsystem host drivers

lore

Share & tools

lore

Uwe Kleine-König

author

2014-03-21 15:03 UTC #0 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

This was tested on a EFM32GG-DK3750 devboard that has a temperature sensor and an eeprom on its i2c bus.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---

Notes:
Changes since (implicit) v1, sent with Message-Id: 1392027598-29015-1-git-send-email-u.kleine-koenig at pengutronix.de
- drop non-dt support
- drop locking
- don't only use -EIO on error
- drop many dev_dbg
- use reinit_completion instead of (repeated) init_completion
- fix .remove callback to also remove the i2c adapter
- use -EINVAL for clock problems
- use namespace for location property
- don't BUG but reset hardware on unexpected failure
- add dt binding doc

Changes since v2, sent with Message-Id: 1395349551-27047-1-git-send-email-u.kleine-koenig at pengutronix.de
- make it actually compile :-|

 .../devicetree/bindings/i2c/i2c-efm32.txt          |  34 ++
 drivers/i2c/busses/Kconfig                         |   7 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-efm32.c                     | 482 +++++++++++++++++++++
 4 files changed, 524 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-efm32.txt
 create mode 100644 drivers/i2c/busses/i2c-efm32.c

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +34 -0

new file mode 100644
index 000000000000..ead4dd33ab66
@@ -0,0 +1,34 @@
+* Energymicro efm32 i2c controller
+
+Required properties :
+
+ - reg : Offset and length of the register set for the device
+ - compatible : should be "efm32,i2c"
+ - interrupts : the interrupt number
+ - clocks : reference to the module clock
+
+Recommended properties :
+
+ - clock-frequency : maximal I2C bus clock frequency in Hz.
+ - efm32,location : Decides the location of the USART I/O pins.
+   Allowed range : [0 .. 6]
+
+Example:
+	i2c0: i2c at 4000a000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "efm32,i2c";
+		reg = <0x4000a000 0x400>;
+		interrupts = <9>;
+		clocks = <&cmu clk_HFPERCLKI2C0>;
+		clock-frequency = <100000>;
+		status = "disabled";
+		efm32,location = <3>;
+
+		eeprom at 50 {
+			compatible = "microchip,24c02";
+			reg = <0x50>;
+			pagesize = <16>;
+		};
+	};
+

drivers/i2c/busses/Kconfig +7 -0

index f5ed03164d86..7322ff0c4c60 100644
@@ -432,6 +432,13 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_EFM32
+	tristate "EFM32 I2C controller"
+	depends on OF && (ARCH_EFM32 || COMPILE_TEST)
+	help
+	  This driver supports the i2c block found in Energy Micro's EFM32
+	  SoCs.
+
 config I2C_EG20T
 	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
 	depends on PCI

drivers/i2c/busses/Makefile +1 -0

index a08931fe73e1..2a56ab181851 100644
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o

drivers/i2c/busses/i2c-efm32.c new file +482 -0

482 lines not on this page · raw message

Signature · 1 line

1.9.1

Wolfram Sang

2014-03-24 17:01 UTC #1 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

Documentation/devicetree/bindings/i2c/i2c-efm32.txt line 6

Quoted 9 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +6 -0

@@ -0,0 +1,34 @@
+* Energymicro efm32 i2c controller
+
+Required properties :
+
+ - reg : Offset and length of the register set for the device
+ - compatible : should be "efm32,i2c"

Ehrm, come to think of it, shouldn't that actually be "energymicro, efm32-i2c" or similar to match the "vendor,product" pattern?

Documentation/devicetree/bindings/i2c/i2c-efm32.txt line 9

Quoted 3 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +3 -0
+config I2C_EFM32
+	tristate "EFM32 I2C controller"
+	depends on OF && (ARCH_EFM32 || COMPILE_TEST)

Is EFM32 DT only? Do we need the dependency on OF?

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +4 -0
+	help
+	  This driver supports the i2c block found in Energy Micro's EFM32
+	  SoCs.
+

...

Documentation/devicetree/bindings/i2c/i2c-efm32.txt

Quoted 3 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +3 -0
+static void efm32_i2c_send_next_byte(struct efm32_i2c_ddata *ddata)
+{
+	struct i2c_msg *cur_msg = &ddata->msgs[ddata->current_msg];

Missing newline.

Quoted 17 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +17 -0

+	if (ddata->current_word >= cur_msg->len) {
+		/* cur_msg completely transferred */
+		ddata->current_word = 0;
+		ddata->current_msg += 1;
+
+		if (ddata->current_msg >= ddata->num_msgs) {
+			efm32_i2c_write32(ddata, REG_CMD, REG_CMD_STOP);
+			complete(&ddata->done);
+		} else {
+			efm32_i2c_send_next_msg(ddata);
+		}
+	} else {
+		efm32_i2c_write32(ddata, REG_TXDATA,
+				cur_msg->buf[ddata->current_word++]);
+	}
+}
+

...

Quoted 21 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +21 -0

+static int efm32_i2c_master_xfer(struct i2c_adapter *adap,
+		struct i2c_msg *msgs, int num)
+{
+	struct efm32_i2c_ddata *ddata = i2c_get_adapdata(adap);
+	int ret;
+
+	if (ddata->msgs)
+		return -EBUSY;
+
+	ddata->msgs = msgs;
+	ddata->num_msgs = num;
+	ddata->current_word = 0;
+	ddata->current_msg = 0;
+	ddata->retval = -EIO;
+
+	reinit_completion(&ddata->done);
+
+	dev_dbg(&ddata->adapter.dev, "state: %08x, status: %08x\n",
+			efm32_i2c_read32(ddata, REG_STATE),
+			efm32_i2c_read32(ddata, REG_STATUS));
+

Documentation/devicetree/bindings/i2c/i2c-efm32.txt

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +10 -0

+	efm32_i2c_send_next_msg(ddata);
+
+	wait_for_completion(&ddata->done);
+
+	if (ddata->current_msg >= ddata->num_msgs)
+		ret = ddata->num_msgs;
+	else
+		ret = ddata->retval;
+
+	ddata->msgs = NULL;

Setting NULL should not be needed?

Documentation/devicetree/bindings/i2c/i2c-efm32.txt

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +4 -0
+
+	return ret;
+}
+

And checkpatch said:

WARNING: braces {} are not necessary for any arm of this statement #345: FILE: drivers/i2c/busses/i2c-efm32.c:239:
+ if (cur_msg->flags & I2C_M_RD) {
...

Thanks,

Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140324/fa97762e/attachment.sig>

Uwe Kleine-König

author

2014-03-24 18:41 UTC #2 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

Quoted 4 lines · Wolfram Sang

Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +1 -0
@@ -0,0 +1,34 @@
+* Energymicro efm32 i2c controller

Quoted 8 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +5 -0
+
+Required properties :
+
+ - reg : Offset and length of the register set for the device
+ - compatible : should be "efm32,i2c"
Ehrm, come to think of it, shouldn't that actually be "energymicro, efm32-i2c" or similar to match the "vendor,product" pattern?

yes.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt line 9
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +3 -0
+config I2C_EFM32
+	tristate "EFM32 I2C controller"
+	depends on OF && (ARCH_EFM32 || COMPILE_TEST)
Is EFM32 DT only? Do we need the dependency on OF?

yes, efm32 is DT only. Still for the COMPILE_TEST branch the OF is needed.

Quoted 32 lines

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +4 -0
+	help
+	  This driver supports the i2c block found in Energy Micro's EFM32
+	  SoCs.
+

...

Quoted 25 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +25 -0

+static int efm32_i2c_master_xfer(struct i2c_adapter *adap,
+		struct i2c_msg *msgs, int num)
+{
+	struct efm32_i2c_ddata *ddata = i2c_get_adapdata(adap);
+	int ret;
+
+	if (ddata->msgs)
+		return -EBUSY;
+
+	ddata->msgs = msgs;
+	ddata->num_msgs = num;
+	ddata->current_word = 0;
+	ddata->current_msg = 0;
+	ddata->retval = -EIO;
+
+	reinit_completion(&ddata->done);
+
+	dev_dbg(&ddata->adapter.dev, "state: %08x, status: %08x\n",
+			efm32_i2c_read32(ddata, REG_STATE),
+			efm32_i2c_read32(ddata, REG_STATUS));
+
+	efm32_i2c_send_next_msg(ddata);
+
+	wait_for_completion(&ddata->done);
+

Quoted 8 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt
Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +6 -0
+	if (ddata->current_msg >= ddata->num_msgs)
+		ret = ddata->num_msgs;
+	else
+		ret = ddata->retval;
+
+	ddata->msgs = NULL;
Setting NULL should not be needed?

Right, that's a left over from the locking stuff.

drivers/i2c/busses/i2c-efm32.c line 239

Quoted 11 lines

Documentation/devicetree/bindings/i2c/i2c-efm32.txt
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-efm32.txt new file +4 -0
+
+	return ret;
+}
+
And checkpatch said:
WARNING: braces {} are not necessary for any arm of this statement #345: FILE: drivers/i2c/busses/i2c-efm32.c:239:
drivers/i2c/busses/i2c-efm32.c new file +1 -0
+		if (cur_msg->flags & I2C_M_RD) {
...

ok.

Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Wolfram Sang

2014-03-24 19:08 UTC #3 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

Quoted 8 lines

Quoted 6 lines
drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 437
Quoted 4 lines
drivers/i2c/busses/Kconfig +3 -0
+config I2C_EFM32
+	tristate "EFM32 I2C controller"
+	depends on OF && (ARCH_EFM32 || COMPILE_TEST)
Is EFM32 DT only? Do we need the dependency on OF?
yes, efm32 is DT only. Still for the COMPILE_TEST branch the OF is needed.

Really? of_* functions should have empty templates if OF is not selected, I'd think.

Uwe Kleine-König

author

2014-03-25 09:07 UTC #4 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

In reply to Wolfram Sang

Hello,

drivers/i2c/busses/i2c-efm32.c line 239

Quoted 6 lines · Wolfram Sang

And checkpatch said:
WARNING: braces {} are not necessary for any arm of this statement #345: FILE: drivers/i2c/busses/i2c-efm32.c:239:
drivers/i2c/busses/i2c-efm32.c new file +1 -0
+		if (cur_msg->flags & I2C_M_RD) {
...

hmm the whole block reads:

        case REG_STATE_STATE_DATA:
                if (cur_msg->flags & I2C_M_RD) {
                        efm32_i2c_recv_next_byte(ddata);
                } else {
                        /* wait for Ack or Nack of slave */
                }
                break;

so just removing the braces does the wrong thing. Is this a false positive checkpatch warning? Andy? Joe?

I can make it:

		if (cur_msg->flags & I2C_M_RD)
			efm32_i2c_recv_next_byte(ddata);
		/* in the write case wait for Ack or Nack of slave */

but I consider the version above more readable.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Wolfram Sang

2014-03-25 09:11 UTC #5 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

Quoted 22 lines · Uwe Kleine-K?nig

Hello,
drivers/i2c/busses/i2c-efm32.c line 239
Quoted 6 lines · Wolfram Sang
And checkpatch said:
WARNING: braces {} are not necessary for any arm of this statement #345: FILE: drivers/i2c/busses/i2c-efm32.c:239:
drivers/i2c/busses/i2c-efm32.c new file +1 -0
+		if (cur_msg->flags & I2C_M_RD) {
...
hmm the whole block reads:
        case REG_STATE_STATE_DATA:
                if (cur_msg->flags & I2C_M_RD) {
                        efm32_i2c_recv_next_byte(ddata);
                } else {
                        /* wait for Ack or Nack of slave */
                }
                break;
so just removing the braces does the wrong thing. Is this a false positive checkpatch warning? Andy? Joe?

Quoted 7 lines

I can make it:
		if (cur_msg->flags & I2C_M_RD)
			efm32_i2c_recv_next_byte(ddata);
		/* in the write case wait for Ack or Nack of slave */
but I consider the version above more readable.

Well, OK, I let you decide.

A message that is not in the archive (sent off-list or not yet received): CAO=4fJM9y6yML77oJ3uW2Wz=nEhm=xA-bKoMHd8DXdxw0z8p7Q@mail.gmail.com

Wolfram Sang

2014-03-25 12:00 UTC #7 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

Quoted 3 lines · Andy Whitcroft

I would call it a false positive and ignore it.=A0 You need to justify any reports from checkpatch that remain "this is more readable" seems a valid justification.

I agree in general. In this case, I think both options have their pros and cons in terms of readability, so I let the author decide.

Joe Perches

2014-03-25 14:01 UTC #8 lore raw

PATCH v3 1/1 i2c: new bus driver for efm32

In reply to Wolfram Sang

Quoted 25 lines · Wolfram Sang

Quoted 24 lines · Uwe Kleine-K?nig
Hello,
drivers/i2c/busses/i2c-efm32.c line 239
Quoted 6 lines · Wolfram Sang
And checkpatch said:
WARNING: braces {} are not necessary for any arm of this statement #345: FILE: drivers/i2c/busses/i2c-efm32.c:239:
drivers/i2c/busses/i2c-efm32.c new file +1 -0
+		if (cur_msg->flags & I2C_M_RD) {
...
hmm the whole block reads:
        case REG_STATE_STATE_DATA:
                if (cur_msg->flags & I2C_M_RD) {
                        efm32_i2c_recv_next_byte(ddata);
                } else {
                        /* wait for Ack or Nack of slave */
                }
                break;
so just removing the braces does the wrong thing. Is this a false positive checkpatch warning? Andy? Joe?
I can make it:

Quoted 7 lines

Quoted 5 lines
		if (cur_msg->flags & I2C_M_RD)
			efm32_i2c_recv_next_byte(ddata);
		/* in the write case wait for Ack or Nack of slave */
but I consider the version above more readable.
Well, OK, I let you decide.

A Semi-Positive I think.

I think your first block is more readable myself.

Adding a semicolon would shut checkpatch up and it's debatable whether it might be better too.

	if (foo) {
		bar();
	} else {
		/* single line comment */
		;
	}

But please feel free to ignore any checkpatch warning where your taste is better than its.

checkpatch is brainless, you're not.