devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Add I2C support to ST SoCs
@ 2013-09-18 10:01 Maxime COQUELIN
  2013-09-18 10:01 ` [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller Maxime COQUELIN
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 10:01 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c
  Cc: stephen.gallimore, stuart.menefy, Lee Jones, gabriel.fernandez,
	olivier.clergeaud, Maxime Coquelin

The goal of this series is to add I2C support to ST SoCs.
The DT definition is added for STiH415 and STiH416 SoCs on
B2000 and B2020 boards.

Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>

Maxime Coquelin (4):
  i2c: busses: i2c-st: Add ST I2C controller
  ARM: STi: Supply I2C configuration to STiH416 SoC
  ARM: STi: Supply I2C configuration to STiH415 SoC
  ARM: STi: Add I2C config to B2000 and B2020 boards

 Documentation/devicetree/bindings/i2c/i2c-st.txt |   35 ++
 arch/arm/boot/dts/stih415-pinctrl.dtsi           |   36 ++
 arch/arm/boot/dts/stih415.dtsi                   |   57 ++
 arch/arm/boot/dts/stih416-pinctrl.dtsi           |   35 ++
 arch/arm/boot/dts/stih416.dtsi                   |   57 ++
 arch/arm/boot/dts/stih41x-b2000.dtsi             |    7 +
 arch/arm/boot/dts/stih41x-b2020.dtsi             |   20 +
 drivers/i2c/busses/Kconfig                       |   10 +
 drivers/i2c/busses/Makefile                      |    1 +
 drivers/i2c/busses/i2c-st.c                      |  713 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-st.h                      |  216 +++++++
 11 files changed, 1187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
 create mode 100644 drivers/i2c/busses/i2c-st.c
 create mode 100644 drivers/i2c/busses/i2c-st.h

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-09-18 10:01 [PATCH 0/4] Add I2C support to ST SoCs Maxime COQUELIN
@ 2013-09-18 10:01 ` Maxime COQUELIN
  2013-09-18 12:47   ` Gabriel FERNANDEZ
       [not found]   ` <1379498483-4236-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
       [not found] ` <1379498483-4236-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 10:01 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c
  Cc: stephen.gallimore, stuart.menefy, Lee Jones, gabriel.fernandez,
	olivier.clergeaud, Maxime Coquelin

This patch adds support to SSC (Synchronous Serial Controller)
I2C driver. This IP also supports SPI protocol, but this is not
the aim of this driver.

This IP is embedded in all ST SoCs for Set-top box platorms, and
supports I2C Standard and Fast modes.

Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 Documentation/devicetree/bindings/i2c/i2c-st.txt |   35 ++
 drivers/i2c/busses/Kconfig                       |   10 +
 drivers/i2c/busses/Makefile                      |    1 +
 drivers/i2c/busses/i2c-st.c                      |  713 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-st.h                      |  216 +++++++
 5 files changed, 975 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
 create mode 100644 drivers/i2c/busses/i2c-st.c
 create mode 100644 drivers/i2c/busses/i2c-st.h

diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
new file mode 100644
index 0000000..a7d6381
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
@@ -0,0 +1,35 @@
+I2C for ST platforms
+
+Required properties :
+- compatible : Must be "st,comms-i2c"
+- reg : Offset and length of the register set for the device
+- interrupts : the interrupt number
+- clocks : phandle to the I2C clock source
+
+Recommended (non-standard) properties :
+- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
+  the default 100 kHz frequency will be used. As only Normal and Fast modes
+  are supported, possible values are 100000 and 400000.
+
+Optional (non-standard) properties:
+- st,glitches : Enable timing glitch suppression support.
+- st,glitch-clk : SCL line timinig glitch suppression value in ns.
+- st,glitch-dat : SDA line timinig glitch suppression value in ns.
+- st,hw-glitches : Enable filter glitch suppression support.
+- st,hw-glitch-clk : SCL line filter glitch suppression value in us.
+- st,hw-glitch-dat : SDA line filter glitch suppression value in us.
+
+Examples :
+
+i2c0: i2c@fed40000{
+	compatible	= "st,comms-i2c";
+	reg		= <0xfed40000 0x110>;
+	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+	clocks		= <&CLK_S_ICN_REG_0>;
+	clock-frequency = <400000>;
+	pinctrl-names	= "default";
+	pinctrl-0	= <&pinctrl_i2c0_default>;
+	st,glitches;
+	st,glitch-clk	= 500;
+	st,glitch-dat	= 500;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cdcbd83..18ff129 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -776,6 +776,16 @@ config I2C_RCAR
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-rcar.
 
+config I2C_ST
+	tristate "STMicroelectronics SSC I2C support"
+	depends on ARCH_STI
+	help
+	  Enable this option to add support for STMicroelectronics SoCs
+	  hardware SSC (Synchronous Serial Controller) as an I2C controller.
+
+	  This driver can also be built as module. If so, the module
+	  will be called i2c-st.
+
 comment "External I2C/SMBus adapter drivers"
 
 config I2C_DIOLAN_U2C
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index d00997f..9ea5d422 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -92,5 +92,6 @@ obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_SCx200_I2C)	+= scx200_i2c.o
+obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
new file mode 100644
index 0000000..fa4451f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-st.c
@@ -0,0 +1,713 @@
+/*
+ * Copyright (C) 2013 STMicroelectronics
+ *
+ * I2C master mode controller driver, used in STMicroelectronics devices.
+ *
+ * Author: Maxime Coquelin <maxime.coquelin@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include "i2c-st.h"
+
+#define DRIVER_NAME "st-i2c"
+
+static struct st_i2c_timings i2c_timings[] = {
+	[I2C_MODE_STANDARD] = {
+		.rate			= 100000,
+		.rep_start_hold		= 4000,
+		.rep_start_setup	= 4700,
+		.start_hold		= 4000,
+		.data_setup_time	= 250,
+		.stop_setup_time	= 4000,
+		.bus_free_time		= 4700,
+		.glitch_dat_max		= 1200,
+	},
+	[I2C_MODE_FAST] = {
+		.rate			= 400000,
+		.rep_start_hold		= 600,
+		.rep_start_setup	= 600,
+		.start_hold		= 1000,
+		.data_setup_time	= 500,
+		.stop_setup_time	= 900,
+		.bus_free_time		= 1300,
+		.glitch_dat_max		= 200,
+	},
+};
+
+static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	int count, i;
+
+	/*
+	 * Counter only counts up to 7 but fifo size is 8...
+	 * When fifo is full, counter is 0 and RIR bit of status register is
+	 * set
+	 */
+	if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
+		count = SSC_RXFIFO_SIZE;
+	else
+		count = readl(i2c_dev->base + SSC_RX_FSTAT) &
+			SSC_RX_FSTAT_STATUS;
+
+	for (i = 0; i < count; i++)
+		readl(i2c_dev->base + SSC_RBUF);
+}
+
+static void st_i2c_soft_reset(struct st_i2c_dev *i2c_dev)
+{
+	/*
+	 * FIFO needs to be emptied before reseting the IP,
+	 * else the controller raises a BUSY error.
+	 */
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+	st_i2c_clr_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+}
+
+/**
+ * st_i2c_hw_config() - Prepare SSC block, calculate and apply tuning timings
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_hw_config(struct st_i2c_dev *i2c_dev)
+{
+	unsigned long rate;
+	u32 val, ns_per_clk;
+	struct st_i2c_timings *t = &i2c_timings[i2c_dev->mode];
+	struct st_i2c_glitches *g = &i2c_dev->glitches;
+
+	st_i2c_soft_reset(i2c_dev);
+
+	val = SSC_CLR_REPSTRT | SSC_CLR_NACK | SSC_CLR_SSCARBL |
+		SSC_CLR_SSCAAS | SSC_CLR_SSCSTOP;
+	writel(val, i2c_dev->base + SSC_CLR);
+
+	/* SSC Control register setup */
+	val = SSC_CTL_PO | SSC_CTL_PH | SSC_CTL_HB | SSC_CTL_DATA_WIDTH_9;
+	writel(val, i2c_dev->base + SSC_CTL);
+
+	rate = clk_get_rate(i2c_dev->clk);
+	ns_per_clk = 1000000000 / rate;
+
+	/* Baudrate */
+	val = rate / (2 * t->rate);
+	writel(val, i2c_dev->base + SSC_BRG);
+
+	/* Pre-scaler baudrate */
+	writel(1, i2c_dev->base + SSC_PRE_SCALER_BRG);
+
+	/* Enable I2C mode */
+	writel(SSC_I2C_I2CM, i2c_dev->base + SSC_I2C);
+
+	/* Repeated start hold time */
+	val = (t->rep_start_hold + g->glitch_dat) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_HOLD);
+
+	/* Repeated start set up time */
+	val = (t->rep_start_setup + g->glitch_clk) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_SETUP);
+
+	/* Start hold time */
+	if (g->glitch_dat < t->glitch_dat_max)
+		val = (t->start_hold + g->glitch_dat) / ns_per_clk;
+	else
+		val = (t->start_hold + t->glitch_dat_max) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_START_HOLD);
+
+	/* Data set up time */
+	val = (t->data_setup_time + g->glitch_dat) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_DATA_SETUP);
+
+	/* Stop set up time */
+	val = (t->stop_setup_time + g->glitch_clk) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_STOP_SETUP);
+
+	/* Bus free time */
+	val = (t->bus_free_time + g->glitch_dat) / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_BUS_FREE);
+
+	if (!g->hw_glitches) {
+		writel(0, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
+		return;
+	}
+
+	/* Prescalers set up */
+	val = rate / 10000000;
+	writel(val, i2c_dev->base + SSC_PRSCALER);
+	writel(val, i2c_dev->base + SSC_PRSCALER_DATAOUT);
+
+	/* Noise suppression witdh */
+	val = g->hw_glitch_clk * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
+
+	/* Noise suppression max output data delay width */
+	val = g->hw_glitch_dat * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH_DATAOUT);
+
+	return;
+}
+
+static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
+{
+	u32 sta;
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		sta = readl(i2c_dev->base + SSC_STA);
+		if (!(sta & SSC_STA_BUSY))
+			return 0;
+
+		usleep_range(2000, 4000);
+	}
+
+	dev_err(i2c_dev->dev, "bus not free (status = 0x%08x)\n", sta);
+
+	return -EBUSY;
+}
+
+/**
+ * st_i2c_write_tx_fifo() - Write a byte in the Tx FIFO
+ * @i2c_dev: Controller's private data
+ * @byte: Data to write in the Tx FIFO
+ */
+static inline void st_i2c_write_tx_fifo(struct st_i2c_dev *i2c_dev, u8 byte)
+{
+	u16 tbuf = byte << 1;
+
+	writel(tbuf | 1, i2c_dev->base + SSC_TBUF);
+}
+
+/**
+ * st_i2c_wr_fill_tx_fifo() - Fill the Tx FIFO in write mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with I2C transfert buffer when
+ * in write mode.
+ */
+static void st_i2c_wr_fill_tx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (c->count < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = c->count;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->count--, c->buf++)
+		st_i2c_write_tx_fifo(i2c_dev, *c->buf);
+}
+
+/**
+ * st_i2c_rd_fill_tx_fifo() - Fill the Tx FIFO in read mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with fixed pattern when
+ * in read mode to trigger clock.
+ */
+static void st_i2c_rd_fill_tx_fifo(struct st_i2c_dev *i2c_dev, int max)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (max < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = max;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->xfered++)
+		st_i2c_write_tx_fifo(i2c_dev, 0xff);
+}
+
+static void st_i2c_read_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 i, sta;
+	u16 rbuf;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_RIR) {
+		i = SSC_RXFIFO_SIZE;
+	} else {
+		i = readl(i2c_dev->base + SSC_RX_FSTAT);
+		i &= SSC_RX_FSTAT_STATUS;
+	}
+
+	for (; (i > 0) && (c->count > 0); i--, c->count--) {
+		rbuf = readl(i2c_dev->base + SSC_RBUF) >> 1;
+		*c->buf++ = (u8)rbuf & 0xff;
+	}
+
+	if (i) {
+		dev_err(i2c_dev->dev, "Unexpected %d bytes in rx fifo\n", i);
+		st_i2c_flush_rx_fifo(i2c_dev);
+	}
+}
+
+/**
+ * st_i2c_terminate_xfer() - Send either STOP or REPSTART condition
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_terminate_xfer(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_clr_bits(i2c_dev->base + SSC_IEN, SSC_IEN_TEEN);
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+
+	if (c->stop) {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_STOPEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+	} else {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_REPSTRTEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_REPSTRTG);
+	}
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO empty interrupt in case of write
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_write(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	if (!c->count)
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	else
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ien;
+
+	/* Trash the address read back */
+	if (!c->xfered) {
+		readl(i2c_dev->base + SSC_RBUF);
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
+	} else
+		st_i2c_read_rx_fifo(i2c_dev);
+
+	if (!c->count) {
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	} else if (c->count == 1) {
+		/* Penultimate byte to xfer, disable ACK gen. */
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
+
+		/* Last received byte is to be handled by NACK interrupt */
+		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
+		writel(ien, i2c_dev->base + SSC_IEN);
+
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
+	} else
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
+}
+
+/**
+ * st_i2c_isr() - Interrupt routine
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t st_i2c_isr(int irq, void *data)
+{
+	struct st_i2c_dev *i2c_dev = data;
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 sta, ien, it;
+
+	ien = readl(i2c_dev->base + SSC_IEN);
+	sta = readl(i2c_dev->base + SSC_STA);
+
+	/* Use __fls() to check error bits first */
+	it = __fls(sta & ien);
+
+	switch (1 << it) {
+	case SSC_STA_TE:
+		if (c->addr & I2C_M_RD)
+			st_i2c_handle_read(i2c_dev);
+		else
+			st_i2c_handle_write(i2c_dev);
+		break;
+
+	case SSC_STA_STOP:
+	case SSC_STA_REPSTRT:
+		writel(0, i2c_dev->base + SSC_IEN);
+		complete(&i2c_dev->complete);
+		break;
+
+	case SSC_STA_NACK:
+		writel(SSC_CLR_NACK, i2c_dev->base + SSC_CLR);
+
+		/* Last received byte handled by NACK interrupt */
+		if ((c->addr & I2C_M_RD) && (c->count == 1) && (c->xfered)) {
+			st_i2c_handle_read(i2c_dev);
+			break;
+		}
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	case SSC_STA_ARBL:
+		writel(SSC_CLR_SSCARBL, i2c_dev->base + SSC_CLR);
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	default:
+		dev_err(i2c_dev->dev, "%s: it %d unhandled (status=0x%08x)\n",
+				__func__, it, readl(i2c_dev->base + SSC_STA));
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * st_i2c_xfer_msg() - Transfer a single I2C message
+ * @i2c_dev: Controller's private data
+ * @msg: I2C message to transfer
+ * @is_first: first message of the sequence
+ * @is_last: last message of the sequence
+ */
+static int st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
+			    bool is_first, bool is_last)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ctl, i2c, it;
+	unsigned long timeout;
+	int ret;
+
+	c->addr		= (u8)(msg->addr << 1);
+	c->addr		|= (msg->flags & I2C_M_RD);
+	c->buf		= msg->buf;
+	c->count	= msg->len;
+	c->xfered	= 0;
+	c->result	= 0;
+	c->stop		= is_last;
+
+	init_completion(&i2c_dev->complete);
+
+	ctl = SSC_CTL_EN | SSC_CTL_MS |	SSC_CTL_EN_RX_FIFO | SSC_CTL_EN_TX_FIFO;
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, ctl);
+
+	i2c = SSC_I2C_TXENB;
+	if (c->addr & I2C_M_RD)
+		i2c |= SSC_I2C_ACKG;
+	st_i2c_set_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	/* Write slave address */
+	st_i2c_write_tx_fifo(i2c_dev, c->addr);
+
+	/* Pre-fill Tx fifo with data in case of write */
+	if (!(c->addr & I2C_M_RD))
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+
+	it = SSC_IEN_NACKEN | SSC_IEN_TEEN | SSC_IEN_ARBLEN;
+	writel(it, i2c_dev->base + SSC_IEN);
+
+	if (is_first) {
+		ret = st_i2c_wait_free_bus(i2c_dev);
+		if (ret)
+			return ret;
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+	}
+
+	timeout = wait_for_completion_timeout(&i2c_dev->complete,
+			i2c_dev->adap.timeout);
+	ret = c->result;
+
+	if (!timeout) {
+		dev_err(i2c_dev->dev, "Write to slave 0x%x timed out\n",
+				c->addr);
+		ret = -ETIMEDOUT;
+	}
+
+	i2c = SSC_I2C_STOPG | SSC_I2C_REPSTRTG;
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	writel(SSC_CLR_SSCSTOP | SSC_CLR_REPSTRT, i2c_dev->base + SSC_CLR);
+
+	return ret;
+}
+
+/**
+ * st_i2c_xfer() - Transfer a single I2C message
+ * @i2c_adap: Adapter pointer to the controller
+ * @msgs: Pointer to data to be written.
+ * @num: Number of messages to be executed
+ */
+static int st_i2c_xfer(struct i2c_adapter *i2c_adap,
+			struct i2c_msg msgs[], int num)
+{
+	struct st_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+	bool last, first;
+	int ret, i;
+
+	i2c_dev->busy = true;
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+
+	st_i2c_hw_config(i2c_dev);
+
+	first = true;
+
+	for (i = 0; i < num; i++) {
+		last = (i < num - 1) ? false : true;
+
+		ret = st_i2c_xfer_msg(i2c_dev, &msgs[i], first, last);
+		if (ret)
+			break;
+
+		first = false;
+	}
+
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	clk_disable_unprepare(i2c_dev->clk);
+
+	i2c_dev->busy = false;
+
+	return (ret < 0) ? ret : i;
+}
+
+#ifdef CONFIG_PM
+static int st_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev =
+		container_of(dev, struct platform_device, dev);
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	if (i2c_dev->busy)
+		return -EBUSY;
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int st_i2c_resume(struct device *dev)
+{
+	pinctrl_pm_select_default_state(dev);
+	/* Go in idle state if available */
+	pinctrl_pm_select_idle_state(dev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(st_i2c_pm, st_i2c_suspend, st_i2c_resume);
+#define ST_I2C_PM	(&st_i2c_pm)
+#else
+#define ST_I2C_PM	NULL
+#endif
+
+static u32 st_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm st_i2c_algo = {
+	.master_xfer = st_i2c_xfer,
+	.functionality = st_i2c_func,
+};
+
+static int st_i2c_of_get_glitches(struct device_node *np,
+		struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_glitches *g = &i2c_dev->glitches;
+	int ret;
+
+	g->glitches = of_property_read_bool(np, "st,glitches");
+	if (g->glitches) {
+		ret = of_property_read_u32(np, "st,glitch-clk",
+				&g->glitch_clk);
+		if (ret) {
+			dev_err(i2c_dev->dev, "st,glitch-clk not found\n");
+			return ret;
+		}
+
+		ret = of_property_read_u32(np, "st,glitch-dat",
+				&g->glitch_dat);
+		if (ret) {
+			dev_err(i2c_dev->dev, "st,glitch-dat not found\n");
+			return ret;
+		}
+	}
+
+	g->hw_glitches = of_property_read_bool(np, "st,hw-glitches");
+	if (g->hw_glitches) {
+		ret = of_property_read_u32(np, "st,hw-glitch-clk",
+				&g->hw_glitch_clk);
+		if (ret) {
+			dev_err(i2c_dev->dev, "st,hw-glitch-clk not found\n");
+			return ret;
+		}
+
+		ret = of_property_read_u32(np, "st,hw-glitch-dat",
+				&g->hw_glitch_dat);
+		if (ret) {
+			dev_err(i2c_dev->dev, "st,hw-glitch-dat not found\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int st_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct st_i2c_dev *i2c_dev;
+	struct resource *res;
+	u32 clk_rate;
+	struct i2c_adapter *adap;
+	int ret;
+
+	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
+	if (!i2c_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	i2c_dev->irq = irq_of_parse_and_map(np, 0);
+	if (!i2c_dev->irq) {
+		dev_err(&pdev->dev, "IRQ missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = of_clk_get(np, 0);
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(&pdev->dev, "Unable to request clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+
+	i2c_dev->mode = I2C_MODE_STANDARD;
+	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
+	if ((!ret) && (clk_rate == 400000))
+		i2c_dev->mode = I2C_MODE_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
+			pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+	/* In case idle state available, select it */
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	ret = st_i2c_of_get_glitches(np, i2c_dev);
+	if (ret)
+		return ret;
+
+	adap = &i2c_dev->adap;
+	i2c_set_adapdata(adap, i2c_dev);
+	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
+	adap->owner = THIS_MODULE;
+	adap->timeout = 2 * HZ;
+	adap->retries = 0;
+	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
+	adap->algo = &st_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	ret = i2c_add_adapter(adap);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add adapter\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
+
+	return 0;
+}
+
+static int st_i2c_remove(struct platform_device *pdev)
+{
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c_dev->adap);
+
+	return 0;
+}
+
+static struct of_device_id st_i2c_match[] = {
+	{ .compatible = "st,comms-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_i2c_match);
+
+static struct platform_driver st_i2c_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = st_i2c_match,
+		.pm = &st_i2c_pm,
+	},
+	.probe = st_i2c_probe,
+	.remove = st_i2c_remove,
+};
+
+module_platform_driver(st_i2c_driver);
+
+MODULE_AUTHOR("STMicroelectronics");
+MODULE_DESCRIPTION("STMicroelectronics I2C driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-st.h b/drivers/i2c/busses/i2c-st.h
new file mode 100644
index 0000000..7bce6c0
--- /dev/null
+++ b/drivers/i2c/busses/i2c-st.h
@@ -0,0 +1,216 @@
+/*
+ * Copyright (C) 2013 STMicroelectronics
+ *
+ * I2C master mode controller driver, used in STMicroelectronics devices.
+ *
+ * Author: Maxime Coquelin <maxime.coquelin@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __I2C_ST_H_
+#define __I2C_ST_H_
+
+/* SSC registers */
+#define SSC_BRG				0x000
+#define SSC_TBUF			0x004
+#define SSC_RBUF			0x008
+#define SSC_CTL				0x00C
+#define SSC_IEN				0x010
+#define SSC_STA				0x014
+#define SSC_I2C				0x018
+#define SSC_SLAD			0x01C
+#define SSC_REP_START_HOLD		0x020
+#define SSC_START_HOLD			0x024
+#define SSC_REP_START_SETUP		0x028
+#define SSC_DATA_SETUP			0x02C
+#define SSC_STOP_SETUP			0x030
+#define SSC_BUS_FREE			0x034
+#define SSC_TX_FSTAT			0x038
+#define SSC_RX_FSTAT			0x03C
+#define SSC_PRE_SCALER_BRG		0x040
+#define SSC_CLR				0x080
+#define SSC_NOISE_SUPP_WIDTH		0x100
+#define SSC_PRSCALER			0x104
+#define SSC_NOISE_SUPP_WIDTH_DATAOUT	0x108
+#define SSC_PRSCALER_DATAOUT		0x10c
+
+/* SSC Control */
+#define SSC_CTL_DATA_WIDTH_9		0x8
+#define SSC_CTL_DATA_WIDTH_MSK		0xf
+#define SSC_CTL_BM			0xf
+#define SSC_CTL_HB			BIT(4)
+#define SSC_CTL_PH			BIT(5)
+#define SSC_CTL_PO			BIT(6)
+#define SSC_CTL_SR			BIT(7)
+#define SSC_CTL_MS			BIT(8)
+#define SSC_CTL_EN			BIT(9)
+#define SSC_CTL_LPB			BIT(10)
+#define SSC_CTL_EN_TX_FIFO		BIT(11)
+#define SSC_CTL_EN_RX_FIFO		BIT(12)
+#define SSC_CTL_EN_CLST_RX		BIT(13)
+
+/* SSC Interrupt Enable */
+#define SSC_IEN_RIEN			BIT(0)
+#define SSC_IEN_TIEN			BIT(1)
+#define SSC_IEN_TEEN			BIT(2)
+#define SSC_IEN_REEN			BIT(3)
+#define SSC_IEN_PEEN			BIT(4)
+#define SSC_IEN_AASEN			BIT(6)
+#define SSC_IEN_STOPEN			BIT(7)
+#define SSC_IEN_ARBLEN			BIT(8)
+#define SSC_IEN_NACKEN			BIT(10)
+#define SSC_IEN_REPSTRTEN		BIT(11)
+#define SSC_IEN_TX_FIFO_HALF		BIT(12)
+#define SSC_IEN_RX_FIFO_HALF_FULL	BIT(14)
+
+/* SSC Status */
+#define SSC_STA_RIR			BIT(0)
+#define SSC_STA_TIR			BIT(1)
+#define SSC_STA_TE			BIT(2)
+#define SSC_STA_RE			BIT(3)
+#define SSC_STA_PE			BIT(4)
+#define SSC_STA_CLST			BIT(5)
+#define SSC_STA_AAS			BIT(6)
+#define SSC_STA_STOP			BIT(7)
+#define SSC_STA_ARBL			BIT(8)
+#define SSC_STA_BUSY			BIT(9)
+#define SSC_STA_NACK			BIT(10)
+#define SSC_STA_REPSTRT			BIT(11)
+#define SSC_STA_TX_FIFO_HALF		BIT(12)
+#define SSC_STA_TX_FIFO_FULL		BIT(13)
+#define SSC_STA_RX_FIFO_HALF		BIT(14)
+
+/* SSC I2C Control */
+#define SSC_I2C_I2CM			BIT(0)
+#define SSC_I2C_STRTG			BIT(1)
+#define SSC_I2C_STOPG			BIT(2)
+#define SSC_I2C_ACKG			BIT(3)
+#define SSC_I2C_AD10			BIT(4)
+#define SSC_I2C_TXENB			BIT(5)
+#define SSC_I2C_REPSTRTG		BIT(11)
+#define SSC_I2C_SLAVE_DISABLE		BIT(12)
+
+/* SSC Tx FIFO Status */
+#define SSC_TX_FSTAT_STATUS		0x07
+
+/* SSC Rx FIFO Status */
+#define SSC_RX_FSTAT_STATUS		0x07
+
+/* SSC Clear bit operation */
+#define SSC_CLR_SSCAAS			BIT(6)
+#define SSC_CLR_SSCSTOP			BIT(7)
+#define SSC_CLR_SSCARBL			BIT(8)
+#define SSC_CLR_NACK			BIT(10)
+#define SSC_CLR_REPSTRT			BIT(11)
+
+/* SSC Clock Prescaler */
+#define SSC_PRSC_VALUE			0x0f
+
+
+#define SSC_TXFIFO_SIZE			0x8
+#define SSC_RXFIFO_SIZE			0x8
+
+enum st_i2c_mode {
+	I2C_MODE_STANDARD,
+	I2C_MODE_FAST,
+	I2C_MODE_END,
+};
+
+/**
+ * struct st_i2c_timings - per-Mode tuning parameters
+ * @rate: I2C bus rate
+ * @rep_start_hold: I2C repeated start hold time requirement
+ * @rep_start_setup: I2C repeated start set up time requirement
+ * @start_hold: I2C start hold time requirement
+ * @data_setup_time: I2C data set up time requirement
+ * @stop_setup_time: I2C stop set up time requirement
+ * @bus_free_time: I2C bus free time requirement
+ * @glitch_dat_max: I2C data glitch max time
+ */
+struct st_i2c_timings {
+	u32 rate;
+	u32 rep_start_hold;
+	u32 rep_start_setup;
+	u32 start_hold;
+	u32 data_setup_time;
+	u32 stop_setup_time;
+	u32 bus_free_time;
+	u32 glitch_dat_max;
+};
+
+/**
+ * struct st_i2c_glitches - per-SoC tuning parameters
+ * @glitches: Glitches are defined for this SoC
+ * @glitch_clk: Clk line glitch tuning value
+ * @glitch_dat: Data line glitch tuning value
+ * @hw_glitches: HW glitches are defined for this SoC
+ * @hw_glitch_clk: Clk line hw glitch value
+ * @hw_glitch_dat: Data line hw glitch value
+ */
+struct st_i2c_glitches {
+	bool	glitches;
+	u32	glitch_clk;
+	u32	glitch_dat;
+	bool	hw_glitches;
+	u32	hw_glitch_clk;
+	u32	hw_glitch_dat;
+};
+
+/**
+ * struct st_i2c_client - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transfered
+ * @xfered: number of bytes already transferred
+ * @buf: data buffer
+ * @result: result of the transfer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ */
+struct st_i2c_client {
+	u8	addr;
+	u32	count;
+	u32	xfered;
+	u8	*buf;
+	int	result;
+	bool	stop;
+};
+
+/**
+ * struct st_i2c_dev - private data of the controller
+ * @adap: I2C adapter for this controller
+ * @dev: device for this controller
+ * @base: virtual memory area
+ * @complete: completion of I2C message
+ * @irq: interrupt line for th controller
+ * @clk: hw ssc block clock
+ * @mode: I2C mode of the controller. Standard or Fast only supported
+ * @glitches: Glitch filters tuning parameters
+ * @client: I2C transfert information
+ * @busy: I2C transfer on-going
+ */
+struct st_i2c_dev {
+	struct i2c_adapter	adap;
+	struct device		*dev;
+	void __iomem		*base;
+	struct completion	complete;
+	int			irq;
+	struct clk		*clk;
+	int			mode;
+	struct st_i2c_glitches glitches;
+	struct st_i2c_client	client;
+	bool			busy;
+};
+
+static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) | mask, reg);
+}
+
+static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) & ~mask, reg);
+}
+
+#endif /* __I2C_ST_H_ */
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
       [not found] ` <1379498483-4236-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-09-18 10:01   ` Maxime COQUELIN
       [not found]     ` <1379498483-4236-3-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 10:01 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla-qxv4g6HH51o, Rob Herring,
	Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
	Rob Landley, Russell King, Grant Likely,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA
  Cc: stephen.gallimore-qxv4g6HH51o, stuart.menefy-qxv4g6HH51o,
	Lee Jones, gabriel.fernandez-qxv4g6HH51o,
	olivier.clergeaud-qxv4g6HH51o, Maxime Coquelin

This patch supplies I2C configuration to STiH416 SoC.

Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
 arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 0f246c9..b29ff4b 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -97,6 +97,24 @@
 					};
 				};
 			};
+
+			sbc_i2c0 {
+				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
+					st,pins {
+						sda = <&PIO4 6 ALT1 BIDIR>;
+						scl = <&PIO4 5 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			sbc_i2c1 {
+				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
+					st,pins {
+						sda = <&PIO3 2 ALT2 BIDIR>;
+						scl = <&PIO3 1 ALT2 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -175,6 +193,23 @@
 				};
 			};
 
+			i2c0 {
+				pinctrl_i2c0_default: i2c0-default {
+					st,pins {
+						sda = <&PIO9 3 ALT1 BIDIR>;
+						scl = <&PIO9 2 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			i2c1 {
+				pinctrl_i2c1_default: i2c1-default {
+					st,pins {
+						sda = <&PIO12 1 ALT1 BIDIR>;
+						scl = <&PIO12 0 ALT1 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 1a0326e..8856470 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -9,6 +9,7 @@
 #include "stih41x.dtsi"
 #include "stih416-clock.dtsi"
 #include "stih416-pinctrl.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 / {
 	L2: cache-controller {
 		compatible = "arm,pl310-cache";
@@ -92,5 +93,61 @@
 			pinctrl-0 	= <&pinctrl_sbc_serial1>;
 			clocks          = <&CLK_SYSIN>;
 		};
+
+		i2c0: i2c@fed40000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed40000 0x110>;
+			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		i2c1: i2c@fed41000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed41000 0x110>;
+			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c0: i2c@fe540000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe540000 0x110>;
+			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c1: i2c@fe541000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe541000 0x110>;
+			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
 	};
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC
  2013-09-18 10:01 [PATCH 0/4] Add I2C support to ST SoCs Maxime COQUELIN
  2013-09-18 10:01 ` [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller Maxime COQUELIN
       [not found] ` <1379498483-4236-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-09-18 10:01 ` Maxime COQUELIN
  2013-09-18 12:00   ` Lee Jones
  2013-09-18 10:01 ` [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards Maxime COQUELIN
  3 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 10:01 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c
  Cc: stephen.gallimore, stuart.menefy, Lee Jones, gabriel.fernandez,
	olivier.clergeaud, Maxime Coquelin

This patch supplies I2C configuration to STiH415 SoC.

Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 arch/arm/boot/dts/stih415-pinctrl.dtsi |   36 ++++++++++++++++++++
 arch/arm/boot/dts/stih415.dtsi         |   57 ++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/arch/arm/boot/dts/stih415-pinctrl.dtsi b/arch/arm/boot/dts/stih415-pinctrl.dtsi
index 1d322b2..e56449d 100644
--- a/arch/arm/boot/dts/stih415-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih415-pinctrl.dtsi
@@ -86,6 +86,24 @@
 					};
 				};
 			};
+
+			sbc_i2c0 {
+				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
+					st,pins {
+						sda = <&PIO4 6 ALT1 BIDIR>;
+						scl = <&PIO4 5 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			sbc_i2c1 {
+				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
+					st,pins {
+						sda = <&PIO3 2 ALT2 BIDIR>;
+						scl = <&PIO3 1 ALT2 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -143,6 +161,24 @@
 				reg		= <0x7000 0x100>;
 				st,bank-name	= "PIO12";
 			};
+
+			i2c0 {
+				pinctrl_i2c0_default: i2c0-default {
+					st,pins {
+						sda = <&PIO9 3 ALT1 BIDIR>;
+						scl = <&PIO9 2 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			i2c1 {
+				pinctrl_i2c1_default: i2c1-default {
+					st,pins {
+						sda = <&PIO12 1 ALT1 BIDIR>;
+						scl = <&PIO12 0 ALT1 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
index 74ab8de..643ae1c 100644
--- a/arch/arm/boot/dts/stih415.dtsi
+++ b/arch/arm/boot/dts/stih415.dtsi
@@ -9,6 +9,7 @@
 #include "stih41x.dtsi"
 #include "stih415-clock.dtsi"
 #include "stih415-pinctrl.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 / {
 
 	L2: cache-controller {
@@ -83,5 +84,61 @@
 			pinctrl-names 	= "default";
 			pinctrl-0	= <&pinctrl_sbc_serial1>;
 		};
+
+		i2c0: i2c@fed40000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed40000 0x110>;
+			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLKS_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		i2c1: i2c@fed41000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed41000 0x110>;
+			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLKS_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c0: i2c@fe540000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe540000 0x110>;
+			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c1: i2c@fe541000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe541000 0x110>;
+			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
 	};
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards
  2013-09-18 10:01 [PATCH 0/4] Add I2C support to ST SoCs Maxime COQUELIN
                   ` (2 preceding siblings ...)
  2013-09-18 10:01 ` [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC Maxime COQUELIN
@ 2013-09-18 10:01 ` Maxime COQUELIN
  2013-09-18 11:40   ` Lee Jones
  3 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 10:01 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c
  Cc: stephen.gallimore, stuart.menefy, Lee Jones, gabriel.fernandez,
	olivier.clergeaud, Maxime Coquelin

This patch supplies I2C configuration to B2000 and B2020
based on either STiH415 or STiH416 SoCs.

Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 arch/arm/boot/dts/stih41x-b2000.dtsi |    7 +++++++
 arch/arm/boot/dts/stih41x-b2020.dtsi |   20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/stih41x-b2000.dtsi b/arch/arm/boot/dts/stih41x-b2000.dtsi
index 8e694d2..346cc4bc 100644
--- a/arch/arm/boot/dts/stih41x-b2000.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2000.dtsi
@@ -37,5 +37,12 @@
 			};
 		};
 
+		/* HDMI Tx I2C */
+		i2c1: i2c@fed41000{
+			status = "okay";
+
+			/* HDMI V1.3a supports Standard mode only */
+			clock-frequency = <100000>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi b/arch/arm/boot/dts/stih41x-b2020.dtsi
index 133e181..10b98da 100644
--- a/arch/arm/boot/dts/stih41x-b2020.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
@@ -38,5 +38,25 @@
 				default-state = "off";
 			};
 		};
+
+		i2c0: i2c@fed40000{
+			status = "okay";
+		};
+
+		/* HDMI Tx I2C */
+		i2c1: i2c@fed41000{
+			status = "okay";
+
+			/* HDMI V1.3a supports Standard mode only */
+			clock-frequency = <100000>;
+		};
+
+		sbc_i2c0: i2c@fe540000{
+			status = "okay";
+		};
+
+		sbc_i2c1: i2c@fe541000{
+			status = "okay";
+		};
 	};
 };
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards
  2013-09-18 10:01 ` [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards Maxime COQUELIN
@ 2013-09-18 11:40   ` Lee Jones
  2013-09-18 12:36     ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Lee Jones @ 2013-09-18 11:40 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c, stephen.gallimore, stuart.menefy,
	gabriel.fernandez, olivier.clergeaud

On Wed, 18 Sep 2013, Maxime COQUELIN wrote:

> This patch supplies I2C configuration to B2000 and B2020
> based on either STiH415 or STiH416 SoCs.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>  arch/arm/boot/dts/stih41x-b2000.dtsi |    7 +++++++
>  arch/arm/boot/dts/stih41x-b2020.dtsi |   20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih41x-b2000.dtsi b/arch/arm/boot/dts/stih41x-b2000.dtsi
> index 8e694d2..346cc4bc 100644
> --- a/arch/arm/boot/dts/stih41x-b2000.dtsi
> +++ b/arch/arm/boot/dts/stih41x-b2000.dtsi
> @@ -37,5 +37,12 @@
>  			};
>  		};
>  
> +		/* HDMI Tx I2C */
> +		i2c1: i2c@fed41000{

nit: Space before the '{'

> +			status = "okay";

Consider enabling the node at the bottom.

> +
> +			/* HDMI V1.3a supports Standard mode only */
> +			clock-frequency = <100000>;
> +		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi b/arch/arm/boot/dts/stih41x-b2020.dtsi
> index 133e181..10b98da 100644
> --- a/arch/arm/boot/dts/stih41x-b2020.dtsi
> +++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
> @@ -38,5 +38,25 @@
>  				default-state = "off";
>  			};
>  		};
> +
> +		i2c0: i2c@fed40000{

As above.

> +			status = "okay";
> +		};
> +
> +		/* HDMI Tx I2C */
> +		i2c1: i2c@fed41000{

As above.

> +			status = "okay";

As above.

> +			/* HDMI V1.3a supports Standard mode only */
> +			clock-frequency = <100000>;
> +		};
> +
> +		sbc_i2c0: i2c@fe540000{

As above.

> +			status = "okay";
> +		};
> +
> +		sbc_i2c1: i2c@fe541000{

Are these nodes referenced by phandle at all?

If not, consider dropping the <lable>:s

> +			status = "okay";
> +		};
>  	};
>  };

Odd tabbing here.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC
  2013-09-18 10:01 ` [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC Maxime COQUELIN
@ 2013-09-18 12:00   ` Lee Jones
  2013-09-18 12:38     ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Lee Jones @ 2013-09-18 12:00 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Wolfram Sang, srinivas.kandagatla, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree, linux-doc, linux-kernel,
	linux-arm-kernel, linux-i2c, stephen.gallimore, stuart.menefy,
	gabriel.fernandez, olivier.clergeaud

On Wed, 18 Sep 2013, Maxime COQUELIN wrote:

> This patch supplies I2C configuration to STiH415 SoC.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>  arch/arm/boot/dts/stih415-pinctrl.dtsi |   36 ++++++++++++++++++++
>  arch/arm/boot/dts/stih415.dtsi         |   57 ++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih415-pinctrl.dtsi b/arch/arm/boot/dts/stih415-pinctrl.dtsi
> index 1d322b2..e56449d 100644
> --- a/arch/arm/boot/dts/stih415-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih415-pinctrl.dtsi
> @@ -86,6 +86,24 @@
>  					};
>  				};
>  			};
> +
> +			sbc_i2c0 {
> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
> +					st,pins {
> +						sda = <&PIO4 6 ALT1 BIDIR>;
> +						scl = <&PIO4 5 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			sbc_i2c1 {
> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
> +					st,pins {
> +						sda = <&PIO3 2 ALT2 BIDIR>;
> +						scl = <&PIO3 1 ALT2 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-front {
> @@ -143,6 +161,24 @@
>  				reg		= <0x7000 0x100>;
>  				st,bank-name	= "PIO12";
>  			};
> +
> +			i2c0 {
> +				pinctrl_i2c0_default: i2c0-default {
> +					st,pins {
> +						sda = <&PIO9 3 ALT1 BIDIR>;
> +						scl = <&PIO9 2 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			i2c1 {
> +				pinctrl_i2c1_default: i2c1-default {
> +					st,pins {
> +						sda = <&PIO12 1 ALT1 BIDIR>;
> +						scl = <&PIO12 0 ALT1 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-rear {
> diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
> index 74ab8de..643ae1c 100644
> --- a/arch/arm/boot/dts/stih415.dtsi
> +++ b/arch/arm/boot/dts/stih415.dtsi
> @@ -9,6 +9,7 @@
>  #include "stih41x.dtsi"
>  #include "stih415-clock.dtsi"
>  #include "stih415-pinctrl.dtsi"
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>  / {
>  
>  	L2: cache-controller {
> @@ -83,5 +84,61 @@
>  			pinctrl-names 	= "default";
>  			pinctrl-0	= <&pinctrl_sbc_serial1>;
>  		};
> +
> +		i2c0: i2c@fed40000{

Space before the '{'.

> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";

Consider putting the node status at the bottom.

> +			reg		= <0xfed40000 0x110>;
> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLKS_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		i2c1: i2c@fed41000{

Same here and throughout.

> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";

Same here and throughout.

> +			reg		= <0xfed41000 0x110>;
> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLKS_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c0: i2c@fe540000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe540000 0x110>;
> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c1: i2c@fe541000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe541000 0x110>;
> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
>  	};
>  };

Is this odd tabbing just the result of the patch format?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
       [not found]     ` <1379498483-4236-3-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-09-18 12:03       ` Lee Jones
  2013-09-18 12:46         ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Lee Jones @ 2013-09-18 12:03 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Wolfram Sang, srinivas.kandagatla-qxv4g6HH51o, Rob Herring,
	Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
	Rob Landley, Russell King, Grant Likely,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, stephen.gallimore-qxv4g6HH51o,
	stuart.menefy-qxv4g6HH51o, gabriel.fernandez-qxv4g6HH51o,
	olivier.clergeaud-qxv4g6HH51o

> This patch supplies I2C configuration to STiH416 SoC.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
> Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
> ---
>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi

I genuinely don't know the answer to this question, but are these
nodes identical to the ones you've just put in the stih415 DTSI file?
If so, I think it will be worth creating a stih41x DTSI rather than
duplicating lots of stuff unnecessarily.

> index 0f246c9..b29ff4b 100644
> --- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> @@ -97,6 +97,24 @@
>  					};
>  				};
>  			};
> +
> +			sbc_i2c0 {
> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
> +					st,pins {
> +						sda = <&PIO4 6 ALT1 BIDIR>;
> +						scl = <&PIO4 5 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			sbc_i2c1 {
> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
> +					st,pins {
> +						sda = <&PIO3 2 ALT2 BIDIR>;
> +						scl = <&PIO3 1 ALT2 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-front {
> @@ -175,6 +193,23 @@
>  				};
>  			};
>  
> +			i2c0 {
> +				pinctrl_i2c0_default: i2c0-default {
> +					st,pins {
> +						sda = <&PIO9 3 ALT1 BIDIR>;
> +						scl = <&PIO9 2 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			i2c1 {
> +				pinctrl_i2c1_default: i2c1-default {
> +					st,pins {
> +						sda = <&PIO12 1 ALT1 BIDIR>;
> +						scl = <&PIO12 0 ALT1 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-rear {
> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
> index 1a0326e..8856470 100644
> --- a/arch/arm/boot/dts/stih416.dtsi
> +++ b/arch/arm/boot/dts/stih416.dtsi
> @@ -9,6 +9,7 @@
>  #include "stih41x.dtsi"
>  #include "stih416-clock.dtsi"
>  #include "stih416-pinctrl.dtsi"
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>  / {
>  	L2: cache-controller {
>  		compatible = "arm,pl310-cache";
> @@ -92,5 +93,61 @@
>  			pinctrl-0 	= <&pinctrl_sbc_serial1>;
>  			clocks          = <&CLK_SYSIN>;
>  		};
> +
> +		i2c0: i2c@fed40000{

Same issues here. I assume most of this is copy paste.

> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfed40000 0x110>;
> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_S_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		i2c1: i2c@fed41000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfed41000 0x110>;
> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_S_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c0: i2c@fe540000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe540000 0x110>;
> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c1: i2c@fe541000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe541000 0x110>;
> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
>  	};
>  };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards
  2013-09-18 11:40   ` Lee Jones
@ 2013-09-18 12:36     ` Maxime COQUELIN
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 12:36 UTC (permalink / raw)
  To: Lee Jones
  Cc: Wolfram Sang, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

Hi Lee,

On 09/18/2013 01:40 PM, Lee Jones wrote:
> On Wed, 18 Sep 2013, Maxime COQUELIN wrote:
>
>> This patch supplies I2C configuration to B2000 and B2020
>> based on either STiH415 or STiH416 SoCs.
>>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/stih41x-b2000.dtsi |    7 +++++++
>>  arch/arm/boot/dts/stih41x-b2020.dtsi |   20 ++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stih41x-b2000.dtsi b/arch/arm/boot/dts/stih41x-b2000.dtsi
>> index 8e694d2..346cc4bc 100644
>> --- a/arch/arm/boot/dts/stih41x-b2000.dtsi
>> +++ b/arch/arm/boot/dts/stih41x-b2000.dtsi
>> @@ -37,5 +37,12 @@
>>  			};
>>  		};
>>  
>> +		/* HDMI Tx I2C */
>> +		i2c1: i2c@fed41000{
> nit: Space before the '{'
Ok. It will be fixed for all nodes in next series.
>
>> +			status = "okay";
> Consider enabling the node at the bottom.
>
>> +
>> +			/* HDMI V1.3a supports Standard mode only */
>> +			clock-frequency = <100000>;
>> +		};
>>  	};
>>  };
>> diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi b/arch/arm/boot/dts/stih41x-b2020.dtsi
>> index 133e181..10b98da 100644
>> --- a/arch/arm/boot/dts/stih41x-b2020.dtsi
>> +++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
>> @@ -38,5 +38,25 @@
>>  				default-state = "off";
>>  			};
>>  		};
>> +
>> +		i2c0: i2c@fed40000{
> As above.
>
>> +			status = "okay";
>> +		};
>> +
>> +		/* HDMI Tx I2C */
>> +		i2c1: i2c@fed41000{
> As above.
>
>> +			status = "okay";
> As above.
>
>> +			/* HDMI V1.3a supports Standard mode only */
>> +			clock-frequency = <100000>;
>> +		};
>> +
>> +		sbc_i2c0: i2c@fe540000{
> As above.
>
>> +			status = "okay";
>> +		};
>> +
>> +		sbc_i2c1: i2c@fe541000{
> Are these nodes referenced by phandle at all?
>
> If not, consider dropping the <lable>:s
It was references in an previous internal version of this series.
This is no more the case, so I will drop the labels.
>
>> +			status = "okay";
>> +		};
>>  	};
>>  };
> Odd tabbing here.
>
Thanks,
Maxime
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC
  2013-09-18 12:00   ` Lee Jones
@ 2013-09-18 12:38     ` Maxime COQUELIN
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 12:38 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mark Rutland, devicetree@vger.kernel.org, Ian Campbell,
	Russell King, Pawel Moll, Srinivas KANDAGATLA, Wolfram Sang,
	Stephen Warren, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, Rob Herring, Stephen GALLIMORE,
	Stuart MENEFY, linux-i2c@vger.kernel.org, Rob Landley,
	Grant Likely, Olivier CLERGEAUD, Gabriel FERNANDEZ,
	linux-arm-kernel@lists.infradead.org

On 09/18/2013 02:00 PM, Lee Jones wrote:
> On Wed, 18 Sep 2013, Maxime COQUELIN wrote:
>
>> This patch supplies I2C configuration to STiH415 SoC.
>>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>> ---
>>  arch/arm/boot/dts/stih415-pinctrl.dtsi |   36 ++++++++++++++++++++
>>  arch/arm/boot/dts/stih415.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stih415-pinctrl.dtsi b/arch/arm/boot/dts/stih415-pinctrl.dtsi
>> index 1d322b2..e56449d 100644
>> --- a/arch/arm/boot/dts/stih415-pinctrl.dtsi
>> +++ b/arch/arm/boot/dts/stih415-pinctrl.dtsi
>> @@ -86,6 +86,24 @@
>>  					};
>>  				};
>>  			};
>> +
>> +			sbc_i2c0 {
>> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO4 6 ALT1 BIDIR>;
>> +						scl = <&PIO4 5 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			sbc_i2c1 {
>> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO3 2 ALT2 BIDIR>;
>> +						scl = <&PIO3 1 ALT2 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-front {
>> @@ -143,6 +161,24 @@
>>  				reg		= <0x7000 0x100>;
>>  				st,bank-name	= "PIO12";
>>  			};
>> +
>> +			i2c0 {
>> +				pinctrl_i2c0_default: i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO9 3 ALT1 BIDIR>;
>> +						scl = <&PIO9 2 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			i2c1 {
>> +				pinctrl_i2c1_default: i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO12 1 ALT1 BIDIR>;
>> +						scl = <&PIO12 0 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-rear {
>> diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
>> index 74ab8de..643ae1c 100644
>> --- a/arch/arm/boot/dts/stih415.dtsi
>> +++ b/arch/arm/boot/dts/stih415.dtsi
>> @@ -9,6 +9,7 @@
>>  #include "stih41x.dtsi"
>>  #include "stih415-clock.dtsi"
>>  #include "stih415-pinctrl.dtsi"
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>  / {
>>  
>>  	L2: cache-controller {
>> @@ -83,5 +84,61 @@
>>  			pinctrl-names 	= "default";
>>  			pinctrl-0	= <&pinctrl_sbc_serial1>;
>>  		};
>> +
>> +		i2c0: i2c@fed40000{
> Space before the '{'.
It will be fixed in next series
>
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
> Consider putting the node status at the bottom.
Ok.
>
>> +			reg		= <0xfed40000 0x110>;
>> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLKS_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		i2c1: i2c@fed41000{
> Same here and throughout.
Ok.
>
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
> Same here and throughout.
Ok.
>
>> +			reg		= <0xfed41000 0x110>;
>> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLKS_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c0: i2c@fe540000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe540000 0x110>;
>> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c1: i2c@fe541000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe541000 0x110>;
>> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>>  	};
>>  };
> Is this odd tabbing just the result of the patch format?
This is just the result of the patch format.

Regards,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
  2013-09-18 12:03       ` Lee Jones
@ 2013-09-18 12:46         ` Maxime COQUELIN
       [not found]           ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD9578-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-18 12:46 UTC (permalink / raw)
  To: Lee Jones
  Cc: Wolfram Sang, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

On 09/18/2013 02:03 PM, Lee Jones wrote:
>> This patch supplies I2C configuration to STiH416 SoC.
>>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>> ---
>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> I genuinely don't know the answer to this question, but are these
> nodes identical to the ones you've just put in the stih415 DTSI file?
> If so, I think it will be worth creating a stih41x DTSI rather than
> duplicating lots of stuff unnecessarily.
There are close to be identical indeed.
For the clocks and pinctrl, the references names are the same, but they are
pointing on different nodes, as STiH415 and STiH416 have their own
clocks and pinctrl dtsi files.

Srini, what is opinion about this?
>
>> index 0f246c9..b29ff4b 100644
>> --- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> +++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> @@ -97,6 +97,24 @@
>>  					};
>>  				};
>>  			};
>> +
>> +			sbc_i2c0 {
>> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO4 6 ALT1 BIDIR>;
>> +						scl = <&PIO4 5 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			sbc_i2c1 {
>> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO3 2 ALT2 BIDIR>;
>> +						scl = <&PIO3 1 ALT2 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-front {
>> @@ -175,6 +193,23 @@
>>  				};
>>  			};
>>  
>> +			i2c0 {
>> +				pinctrl_i2c0_default: i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO9 3 ALT1 BIDIR>;
>> +						scl = <&PIO9 2 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			i2c1 {
>> +				pinctrl_i2c1_default: i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO12 1 ALT1 BIDIR>;
>> +						scl = <&PIO12 0 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-rear {
>> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
>> index 1a0326e..8856470 100644
>> --- a/arch/arm/boot/dts/stih416.dtsi
>> +++ b/arch/arm/boot/dts/stih416.dtsi
>> @@ -9,6 +9,7 @@
>>  #include "stih41x.dtsi"
>>  #include "stih416-clock.dtsi"
>>  #include "stih416-pinctrl.dtsi"
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>  / {
>>  	L2: cache-controller {
>>  		compatible = "arm,pl310-cache";
>> @@ -92,5 +93,61 @@
>>  			pinctrl-0 	= <&pinctrl_sbc_serial1>;
>>  			clocks          = <&CLK_SYSIN>;
>>  		};
>> +
>> +		i2c0: i2c@fed40000{
> Same issues here. I assume most of this is copy paste.
Yes indeed. This will be fixed in next version.
>
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfed40000 0x110>;
>> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_S_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		i2c1: i2c@fed41000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfed41000 0x110>;
>> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_S_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c0: i2c@fe540000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe540000 0x110>;
>> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c1: i2c@fe541000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe541000 0x110>;
>> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>>  	};
>>  };
Thanks,
Maxime


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-09-18 10:01 ` [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller Maxime COQUELIN
@ 2013-09-18 12:47   ` Gabriel FERNANDEZ
       [not found]     ` <5239A0ED.6010606-qxv4g6HH51o@public.gmane.org>
       [not found]   ` <1379498483-4236-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
  1 sibling, 1 reply; 28+ messages in thread
From: Gabriel FERNANDEZ @ 2013-09-18 12:47 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Mark Rutland, devicetree@vger.kernel.org, Ian Campbell,
	Russell King, Pawel Moll, Srinivas KANDAGATLA, Wolfram Sang,
	Stephen Warren, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, Rob Herring, Stephen GALLIMORE,
	Stuart MENEFY, linux-i2c@vger.kernel.org, Rob Landley,
	Grant Likely, Olivier CLERGEAUD, Lee Jones,
	linux-arm-kernel@lists.infradead.org

On 18/09/2013 12:01, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
>
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.
>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>   Documentation/devicetree/bindings/i2c/i2c-st.txt |   35 ++
>   drivers/i2c/busses/Kconfig                       |   10 +
>   drivers/i2c/busses/Makefile                      |    1 +
>   drivers/i2c/busses/i2c-st.c                      |  713 ++++++++++++++++++++++
>   drivers/i2c/busses/i2c-st.h                      |  216 +++++++
>   5 files changed, 975 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
>   create mode 100644 drivers/i2c/busses/i2c-st.c
>   create mode 100644 drivers/i2c/busses/i2c-st.h
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> new file mode 100644
> index 0000000..a7d6381
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> @@ -0,0 +1,35 @@
> +I2C for ST platforms
> +
> +Required properties :
> +- compatible : Must be "st,comms-i2c"
> +- reg : Offset and length of the register set for the device
> +- interrupts : the interrupt number
> +- clocks : phandle to the I2C clock source
> +
> +Recommended (non-standard) properties :
> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
> +  are supported, possible values are 100000 and 400000.
> +
> +Optional (non-standard) properties:
> +- st,glitches : Enable timing glitch suppression support.
> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.
> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.
> +- st,hw-glitches : Enable filter glitch suppression support.
> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.
> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.
> +
> +Examples :
> +
> +i2c0: i2c@fed40000{

Space before the '{'

> +       compatible      = "st,comms-i2c";
> +       reg             = <0xfed40000 0x110>;
> +       interrupts      =  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +       clocks          = <&CLK_S_ICN_REG_0>;
> +       clock-frequency = <400000>;
> +       pinctrl-names   = "default";
> +       pinctrl-0       = <&pinctrl_i2c0_default>;
> +       st,glitches;
> +       st,glitch-clk   = 500;
> +       st,glitch-dat   = 500;
> +};
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index cdcbd83..18ff129 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -776,6 +776,16 @@ config I2C_RCAR
>            This driver can also be built as a module.  If so, the module
>            will be called i2c-rcar.
>
> +config I2C_ST
> +       tristate "STMicroelectronics SSC I2C support"
> +       depends on ARCH_STI
> +       help
> +         Enable this option to add support for STMicroelectronics SoCs
> +         hardware SSC (Synchronous Serial Controller) as an I2C controller.
> +
> +         This driver can also be built as module. If so, the module
> +         will be called i2c-st.
> +
>   comment "External I2C/SMBus adapter drivers"
>
>   config I2C_DIOLAN_U2C
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index d00997f..9ea5d422 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -92,5 +92,6 @@ obj-$(CONFIG_I2C_PCA_ISA)     += i2c-pca-isa.o
>   obj-$(CONFIG_I2C_SIBYTE)       += i2c-sibyte.o
>   obj-$(CONFIG_SCx200_ACB)       += scx200_acb.o
>   obj-$(CONFIG_SCx200_I2C)       += scx200_i2c.o
> +obj-$(CONFIG_I2C_ST)           += i2c-st.o
>
>   ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
> new file mode 100644
> index 0000000..fa4451f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-st.c
> @@ -0,0 +1,713 @@
> +/*
> + * Copyright (C) 2013 STMicroelectronics
> + *
> + * I2C master mode controller driver, used in STMicroelectronics devices.
> + *
> + * Author: Maxime Coquelin <maxime.coquelin@st.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include "i2c-st.h"
> +
> +#define DRIVER_NAME "st-i2c"
> +
> +static struct st_i2c_timings i2c_timings[] = {
> +       [I2C_MODE_STANDARD] = {
> +               .rate                   = 100000,
> +               .rep_start_hold         = 4000,
> +               .rep_start_setup        = 4700,
> +               .start_hold             = 4000,
> +               .data_setup_time        = 250,
> +               .stop_setup_time        = 4000,
> +               .bus_free_time          = 4700,
> +               .glitch_dat_max         = 1200,
> +       },
> +       [I2C_MODE_FAST] = {
> +               .rate                   = 400000,
> +               .rep_start_hold         = 600,
> +               .rep_start_setup        = 600,
> +               .start_hold             = 1000,
> +               .data_setup_time        = 500,
> +               .stop_setup_time        = 900,
> +               .bus_free_time          = 1300,
> +               .glitch_dat_max         = 200,
> +       },
> +};
> +
> +static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +       int count, i;
> +
> +       /*
> +        * Counter only counts up to 7 but fifo size is 8...
> +        * When fifo is full, counter is 0 and RIR bit of status register is
> +        * set
> +        */
> +       if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
> +               count = SSC_RXFIFO_SIZE;
> +       else
> +               count = readl(i2c_dev->base + SSC_RX_FSTAT) &
> +                       SSC_RX_FSTAT_STATUS;
> +
> +       for (i = 0; i < count; i++)
> +               readl(i2c_dev->base + SSC_RBUF);
> +}
> +
> +static void st_i2c_soft_reset(struct st_i2c_dev *i2c_dev)
> +{
> +       /*
> +        * FIFO needs to be emptied before reseting the IP,
> +        * else the controller raises a BUSY error.
> +        */
> +       st_i2c_flush_rx_fifo(i2c_dev);
> +
> +       st_i2c_set_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
> +       st_i2c_clr_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
> +}
> +
> +/**
> + * st_i2c_hw_config() - Prepare SSC block, calculate and apply tuning timings
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_hw_config(struct st_i2c_dev *i2c_dev)
> +{
> +       unsigned long rate;
> +       u32 val, ns_per_clk;
> +       struct st_i2c_timings *t = &i2c_timings[i2c_dev->mode];
> +       struct st_i2c_glitches *g = &i2c_dev->glitches;
> +
> +       st_i2c_soft_reset(i2c_dev);
> +
> +       val = SSC_CLR_REPSTRT | SSC_CLR_NACK | SSC_CLR_SSCARBL |
> +               SSC_CLR_SSCAAS | SSC_CLR_SSCSTOP;
> +       writel(val, i2c_dev->base + SSC_CLR);
> +
> +       /* SSC Control register setup */
> +       val = SSC_CTL_PO | SSC_CTL_PH | SSC_CTL_HB | SSC_CTL_DATA_WIDTH_9;
> +       writel(val, i2c_dev->base + SSC_CTL);
> +
> +       rate = clk_get_rate(i2c_dev->clk);
> +       ns_per_clk = 1000000000 / rate;
> +
> +       /* Baudrate */
> +       val = rate / (2 * t->rate);
> +       writel(val, i2c_dev->base + SSC_BRG);
> +
> +       /* Pre-scaler baudrate */
> +       writel(1, i2c_dev->base + SSC_PRE_SCALER_BRG);
> +
> +       /* Enable I2C mode */
> +       writel(SSC_I2C_I2CM, i2c_dev->base + SSC_I2C);
> +
> +       /* Repeated start hold time */
> +       val = (t->rep_start_hold + g->glitch_dat) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_REP_START_HOLD);
> +
> +       /* Repeated start set up time */
> +       val = (t->rep_start_setup + g->glitch_clk) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_REP_START_SETUP);
> +
> +       /* Start hold time */
> +       if (g->glitch_dat < t->glitch_dat_max)
> +               val = (t->start_hold + g->glitch_dat) / ns_per_clk;
> +       else
> +               val = (t->start_hold + t->glitch_dat_max) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_START_HOLD);
> +
> +       /* Data set up time */
> +       val = (t->data_setup_time + g->glitch_dat) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_DATA_SETUP);
> +
> +       /* Stop set up time */
> +       val = (t->stop_setup_time + g->glitch_clk) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_STOP_SETUP);
> +
> +       /* Bus free time */
> +       val = (t->bus_free_time + g->glitch_dat) / ns_per_clk;
> +       writel(val, i2c_dev->base + SSC_BUS_FREE);
> +
> +       if (!g->hw_glitches) {
> +               writel(0, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
> +               return;
> +       }
> +
> +       /* Prescalers set up */
> +       val = rate / 10000000;
> +       writel(val, i2c_dev->base + SSC_PRSCALER);
> +       writel(val, i2c_dev->base + SSC_PRSCALER_DATAOUT);
> +
> +       /* Noise suppression witdh */
> +       val = g->hw_glitch_clk * rate / 100000000;
> +       writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
> +
> +       /* Noise suppression max output data delay width */
> +       val = g->hw_glitch_dat * rate / 100000000;
> +       writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH_DATAOUT);
> +
> +       return;
> +}
> +
> +static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
> +{
> +       u32 sta;
> +       int i;
> +
> +       for (i = 0; i < 10; i++) {
> +               sta = readl(i2c_dev->base + SSC_STA);
> +               if (!(sta & SSC_STA_BUSY))
> +                       return 0;
> +
> +               usleep_range(2000, 4000);
> +       }
> +
> +       dev_err(i2c_dev->dev, "bus not free (status = 0x%08x)\n", sta);
> +
> +       return -EBUSY;
> +}
> +
> +/**
> + * st_i2c_write_tx_fifo() - Write a byte in the Tx FIFO
> + * @i2c_dev: Controller's private data
> + * @byte: Data to write in the Tx FIFO
> + */
> +static inline void st_i2c_write_tx_fifo(struct st_i2c_dev *i2c_dev, u8 byte)
> +{
> +       u16 tbuf = byte << 1;
> +
> +       writel(tbuf | 1, i2c_dev->base + SSC_TBUF);
> +}
> +
> +/**
> + * st_i2c_wr_fill_tx_fifo() - Fill the Tx FIFO in write mode
> + * @i2c_dev: Controller's private data
> + *
> + * This functions fills the Tx FIFO with I2C transfert buffer when
> + * in write mode.
> + */
> +static void st_i2c_wr_fill_tx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 tx_fstat, sta;
> +       int i;
> +
> +       sta = readl(i2c_dev->base + SSC_STA);
> +       if (sta & SSC_STA_TX_FIFO_FULL)
> +               return;
> +
> +       tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
> +
> +       if (c->count < (SSC_TXFIFO_SIZE - tx_fstat))
> +               i = c->count;
> +       else
> +               i = SSC_TXFIFO_SIZE - tx_fstat;
> +
> +       for (; i > 0; i--, c->count--, c->buf++)
> +               st_i2c_write_tx_fifo(i2c_dev, *c->buf);
> +}
> +
> +/**
> + * st_i2c_rd_fill_tx_fifo() - Fill the Tx FIFO in read mode
> + * @i2c_dev: Controller's private data
> + *
> + * This functions fills the Tx FIFO with fixed pattern when
> + * in read mode to trigger clock.
> + */
> +static void st_i2c_rd_fill_tx_fifo(struct st_i2c_dev *i2c_dev, int max)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 tx_fstat, sta;
> +       int i;
> +
> +       sta = readl(i2c_dev->base + SSC_STA);
> +       if (sta & SSC_STA_TX_FIFO_FULL)
> +               return;
> +
> +       tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
> +
> +       if (max < (SSC_TXFIFO_SIZE - tx_fstat))
> +               i = max;
> +       else
> +               i = SSC_TXFIFO_SIZE - tx_fstat;
> +
> +       for (; i > 0; i--, c->xfered++)
> +               st_i2c_write_tx_fifo(i2c_dev, 0xff);
> +}
> +
> +static void st_i2c_read_rx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 i, sta;
> +       u16 rbuf;
> +
> +       sta = readl(i2c_dev->base + SSC_STA);
> +       if (sta & SSC_STA_RIR) {
> +               i = SSC_RXFIFO_SIZE;
> +       } else {
> +               i = readl(i2c_dev->base + SSC_RX_FSTAT);
> +               i &= SSC_RX_FSTAT_STATUS;
> +       }
> +
> +       for (; (i > 0) && (c->count > 0); i--, c->count--) {
> +               rbuf = readl(i2c_dev->base + SSC_RBUF) >> 1;
> +               *c->buf++ = (u8)rbuf & 0xff;
> +       }
> +
> +       if (i) {
> +               dev_err(i2c_dev->dev, "Unexpected %d bytes in rx fifo\n", i);
> +               st_i2c_flush_rx_fifo(i2c_dev);
> +       }
> +}
> +
> +/**
> + * st_i2c_terminate_xfer() - Send either STOP or REPSTART condition
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_terminate_xfer(struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +
> +       st_i2c_clr_bits(i2c_dev->base + SSC_IEN, SSC_IEN_TEEN);
> +       st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
> +
> +       if (c->stop) {
> +               st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_STOPEN);
> +               st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +       } else {
> +               st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_REPSTRTEN);
> +               st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_REPSTRTG);
> +       }
> +}
> +
> +/**
> + * st_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_handle_write(struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +
> +       st_i2c_flush_rx_fifo(i2c_dev);
> +
> +       if (!c->count)
> +               /* End of xfer, send stop or repstart */
> +               st_i2c_terminate_xfer(i2c_dev);
> +       else
> +               st_i2c_wr_fill_tx_fifo(i2c_dev);
> +}
> +
> +/**
> + * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 ien;
> +
> +       /* Trash the address read back */
> +       if (!c->xfered) {
> +               readl(i2c_dev->base + SSC_RBUF);
> +               st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
> +       } else
> +               st_i2c_read_rx_fifo(i2c_dev);
> +
> +       if (!c->count) {
> +               /* End of xfer, send stop or repstart */
> +               st_i2c_terminate_xfer(i2c_dev);
> +       } else if (c->count == 1) {
> +               /* Penultimate byte to xfer, disable ACK gen. */
> +               st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
> +
> +               /* Last received byte is to be handled by NACK interrupt */
> +               ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
> +               writel(ien, i2c_dev->base + SSC_IEN);
> +
> +               st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
> +       } else
> +               st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
> +}
> +
> +/**
> + * st_i2c_isr() - Interrupt routine
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t st_i2c_isr(int irq, void *data)
> +{
> +       struct st_i2c_dev *i2c_dev = data;
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 sta, ien, it;
> +
> +       ien = readl(i2c_dev->base + SSC_IEN);
> +       sta = readl(i2c_dev->base + SSC_STA);
> +
> +       /* Use __fls() to check error bits first */
> +       it = __fls(sta & ien);
> +
> +       switch (1 << it) {
> +       case SSC_STA_TE:
> +               if (c->addr & I2C_M_RD)
> +                       st_i2c_handle_read(i2c_dev);
> +               else
> +                       st_i2c_handle_write(i2c_dev);
> +               break;
> +
> +       case SSC_STA_STOP:
> +       case SSC_STA_REPSTRT:
> +               writel(0, i2c_dev->base + SSC_IEN);
> +               complete(&i2c_dev->complete);
> +               break;
> +
> +       case SSC_STA_NACK:
> +               writel(SSC_CLR_NACK, i2c_dev->base + SSC_CLR);
> +
> +               /* Last received byte handled by NACK interrupt */
> +               if ((c->addr & I2C_M_RD) && (c->count == 1) && (c->xfered)) {
> +                       st_i2c_handle_read(i2c_dev);
> +                       break;
> +               }
> +
> +               it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
> +               writel(it, i2c_dev->base + SSC_IEN);
> +
> +               st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +               c->result = -EIO;
> +               break;
> +
> +       case SSC_STA_ARBL:
> +               writel(SSC_CLR_SSCARBL, i2c_dev->base + SSC_CLR);
> +
> +               it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
> +               writel(it, i2c_dev->base + SSC_IEN);
> +
> +               st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +               c->result = -EIO;
> +               break;
> +
> +       default:
> +               dev_err(i2c_dev->dev, "%s: it %d unhandled (status=0x%08x)\n",
> +                               __func__, it, readl(i2c_dev->base + SSC_STA));
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +/**
> + * st_i2c_xfer_msg() - Transfer a single I2C message
> + * @i2c_dev: Controller's private data
> + * @msg: I2C message to transfer
> + * @is_first: first message of the sequence
> + * @is_last: last message of the sequence
> + */
> +static int st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
> +                           bool is_first, bool is_last)
> +{
> +       struct st_i2c_client *c = &i2c_dev->client;
> +       u32 ctl, i2c, it;
> +       unsigned long timeout;
> +       int ret;
> +
> +       c->addr         = (u8)(msg->addr << 1);
> +       c->addr         |= (msg->flags & I2C_M_RD);
> +       c->buf          = msg->buf;
> +       c->count        = msg->len;
> +       c->xfered       = 0;
> +       c->result       = 0;
> +       c->stop         = is_last;
> +
> +       init_completion(&i2c_dev->complete);
> +
> +       ctl = SSC_CTL_EN | SSC_CTL_MS | SSC_CTL_EN_RX_FIFO | SSC_CTL_EN_TX_FIFO;
> +       st_i2c_set_bits(i2c_dev->base + SSC_CTL, ctl);
> +
> +       i2c = SSC_I2C_TXENB;
> +       if (c->addr & I2C_M_RD)
> +               i2c |= SSC_I2C_ACKG;
> +       st_i2c_set_bits(i2c_dev->base + SSC_I2C, i2c);
> +
> +       /* Write slave address */
> +       st_i2c_write_tx_fifo(i2c_dev, c->addr);
> +
> +       /* Pre-fill Tx fifo with data in case of write */
> +       if (!(c->addr & I2C_M_RD))
> +               st_i2c_wr_fill_tx_fifo(i2c_dev);
> +
> +       it = SSC_IEN_NACKEN | SSC_IEN_TEEN | SSC_IEN_ARBLEN;
> +       writel(it, i2c_dev->base + SSC_IEN);
> +
> +       if (is_first) {
> +               ret = st_i2c_wait_free_bus(i2c_dev);
> +               if (ret)
> +                       return ret;
> +
> +               st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
> +       }
> +
> +       timeout = wait_for_completion_timeout(&i2c_dev->complete,
> +                       i2c_dev->adap.timeout);
> +       ret = c->result;
> +
> +       if (!timeout) {
> +               dev_err(i2c_dev->dev, "Write to slave 0x%x timed out\n",
> +                               c->addr);
> +               ret = -ETIMEDOUT;
> +       }
> +
> +       i2c = SSC_I2C_STOPG | SSC_I2C_REPSTRTG;
> +       st_i2c_clr_bits(i2c_dev->base + SSC_I2C, i2c);
> +
> +       writel(SSC_CLR_SSCSTOP | SSC_CLR_REPSTRT, i2c_dev->base + SSC_CLR);
> +
> +       return ret;
> +}
> +
> +/**
> + * st_i2c_xfer() - Transfer a single I2C message
> + * @i2c_adap: Adapter pointer to the controller
> + * @msgs: Pointer to data to be written.
> + * @num: Number of messages to be executed
> + */
> +static int st_i2c_xfer(struct i2c_adapter *i2c_adap,
> +                       struct i2c_msg msgs[], int num)
> +{
> +       struct st_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +       bool last, first;
> +       int ret, i;
> +
> +       i2c_dev->busy = true;
> +
> +       ret = clk_prepare_enable(i2c_dev->clk);
> +       if (ret) {
> +               dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
> +               return ret;
> +       }
> +
> +       pinctrl_pm_select_default_state(i2c_dev->dev);
> +
> +       st_i2c_hw_config(i2c_dev);
> +
> +       first = true;
> +
> +       for (i = 0; i < num; i++) {
> +               last = (i < num - 1) ? false : true;
> +
> +               ret = st_i2c_xfer_msg(i2c_dev, &msgs[i], first, last);
> +               if (ret)
> +                       break;
> +
> +               first = false;
> +       }
> +
> +       pinctrl_pm_select_idle_state(i2c_dev->dev);
> +
> +       clk_disable_unprepare(i2c_dev->clk);
> +
> +       i2c_dev->busy = false;
> +
> +       return (ret < 0) ? ret : i;
> +}
> +
> +#ifdef CONFIG_PM
> +static int st_i2c_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev =
> +               container_of(dev, struct platform_device, dev);
> +       struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +       if (i2c_dev->busy)
> +               return -EBUSY;
> +
> +       pinctrl_pm_select_sleep_state(dev);
> +
> +       return 0;
> +}
> +
> +static int st_i2c_resume(struct device *dev)
> +{
> +       pinctrl_pm_select_default_state(dev);
> +       /* Go in idle state if available */
> +       pinctrl_pm_select_idle_state(dev);
> +
> +       return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(st_i2c_pm, st_i2c_suspend, st_i2c_resume);
> +#define ST_I2C_PM      (&st_i2c_pm)
> +#else
> +#define ST_I2C_PM      NULL
> +#endif
> +
> +static u32 st_i2c_func(struct i2c_adapter *adap)
> +{
> +       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm st_i2c_algo = {
> +       .master_xfer = st_i2c_xfer,
> +       .functionality = st_i2c_func,
> +};
> +
> +static int st_i2c_of_get_glitches(struct device_node *np,
> +               struct st_i2c_dev *i2c_dev)
> +{
> +       struct st_i2c_glitches *g = &i2c_dev->glitches;
> +       int ret;
> +
> +       g->glitches = of_property_read_bool(np, "st,glitches");
> +       if (g->glitches) {
> +               ret = of_property_read_u32(np, "st,glitch-clk",
> +                               &g->glitch_clk);
> +               if (ret) {
> +                       dev_err(i2c_dev->dev, "st,glitch-clk not found\n");
> +                       return ret;
> +               }
> +
> +               ret = of_property_read_u32(np, "st,glitch-dat",
> +                               &g->glitch_dat);
> +               if (ret) {
> +                       dev_err(i2c_dev->dev, "st,glitch-dat not found\n");
> +                       return ret;
> +               }
> +       }
> +
> +       g->hw_glitches = of_property_read_bool(np, "st,hw-glitches");
> +       if (g->hw_glitches) {
> +               ret = of_property_read_u32(np, "st,hw-glitch-clk",
> +                               &g->hw_glitch_clk);
> +               if (ret) {
> +                       dev_err(i2c_dev->dev, "st,hw-glitch-clk not found\n");
> +                       return ret;
> +               }
> +
> +               ret = of_property_read_u32(np, "st,hw-glitch-dat",
> +                               &g->hw_glitch_dat);
> +               if (ret) {
> +                       dev_err(i2c_dev->dev, "st,hw-glitch-dat not found\n");
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int st_i2c_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct st_i2c_dev *i2c_dev;
> +       struct resource *res;
> +       u32 clk_rate;
> +       struct i2c_adapter *adap;
> +       int ret;
> +
> +       i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
> +       if (!i2c_dev)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(i2c_dev->base))
> +               return PTR_ERR(i2c_dev->base);
> +
> +       i2c_dev->irq = irq_of_parse_and_map(np, 0);
> +       if (!i2c_dev->irq) {
> +               dev_err(&pdev->dev, "IRQ missing or invalid\n");
> +               return -EINVAL;
> +       }
> +
> +       i2c_dev->clk = of_clk_get(np, 0);
> +       if (IS_ERR(i2c_dev->clk)) {
> +               dev_err(&pdev->dev, "Unable to request clock\n");
> +               return PTR_ERR(i2c_dev->clk);
> +       }
> +
> +       i2c_dev->mode = I2C_MODE_STANDARD;
> +       ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> +       if ((!ret) && (clk_rate == 400000))
> +               i2c_dev->mode = I2C_MODE_FAST;
> +
> +       i2c_dev->dev = &pdev->dev;
> +
> +       ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
> +                       pdev->name, i2c_dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> +               return ret;
> +       }
> +
> +       pinctrl_pm_select_default_state(i2c_dev->dev);
> +       /* In case idle state available, select it */
> +       pinctrl_pm_select_idle_state(i2c_dev->dev);
> +
> +       ret = st_i2c_of_get_glitches(np, i2c_dev);
> +       if (ret)
> +               return ret;
> +
> +       adap = &i2c_dev->adap;
> +       i2c_set_adapdata(adap, i2c_dev);
> +       snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
> +       adap->owner = THIS_MODULE;
> +       adap->timeout = 2 * HZ;
> +       adap->retries = 0;
> +       adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
> +       adap->algo = &st_i2c_algo;
> +       adap->dev.parent = &pdev->dev;
> +       adap->dev.of_node = pdev->dev.of_node;
> +
> +       ret = i2c_add_adapter(adap);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to add adapter\n");
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, i2c_dev);
> +
> +       dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
> +
> +       return 0;
> +}
> +
> +static int st_i2c_remove(struct platform_device *pdev)
> +{
> +       struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +       i2c_del_adapter(&i2c_dev->adap);
> +
> +       return 0;
> +}
> +
> +static struct of_device_id st_i2c_match[] = {
> +       { .compatible = "st,comms-i2c", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, st_i2c_match);
> +
> +static struct platform_driver st_i2c_driver = {
> +       .driver = {
> +               .name = DRIVER_NAME,
> +               .owner = THIS_MODULE,
> +               .of_match_table = st_i2c_match,
> +               .pm = &st_i2c_pm,
> +       },
> +       .probe = st_i2c_probe,
> +       .remove = st_i2c_remove,
> +};
> +
> +module_platform_driver(st_i2c_driver);
> +
> +MODULE_AUTHOR("STMicroelectronics");
> +MODULE_DESCRIPTION("STMicroelectronics I2C driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/i2c/busses/i2c-st.h b/drivers/i2c/busses/i2c-st.h
> new file mode 100644
> index 0000000..7bce6c0
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-st.h
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (C) 2013 STMicroelectronics
> + *
> + * I2C master mode controller driver, used in STMicroelectronics devices.
> + *
> + * Author: Maxime Coquelin <maxime.coquelin@st.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __I2C_ST_H_
> +#define __I2C_ST_H_
> +
> +/* SSC registers */
> +#define SSC_BRG                                0x000
> +#define SSC_TBUF                       0x004
> +#define SSC_RBUF                       0x008
> +#define SSC_CTL                                0x00C
> +#define SSC_IEN                                0x010
> +#define SSC_STA                                0x014
> +#define SSC_I2C                                0x018
> +#define SSC_SLAD                       0x01C
> +#define SSC_REP_START_HOLD             0x020
> +#define SSC_START_HOLD                 0x024
> +#define SSC_REP_START_SETUP            0x028
> +#define SSC_DATA_SETUP                 0x02C
> +#define SSC_STOP_SETUP                 0x030
> +#define SSC_BUS_FREE                   0x034
> +#define SSC_TX_FSTAT                   0x038
> +#define SSC_RX_FSTAT                   0x03C
> +#define SSC_PRE_SCALER_BRG             0x040
> +#define SSC_CLR                                0x080
> +#define SSC_NOISE_SUPP_WIDTH           0x100
> +#define SSC_PRSCALER                   0x104
> +#define SSC_NOISE_SUPP_WIDTH_DATAOUT   0x108
> +#define SSC_PRSCALER_DATAOUT           0x10c
> +
> +/* SSC Control */
> +#define SSC_CTL_DATA_WIDTH_9           0x8
> +#define SSC_CTL_DATA_WIDTH_MSK         0xf
> +#define SSC_CTL_BM                     0xf
> +#define SSC_CTL_HB                     BIT(4)
> +#define SSC_CTL_PH                     BIT(5)
> +#define SSC_CTL_PO                     BIT(6)
> +#define SSC_CTL_SR                     BIT(7)
> +#define SSC_CTL_MS                     BIT(8)
> +#define SSC_CTL_EN                     BIT(9)
> +#define SSC_CTL_LPB                    BIT(10)
> +#define SSC_CTL_EN_TX_FIFO             BIT(11)
> +#define SSC_CTL_EN_RX_FIFO             BIT(12)
> +#define SSC_CTL_EN_CLST_RX             BIT(13)
> +
> +/* SSC Interrupt Enable */
> +#define SSC_IEN_RIEN                   BIT(0)
> +#define SSC_IEN_TIEN                   BIT(1)
> +#define SSC_IEN_TEEN                   BIT(2)
> +#define SSC_IEN_REEN                   BIT(3)
> +#define SSC_IEN_PEEN                   BIT(4)
> +#define SSC_IEN_AASEN                  BIT(6)
> +#define SSC_IEN_STOPEN                 BIT(7)
> +#define SSC_IEN_ARBLEN                 BIT(8)
> +#define SSC_IEN_NACKEN                 BIT(10)
> +#define SSC_IEN_REPSTRTEN              BIT(11)
> +#define SSC_IEN_TX_FIFO_HALF           BIT(12)
> +#define SSC_IEN_RX_FIFO_HALF_FULL      BIT(14)
> +
> +/* SSC Status */
> +#define SSC_STA_RIR                    BIT(0)
> +#define SSC_STA_TIR                    BIT(1)
> +#define SSC_STA_TE                     BIT(2)
> +#define SSC_STA_RE                     BIT(3)
> +#define SSC_STA_PE                     BIT(4)
> +#define SSC_STA_CLST                   BIT(5)
> +#define SSC_STA_AAS                    BIT(6)
> +#define SSC_STA_STOP                   BIT(7)
> +#define SSC_STA_ARBL                   BIT(8)
> +#define SSC_STA_BUSY                   BIT(9)
> +#define SSC_STA_NACK                   BIT(10)
> +#define SSC_STA_REPSTRT                        BIT(11)
> +#define SSC_STA_TX_FIFO_HALF           BIT(12)
> +#define SSC_STA_TX_FIFO_FULL           BIT(13)
> +#define SSC_STA_RX_FIFO_HALF           BIT(14)
> +
> +/* SSC I2C Control */
> +#define SSC_I2C_I2CM                   BIT(0)
> +#define SSC_I2C_STRTG                  BIT(1)
> +#define SSC_I2C_STOPG                  BIT(2)
> +#define SSC_I2C_ACKG                   BIT(3)
> +#define SSC_I2C_AD10                   BIT(4)
> +#define SSC_I2C_TXENB                  BIT(5)
> +#define SSC_I2C_REPSTRTG               BIT(11)
> +#define SSC_I2C_SLAVE_DISABLE          BIT(12)
> +
> +/* SSC Tx FIFO Status */
> +#define SSC_TX_FSTAT_STATUS            0x07
> +
> +/* SSC Rx FIFO Status */
> +#define SSC_RX_FSTAT_STATUS            0x07
> +
> +/* SSC Clear bit operation */
> +#define SSC_CLR_SSCAAS                 BIT(6)
> +#define SSC_CLR_SSCSTOP                        BIT(7)
> +#define SSC_CLR_SSCARBL                        BIT(8)
> +#define SSC_CLR_NACK                   BIT(10)
> +#define SSC_CLR_REPSTRT                        BIT(11)
> +
> +/* SSC Clock Prescaler */
> +#define SSC_PRSC_VALUE                 0x0f
> +
> +
> +#define SSC_TXFIFO_SIZE                        0x8
> +#define SSC_RXFIFO_SIZE                        0x8
> +
> +enum st_i2c_mode {
> +       I2C_MODE_STANDARD,
> +       I2C_MODE_FAST,
> +       I2C_MODE_END,
> +};
> +
> +/**
> + * struct st_i2c_timings - per-Mode tuning parameters
> + * @rate: I2C bus rate
> + * @rep_start_hold: I2C repeated start hold time requirement
> + * @rep_start_setup: I2C repeated start set up time requirement
> + * @start_hold: I2C start hold time requirement
> + * @data_setup_time: I2C data set up time requirement
> + * @stop_setup_time: I2C stop set up time requirement
> + * @bus_free_time: I2C bus free time requirement
> + * @glitch_dat_max: I2C data glitch max time
> + */
> +struct st_i2c_timings {
> +       u32 rate;
> +       u32 rep_start_hold;
> +       u32 rep_start_setup;
> +       u32 start_hold;
> +       u32 data_setup_time;
> +       u32 stop_setup_time;
> +       u32 bus_free_time;
> +       u32 glitch_dat_max;
> +};
> +
> +/**
> + * struct st_i2c_glitches - per-SoC tuning parameters
> + * @glitches: Glitches are defined for this SoC
> + * @glitch_clk: Clk line glitch tuning value
> + * @glitch_dat: Data line glitch tuning value
> + * @hw_glitches: HW glitches are defined for this SoC
> + * @hw_glitch_clk: Clk line hw glitch value
> + * @hw_glitch_dat: Data line hw glitch value
> + */
> +struct st_i2c_glitches {
> +       bool    glitches;
> +       u32     glitch_clk;
> +       u32     glitch_dat;
> +       bool    hw_glitches;
> +       u32     hw_glitch_clk;
> +       u32     hw_glitch_dat;
> +};
> +
> +/**
> + * struct st_i2c_client - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transfered
> + * @xfered: number of bytes already transferred
> + * @buf: data buffer
> + * @result: result of the transfer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + */
> +struct st_i2c_client {
> +       u8      addr;
> +       u32     count;
> +       u32     xfered;
> +       u8      *buf;
> +       int     result;
> +       bool    stop;
> +};
> +
> +/**
> + * struct st_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @irq: interrupt line for th controller
> + * @clk: hw ssc block clock
> + * @mode: I2C mode of the controller. Standard or Fast only supported
> + * @glitches: Glitch filters tuning parameters
> + * @client: I2C transfert information
> + * @busy: I2C transfer on-going
> + */
> +struct st_i2c_dev {
> +       struct i2c_adapter      adap;
> +       struct device           *dev;
> +       void __iomem            *base;
> +       struct completion       complete;
> +       int                     irq;
> +       struct clk              *clk;
> +       int                     mode;
> +       struct st_i2c_glitches glitches;
> +       struct st_i2c_client    client;
> +       bool                    busy;
> +};
> +
> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
> +{
> +       writel(readl(reg) | mask, reg);
> +}
> +
> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
> +{
> +       writel(readl(reg) & ~mask, reg);
> +}
> +
> +#endif /* __I2C_ST_H_ */
> --
> 1.7.9.5
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
       [not found]           ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD9578-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
@ 2013-09-18 12:57             ` Srinivas KANDAGATLA
  2013-09-19  7:16               ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Srinivas KANDAGATLA @ 2013-09-18 12:57 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Lee Jones, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

On 18/09/13 13:46, Maxime COQUELIN wrote:
> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>> >> This patch supplies I2C configuration to STiH416 SoC.
>>> >>
>>> >> Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>>> >> Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
>>> >> ---
>>> >>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>> >>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>> >>  2 files changed, 92 insertions(+)
>>> >>
>>> >> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> > I genuinely don't know the answer to this question, but are these
>> > nodes identical to the ones you've just put in the stih415 DTSI file?
>> > If so, I think it will be worth creating a stih41x DTSI rather than
>> > duplicating lots of stuff unnecessarily.
> There are close to be identical indeed.
> For the clocks and pinctrl, the references names are the same, but they are
> pointing on different nodes, as STiH415 and STiH416 have their own
> clocks and pinctrl dtsi files.
> 
> Srini, what is opinion about this?

There is already a stih41x.dtsi file, but I don't think it is the right
place for the pinctrl nodes there.

Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
for two reasons.

1> If we common up the pinctrl nodes, it will be very difficult to
accommodate new pinctrls layout which is not guaranteed to be in same
layout in future SOCs.

2> The retiming values in the pinctrl nodes tend to change as per SOC,
so it will be difficult to manage it if we common it up.

Am sure we can come up with a dt layout which can reduce duplication,
but we have to be careful here not to lose the flexiblity to accommodate
new picntrl layouts, new retimings values based on SOC.


thanks,
srini

>> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
  2013-09-18 12:57             ` Srinivas KANDAGATLA
@ 2013-09-19  7:16               ` Maxime COQUELIN
  2013-09-19 12:59                 ` Srinivas KANDAGATLA
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-19  7:16 UTC (permalink / raw)
  To: Srinivas KANDAGATLA
  Cc: Lee Jones, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

Hi Srini,

On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
> On 18/09/13 13:46, Maxime COQUELIN wrote:
>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>
>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 92 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>> I genuinely don't know the answer to this question, but are these
>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>> duplicating lots of stuff unnecessarily.
>> There are close to be identical indeed.
>> For the clocks and pinctrl, the references names are the same, but they are
>> pointing on different nodes, as STiH415 and STiH416 have their own
>> clocks and pinctrl dtsi files.
>>
>> Srini, what is opinion about this?
> There is already a stih41x.dtsi file, but I don't think it is the right
> place for the pinctrl nodes there.
>
> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
> for two reasons.
>
> 1> If we common up the pinctrl nodes, it will be very difficult to
> accommodate new pinctrls layout which is not guaranteed to be in same
> layout in future SOCs.
>
> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
> so it will be difficult to manage it if we common it up.
>
> Am sure we can come up with a dt layout which can reduce duplication,
> but we have to be careful here not to lose the flexiblity to accommodate
> new picntrl layouts, new retimings values based on SOC.
Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
file,
and overload them with the pinctrl and clock properties in the stih416
and stih415 dtsi files?

Regards,
Maxime
>
>
> thanks,
> srini
>
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
  2013-09-19  7:16               ` Maxime COQUELIN
@ 2013-09-19 12:59                 ` Srinivas KANDAGATLA
  2013-09-19 15:22                   ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Srinivas KANDAGATLA @ 2013-09-19 12:59 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Lee Jones, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

On 19/09/13 08:16, Maxime COQUELIN wrote:
> Hi Srini,
> 
> On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
>> On 18/09/13 13:46, Maxime COQUELIN wrote:
>>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>>
>>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 92 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>>> I genuinely don't know the answer to this question, but are these
>>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>>> duplicating lots of stuff unnecessarily.
>>> There are close to be identical indeed.
>>> For the clocks and pinctrl, the references names are the same, but they are
>>> pointing on different nodes, as STiH415 and STiH416 have their own
>>> clocks and pinctrl dtsi files.
>>>
>>> Srini, what is opinion about this?
>> There is already a stih41x.dtsi file, but I don't think it is the right
>> place for the pinctrl nodes there.
>>
>> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
>> for two reasons.
>>
>> 1> If we common up the pinctrl nodes, it will be very difficult to
>> accommodate new pinctrls layout which is not guaranteed to be in same
>> layout in future SOCs.
>>
>> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
>> so it will be difficult to manage it if we common it up.
>>
>> Am sure we can come up with a dt layout which can reduce duplication,
>> but we have to be careful here not to lose the flexiblity to accommodate
>> new picntrl layouts, new retimings values based on SOC.
> Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
> file,
> and overload them with the pinctrl and clock properties in the stih416
> and stih415 dtsi files?
Am not very comfortable with this idea.

As there is no guarantee that the interrupt number/memory map and the
i2c numbering will be same in future SOCs or other IPs.

You might be already aware that the number of i2cs on each SOC are
different as example on STiH415 we have 10 SSCs and on STiH416 we have
11 SSCs. So, At what point you decide that which devices/IPs should be
in stih41x and which should in stih415/Stih416?

Each i2c node will save around 5 lines if we common it up, but if the
interrupt number or map changes this difference will be negligible.

Common up at this level and mixing un-common ones in stih415.dtsi or
stih416.dtsi will add confusion to readers as the information is split
at multiple places.

IMO the common up idea sounds good but reduces the readability and has
no effect on final dtb size.

Thanks,
srini


> 
> Regards,
> Maxime
>>
>>
>> thanks,
>> srini
>>
>>
> 
> 
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
  2013-09-19 12:59                 ` Srinivas KANDAGATLA
@ 2013-09-19 15:22                   ` Maxime COQUELIN
       [not found]                     ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD957D-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-19 15:22 UTC (permalink / raw)
  To: Srinivas KANDAGATLA
  Cc: Lee Jones, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

On 09/19/2013 03:01 PM, Srinivas KANDAGATLA wrote:
> On 19/09/13 08:16, Maxime COQUELIN wrote:
>> Hi Srini,
>>
>> On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
>>> On 18/09/13 13:46, Maxime COQUELIN wrote:
>>>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>>>
>>>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>>>  2 files changed, 92 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>>>> I genuinely don't know the answer to this question, but are these
>>>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>>>> duplicating lots of stuff unnecessarily.
>>>> There are close to be identical indeed.
>>>> For the clocks and pinctrl, the references names are the same, but they are
>>>> pointing on different nodes, as STiH415 and STiH416 have their own
>>>> clocks and pinctrl dtsi files.
>>>>
>>>> Srini, what is opinion about this?
>>> There is already a stih41x.dtsi file, but I don't think it is the right
>>> place for the pinctrl nodes there.
>>>
>>> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
>>> for two reasons.
>>>
>>> 1> If we common up the pinctrl nodes, it will be very difficult to
>>> accommodate new pinctrls layout which is not guaranteed to be in same
>>> layout in future SOCs.
>>>
>>> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
>>> so it will be difficult to manage it if we common it up.
>>>
>>> Am sure we can come up with a dt layout which can reduce duplication,
>>> but we have to be careful here not to lose the flexiblity to accommodate
>>> new picntrl layouts, new retimings values based on SOC.
>> Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
>> file,
>> and overload them with the pinctrl and clock properties in the stih416
>> and stih415 dtsi files?
> Am not very comfortable with this idea.
>
> As there is no guarantee that the interrupt number/memory map and the
> i2c numbering will be same in future SOCs or other IPs.
>
> You might be already aware that the number of i2cs on each SOC are
> different as example on STiH415 we have 10 SSCs and on STiH416 we have
> 11 SSCs. So, At what point you decide that which devices/IPs should be
> in stih41x and which should in stih415/Stih416?
Yes, I know there is one more SSC on STiH416.

On one hand, this could add some confusion. But on the other hand,
someone who will need to activate a SSP will know which one he has
to activate.

>
> Each i2c node will save around 5 lines if we common it up, but if the
> interrupt number or map changes this difference will be negligible.
>
> Common up at this level and mixing un-common ones in stih415.dtsi or
> stih416.dtsi will add confusion to readers as the information is split
> at multiple places.
I agree it will be messy if one part of the node declared at one place,
and the rest at another place.
>
> IMO the common up idea sounds good but reduces the readability and has
> no effect on final dtb size.

Fair enough. Lee, are you ok with keeping it as is?

Thanks,
Maxime
>
> Thanks,
> srini
>
>
>> Regards,
>> Maxime
>>>
>>> thanks,
>>> srini
>>>
>>>
>>
>>
>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC
       [not found]                     ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD957D-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
@ 2013-09-19 15:32                       ` Lee Jones
  0 siblings, 0 replies; 28+ messages in thread
From: Lee Jones @ 2013-09-19 15:32 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Srinivas KANDAGATLA, Wolfram Sang, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	Russell King, Grant Likely,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

> > Am not very comfortable with this idea.
> >
> > As there is no guarantee that the interrupt number/memory map and the
> > i2c numbering will be same in future SOCs or other IPs.
> >
> > You might be already aware that the number of i2cs on each SOC are
> > different as example on STiH415 we have 10 SSCs and on STiH416 we have
> > 11 SSCs. So, At what point you decide that which devices/IPs should be
> > in stih41x and which should in stih415/Stih416?
> Yes, I know there is one more SSC on STiH416.
> 
> On one hand, this could add some confusion. But on the other hand,
> someone who will need to activate a SSP will know which one he has
> to activate.
> 
> > Each i2c node will save around 5 lines if we common it up, but if the
> > interrupt number or map changes this difference will be negligible.
> >
> > Common up at this level and mixing un-common ones in stih415.dtsi or
> > stih416.dtsi will add confusion to readers as the information is split
> > at multiple places.
> I agree it will be messy if one part of the node declared at one place,
> and the rest at another place.
> >
> > IMO the common up idea sounds good but reduces the readability and has
> > no effect on final dtb size.
> 
> Fair enough. Lee, are you ok with keeping it as is?

To be honest I haven't taken a look at the layout of the dts[i] files
yet, so I can't really comment. Srini knows then better than anyone,
so if he says it doesn't make sense, then I'm happy to take his word
for it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]     ` <5239A0ED.6010606-qxv4g6HH51o@public.gmane.org>
@ 2013-09-23 20:55       ` Stephen Warren
  0 siblings, 0 replies; 28+ messages in thread
From: Stephen Warren @ 2013-09-23 20:55 UTC (permalink / raw)
  To: Gabriel FERNANDEZ
  Cc: Maxime COQUELIN, Wolfram Sang, Srinivas KANDAGATLA, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Olivier CLERGEAUD

On 09/18/2013 06:47 AM, Gabriel FERNANDEZ wrote:
> On 18/09/2013 12:01, Maxime COQUELIN wrote:
>> This patch adds support to SSC (Synchronous Serial Controller)
>> I2C driver. This IP also supports SPI protocol, but this is not
>> the aim of this driver.
>>
>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>> supports I2C Standard and Fast modes.
> ... [entire patch quoted for a one-line comment] ...

Please trim down the patch so you only quote relevant lines. Quoting the
entire patch for a 1-line comment makes people wast time searching for
your reply.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]   ` <1379498483-4236-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-09-23 21:06     ` Stephen Warren
       [not found]       ` <5240AD6E.4090905-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Stephen Warren @ 2013-09-23 21:06 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Wolfram Sang, srinivas.kandagatla-qxv4g6HH51o, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, stephen.gallimore-qxv4g6HH51o,
	stuart.menefy-qxv4g6HH51o, Lee Jones,
	gabriel.fernandez-qxv4g6HH51o, olivier.clergeaud-qxv4g6HH51o

On 09/18/2013 04:01 AM, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt

> +I2C for ST platforms

If the HW block supports both I2C and SPI, the DT binding ought to
support that too. It's be best to create a single DT binding that
represents the IP block, and include a property that indicates whether
the device should operate in I2C or SPI mode.

I suppose you could reasonably define different compatible values for
those two cases. However, the binding should be titled something more
like "ST SSC binding, for I2C mode operation" and "ST SSC binding, for
SPI mode operation", rather than "I2C for ST platforms", since the HW
includes an SSC block, not an I2C block.

> +Required properties :
> +- compatible : Must be "st,comms-i2c"
> +- reg : Offset and length of the register set for the device
> +- interrupts : the interrupt number

It's an interrupt specifier, not an interrupt number. The format is
defined by the interrupt controller, not this binding.

> +- clocks : phandle to the I2C clock source

What about clock-names?

> +Recommended (non-standard) properties :

Usually you'd just say "Optional properties:", or perhaps "Recommended
properties:". I don't think adding "(non-standard)" serves any purpose.

> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
> +  are supported, possible values are 100000 and 400000.
> +
> +Optional (non-standard) properties:

Same here.

> +- st,glitches : Enable timing glitch suppression support.

That property name doesn't really convey the "enables" that appears in
the property description to me...

> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.
> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.

s/timinig/timing/

> +- st,hw-glitches : Enable filter glitch suppression support.
> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.
> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.

Those sound more like runtime configuration rather than HW description.
Can you rephrase the descriptions (and property names) more along the
lines of HW properties? Perhaps e.g.:

st,needs-glitch-suppression: The board design needs timing glitch
suppression enabled to operate reliably.

st,min-scl-pulse-width: The minimum valid SCL pulse width that is
allowed through the deglitch circuit. In units of ns.

(I just made up those descriptions to give a feel for the flavor of
description that I expect. They likely need some adjustment to reflect
whatever they're actually intended to represent in HW).

What is the difference between "glitch" and "hw-glitch"?

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]       ` <5240AD6E.4090905-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
@ 2013-09-24 15:38         ` Maxime COQUELIN
       [not found]           ` <5241B1FA.6020500-qxv4g6HH51o@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-24 15:38 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Wolfram Sang, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	Olivier CLERGEAUD


On 09/23/2013 11:06 PM, Stephen Warren wrote:
> On 09/18/2013 04:01 AM, Maxime COQUELIN wrote:
>> This patch adds support to SSC (Synchronous Serial Controller)
>> I2C driver. This IP also supports SPI protocol, but this is not
>> the aim of this driver.
>>
>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>> supports I2C Standard and Fast modes.
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> +I2C for ST platforms
> If the HW block supports both I2C and SPI, the DT binding ought to
> support that too. It's be best to create a single DT binding that
> represents the IP block, and include a property that indicates whether
> the device should operate in I2C or SPI mode.
>
> I suppose you could reasonably define different compatible values for
> those two cases. However, the binding should be titled something more
> like "ST SSC binding, for I2C mode operation" and "ST SSC binding, for
> SPI mode operation", rather than "I2C for ST platforms", since the HW
> includes an SSC block, not an I2C block.
You are right Stephen.
I will rename the title and change the compatible value as per your 
recommendation.

>> +Required properties :
>> +- compatible : Must be "st,comms-i2c"
>> +- reg : Offset and length of the register set for the device
>> +- interrupts : the interrupt number
> It's an interrupt specifier, not an interrupt number. The format is
> defined by the interrupt controller, not this binding.
Ok. I will change to "the interrupt specifier".
>> +- clocks : phandle to the I2C clock source
> What about clock-names?
It will be added in next revision. It will be named "ssc"
>> +Recommended (non-standard) properties :
> Usually you'd just say "Optional properties:", or perhaps "Recommended
> properties:". I don't think adding "(non-standard)" serves any purpose.
Ok. then I will remove all the "non-standard" occurences.
>> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
>> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
>> +  are supported, possible values are 100000 and 400000.
>> +
>> +Optional (non-standard) properties:
> Same here.
>
>> +- st,glitches : Enable timing glitch suppression support.
> That property name doesn't really convey the "enables" that appears in
> the property description to me...
>
>> +- st,glitch-clk : SCL line timinig glitch suppression value in ns.
>> +- st,glitch-dat : SDA line timinig glitch suppression value in ns.
> s/timinig/timing/
>
>> +- st,hw-glitches : Enable filter glitch suppression support.
>> +- st,hw-glitch-clk : SCL line filter glitch suppression value in us.
>> +- st,hw-glitch-dat : SDA line filter glitch suppression value in us.
> Those sound more like runtime configuration rather than HW description.
> Can you rephrase the descriptions (and property names) more along the
> lines of HW properties? Perhaps e.g.:
>
> st,needs-glitch-suppression: The board design needs timing glitch
> suppression enabled to operate reliably.
>
> st,min-scl-pulse-width: The minimum valid SCL pulse width that is
> allowed through the deglitch circuit. In units of ns.
>
> (I just made up those descriptions to give a feel for the flavor of
> description that I expect. They likely need some adjustment to reflect
> whatever they're actually intended to represent in HW).
>
> What is the difference between "glitch" and "hw-glitch"?
"hw-glitch" is used to configure the anti-glitch filters.
It suppresses the pulses with a width lower than x microseconds.

"glitch" is is used to tune the I2C timing requirements, and has a 
nanosecond granularity.
These values are added to default timing values.
I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the 
i2c-s3c2410 driver.

I agree the names and descriptions are not clear, and even misleading.
I will come with a clearer implementation, as soon as I get 
clarification from HW team.

Thanks for the review,
Maxime

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]           ` <5241B1FA.6020500-qxv4g6HH51o@public.gmane.org>
@ 2013-09-24 15:59             ` Wolfram Sang
  2013-09-26  9:30               ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Wolfram Sang @ 2013-09-24 15:59 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Stephen Warren, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	Olivier CLERGEAUD

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]


> "glitch" is is used to tune the I2C timing requirements, and has a 
> nanosecond granularity.
> These values are added to default timing values.
> I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the 
> i2c-s3c2410 driver.

For that, we have the generic "i2c-sda-hold-time-ns" property. The s3c
driver hasn't been converted to use it, sadly.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-09-24 15:59             ` Wolfram Sang
@ 2013-09-26  9:30               ` Maxime COQUELIN
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-09-26  9:30 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Stephen Warren, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	Olivier CLERGEAUD


On 09/24/2013 05:59 PM, Wolfram Sang wrote:
>> "glitch" is is used to tune the I2C timing requirements, and has a
>> nanosecond granularity.
>> These values are added to default timing values.
>> I'm not 100% sure, but it looks like the "samsung,i2c-sda-delay" in the
>> i2c-s3c2410 driver.
> For that, we have the generic "i2c-sda-hold-time-ns" property. The s3c
> driver hasn't been converted to use it, sadly.
>
Ok, thank you for pointing this out.
After some checks, this property doesn't match with what is done in this 
driver.

In the v2 series I am sending, I removed the I2C timing tuning as it 
works on all
the boards I have tested with.
I will only keep the anti-glitch filtering, using private properties as 
I didn't find any generic
ones that could fit.

Regards,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found] ` <1380623952-4252-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-10-01 10:39   ` Maxime COQUELIN
       [not found]     ` <1380623952-4252-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-10-01 10:39 UTC (permalink / raw)
  To: Wolfram Sang, srinivas.kandagatla-qxv4g6HH51o, Rob Herring,
	Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
	Rob Landley, Russell King, Grant Likely,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA
  Cc: stephen.gallimore-qxv4g6HH51o, stuart.menefy-qxv4g6HH51o,
	Lee Jones, gabriel.fernandez-qxv4g6HH51o,
	kernel-F5mvAk5X5gdBDgjK7y7TUQ, Maxime Coquelin

This patch adds support to SSC (Synchronous Serial Controller)
I2C driver. This IP also supports SPI protocol, but this is not
the aim of this driver.

This IP is embedded in all ST SoCs for Set-top box platorms, and
supports I2C Standard and Fast modes.

Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-st.txt |   34 +
 drivers/i2c/busses/Kconfig                       |   10 +
 drivers/i2c/busses/Makefile                      |    1 +
 drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
 4 files changed, 912 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
 create mode 100644 drivers/i2c/busses/i2c-st.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
new file mode 100644
index 0000000..ea0572e
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
@@ -0,0 +1,34 @@
+ST SSC binding, for I2C mode operation
+
+Required properties :
+- compatible : Must be "st,comms-i2c"
+- reg : Offset and length of the register set for the device
+- interrupts : the interrupt specifier
+- clocks : phandle to the I2C clock source
+- clock-names : from common clock binding: Shall be "ssc"
+
+Recommended properties :
+- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
+  the default 100 kHz frequency will be used. As only Normal and Fast modes
+  are supported, possible values are 100000 and 400000.
+
+Optional properties :
+- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is allowed
+  through the deglitch circuit. In units of us.
+- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is allowed
+  through the deglitch circuit. In units of us.
+
+Examples :
+
+i2c0: i2c@fed40000 {
+	compatible	= "st,comms-ssc-i2c";
+	reg		= <0xfed40000 0x110>;
+	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+	clocks		= <&CLK_S_ICN_REG_0>;
+	clock-names	= "ssc";
+	clock-frequency = <400000>;
+	pinctrl-names	= "default";
+	pinctrl-0	= <&pinctrl_i2c0_default>;
+	i2c-min-scl-pulse-width-us = <0>;
+	i2c-min-sda-pulse-width-us = <5>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cdcbd83..67f85f9 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -695,6 +695,16 @@ config I2C_SIRF
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-sirf.
 
+config I2C_ST
+	tristate "STMicroelectronics SSC I2C support"
+	depends on ARCH_STI
+	help
+	  Enable this option to add support for STMicroelectronics SoCs
+	  hardware SSC (Synchronous Serial Controller) as an I2C controller.
+
+	  This driver can also be built as module. If so, the module
+	  will be called i2c-st.
+
 config I2C_STU300
 	tristate "ST Microelectronics DDC I2C interface"
 	depends on MACH_U300
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index d00997f..a0f52e4 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
+obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
new file mode 100644
index 0000000..a22e0b2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-st.c
@@ -0,0 +1,867 @@
+/*
+ * Copyright (C) 2013 STMicroelectronics
+ *
+ * I2C master mode controller driver, used in STMicroelectronics devices.
+ *
+ * Author: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/* SSC registers */
+#define SSC_BRG				0x000
+#define SSC_TBUF			0x004
+#define SSC_RBUF			0x008
+#define SSC_CTL				0x00C
+#define SSC_IEN				0x010
+#define SSC_STA				0x014
+#define SSC_I2C				0x018
+#define SSC_SLAD			0x01C
+#define SSC_REP_START_HOLD		0x020
+#define SSC_START_HOLD			0x024
+#define SSC_REP_START_SETUP		0x028
+#define SSC_DATA_SETUP			0x02C
+#define SSC_STOP_SETUP			0x030
+#define SSC_BUS_FREE			0x034
+#define SSC_TX_FSTAT			0x038
+#define SSC_RX_FSTAT			0x03C
+#define SSC_PRE_SCALER_BRG		0x040
+#define SSC_CLR				0x080
+#define SSC_NOISE_SUPP_WIDTH		0x100
+#define SSC_PRSCALER			0x104
+#define SSC_NOISE_SUPP_WIDTH_DATAOUT	0x108
+#define SSC_PRSCALER_DATAOUT		0x10c
+
+/* SSC Control */
+#define SSC_CTL_DATA_WIDTH_9		0x8
+#define SSC_CTL_DATA_WIDTH_MSK		0xf
+#define SSC_CTL_BM			0xf
+#define SSC_CTL_HB			BIT(4)
+#define SSC_CTL_PH			BIT(5)
+#define SSC_CTL_PO			BIT(6)
+#define SSC_CTL_SR			BIT(7)
+#define SSC_CTL_MS			BIT(8)
+#define SSC_CTL_EN			BIT(9)
+#define SSC_CTL_LPB			BIT(10)
+#define SSC_CTL_EN_TX_FIFO		BIT(11)
+#define SSC_CTL_EN_RX_FIFO		BIT(12)
+#define SSC_CTL_EN_CLST_RX		BIT(13)
+
+/* SSC Interrupt Enable */
+#define SSC_IEN_RIEN			BIT(0)
+#define SSC_IEN_TIEN			BIT(1)
+#define SSC_IEN_TEEN			BIT(2)
+#define SSC_IEN_REEN			BIT(3)
+#define SSC_IEN_PEEN			BIT(4)
+#define SSC_IEN_AASEN			BIT(6)
+#define SSC_IEN_STOPEN			BIT(7)
+#define SSC_IEN_ARBLEN			BIT(8)
+#define SSC_IEN_NACKEN			BIT(10)
+#define SSC_IEN_REPSTRTEN		BIT(11)
+#define SSC_IEN_TX_FIFO_HALF		BIT(12)
+#define SSC_IEN_RX_FIFO_HALF_FULL	BIT(14)
+
+/* SSC Status */
+#define SSC_STA_RIR			BIT(0)
+#define SSC_STA_TIR			BIT(1)
+#define SSC_STA_TE			BIT(2)
+#define SSC_STA_RE			BIT(3)
+#define SSC_STA_PE			BIT(4)
+#define SSC_STA_CLST			BIT(5)
+#define SSC_STA_AAS			BIT(6)
+#define SSC_STA_STOP			BIT(7)
+#define SSC_STA_ARBL			BIT(8)
+#define SSC_STA_BUSY			BIT(9)
+#define SSC_STA_NACK			BIT(10)
+#define SSC_STA_REPSTRT			BIT(11)
+#define SSC_STA_TX_FIFO_HALF		BIT(12)
+#define SSC_STA_TX_FIFO_FULL		BIT(13)
+#define SSC_STA_RX_FIFO_HALF		BIT(14)
+
+/* SSC I2C Control */
+#define SSC_I2C_I2CM			BIT(0)
+#define SSC_I2C_STRTG			BIT(1)
+#define SSC_I2C_STOPG			BIT(2)
+#define SSC_I2C_ACKG			BIT(3)
+#define SSC_I2C_AD10			BIT(4)
+#define SSC_I2C_TXENB			BIT(5)
+#define SSC_I2C_REPSTRTG		BIT(11)
+#define SSC_I2C_SLAVE_DISABLE		BIT(12)
+
+/* SSC Tx FIFO Status */
+#define SSC_TX_FSTAT_STATUS		0x07
+
+/* SSC Rx FIFO Status */
+#define SSC_RX_FSTAT_STATUS		0x07
+
+/* SSC Clear bit operation */
+#define SSC_CLR_SSCAAS			BIT(6)
+#define SSC_CLR_SSCSTOP			BIT(7)
+#define SSC_CLR_SSCARBL			BIT(8)
+#define SSC_CLR_NACK			BIT(10)
+#define SSC_CLR_REPSTRT			BIT(11)
+
+/* SSC Clock Prescaler */
+#define SSC_PRSC_VALUE			0x0f
+
+
+#define SSC_TXFIFO_SIZE			0x8
+#define SSC_RXFIFO_SIZE			0x8
+
+enum st_i2c_mode {
+	I2C_MODE_STANDARD,
+	I2C_MODE_FAST,
+	I2C_MODE_END,
+};
+
+/**
+ * struct st_i2c_timings - per-Mode tuning parameters
+ * @rate: I2C bus rate
+ * @rep_start_hold: I2C repeated start hold time requirement
+ * @rep_start_setup: I2C repeated start set up time requirement
+ * @start_hold: I2C start hold time requirement
+ * @data_setup_time: I2C data set up time requirement
+ * @stop_setup_time: I2C stop set up time requirement
+ * @bus_free_time: I2C bus free time requirement
+ * @sda_pulse_min_limit: I2C SDA pulse mini width limit
+ */
+struct st_i2c_timings {
+	u32 rate;
+	u32 rep_start_hold;
+	u32 rep_start_setup;
+	u32 start_hold;
+	u32 data_setup_time;
+	u32 stop_setup_time;
+	u32 bus_free_time;
+	u32 sda_pulse_min_limit;
+};
+
+/**
+ * struct st_i2c_deglitch - Anti-glitch filter configuration
+ * @scl_min_width_us: SCL line minimum pulse width in ns
+ * @sda_min_width_us: SDA line minimum pulse width in ns
+ */
+struct st_i2c_deglitch {
+	u32	scl_min_width_us;
+	u32	sda_min_width_us;
+};
+
+/**
+ * struct st_i2c_client - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transfered
+ * @xfered: number of bytes already transferred
+ * @buf: data buffer
+ * @result: result of the transfer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ */
+struct st_i2c_client {
+	u8	addr;
+	u32	count;
+	u32	xfered;
+	u8	*buf;
+	int	result;
+	bool	stop;
+};
+
+/**
+ * struct st_i2c_dev - private data of the controller
+ * @adap: I2C adapter for this controller
+ * @dev: device for this controller
+ * @base: virtual memory area
+ * @complete: completion of I2C message
+ * @irq: interrupt line for th controller
+ * @clk: hw ssc block clock
+ * @mode: I2C mode of the controller. Standard or Fast only supported
+ * @deglitch: Anti-glitch filters parameters
+ * @client: I2C transfert information
+ * @busy: I2C transfer on-going
+ */
+struct st_i2c_dev {
+	struct i2c_adapter	adap;
+	struct device		*dev;
+	void __iomem		*base;
+	struct completion	complete;
+	int			irq;
+	struct clk		*clk;
+	int			mode;
+	struct st_i2c_deglitch	deglitch;
+	struct st_i2c_client	client;
+	bool			busy;
+};
+
+static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) | mask, reg);
+}
+
+static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) & ~mask, reg);
+}
+
+/* From I2C Specifications v0.5 */
+static struct st_i2c_timings i2c_timings[] = {
+	[I2C_MODE_STANDARD] = {
+		.rate			= 100000,
+		.rep_start_hold		= 4000,
+		.rep_start_setup	= 4700,
+		.start_hold		= 4000,
+		.data_setup_time	= 250,
+		.stop_setup_time	= 4000,
+		.bus_free_time		= 4700,
+	},
+	[I2C_MODE_FAST] = {
+		.rate			= 400000,
+		.rep_start_hold		= 600,
+		.rep_start_setup	= 600,
+		.start_hold		= 600,
+		.data_setup_time	= 100,
+		.stop_setup_time	= 600,
+		.bus_free_time		= 1300,
+	},
+};
+
+static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	int count, i;
+
+	/*
+	 * Counter only counts up to 7 but fifo size is 8...
+	 * When fifo is full, counter is 0 and RIR bit of status register is
+	 * set
+	 */
+	if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
+		count = SSC_RXFIFO_SIZE;
+	else
+		count = readl(i2c_dev->base + SSC_RX_FSTAT) &
+			SSC_RX_FSTAT_STATUS;
+
+	for (i = 0; i < count; i++)
+		readl(i2c_dev->base + SSC_RBUF);
+}
+
+static void st_i2c_soft_reset(struct st_i2c_dev *i2c_dev)
+{
+	/*
+	 * FIFO needs to be emptied before reseting the IP,
+	 * else the controller raises a BUSY error.
+	 */
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+	st_i2c_clr_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+}
+
+/**
+ * st_i2c_hw_config() - Prepare SSC block, calculate and apply tuning timings
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_hw_config(struct st_i2c_dev *i2c_dev)
+{
+	unsigned long rate;
+	u32 val, ns_per_clk;
+	struct st_i2c_timings *t = &i2c_timings[i2c_dev->mode];
+
+	st_i2c_soft_reset(i2c_dev);
+
+	val = SSC_CLR_REPSTRT | SSC_CLR_NACK | SSC_CLR_SSCARBL |
+		SSC_CLR_SSCAAS | SSC_CLR_SSCSTOP;
+	writel(val, i2c_dev->base + SSC_CLR);
+
+	/* SSC Control register setup */
+	val = SSC_CTL_PO | SSC_CTL_PH | SSC_CTL_HB | SSC_CTL_DATA_WIDTH_9;
+	writel(val, i2c_dev->base + SSC_CTL);
+
+	rate = clk_get_rate(i2c_dev->clk);
+	ns_per_clk = 1000000000 / rate;
+
+	/* Baudrate */
+	val = rate / (2 * t->rate);
+	writel(val, i2c_dev->base + SSC_BRG);
+
+	/* Pre-scaler baudrate */
+	writel(1, i2c_dev->base + SSC_PRE_SCALER_BRG);
+
+	/* Enable I2C mode */
+	writel(SSC_I2C_I2CM, i2c_dev->base + SSC_I2C);
+
+	/* Repeated start hold time */
+	val = t->rep_start_hold / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_HOLD);
+
+	/* Repeated start set up time */
+	val = t->rep_start_setup / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_SETUP);
+
+	/* Start hold time */
+	val = t->start_hold / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_START_HOLD);
+
+	/* Data set up time */
+	val = t->data_setup_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_DATA_SETUP);
+
+	/* Stop set up time */
+	val = t->stop_setup_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_STOP_SETUP);
+
+	/* Bus free time */
+	val = t->bus_free_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_BUS_FREE);
+
+	/* Prescalers set up */
+	val = rate / 10000000;
+	writel(val, i2c_dev->base + SSC_PRSCALER);
+	writel(val, i2c_dev->base + SSC_PRSCALER_DATAOUT);
+
+	/* Noise suppression witdh */
+	val = i2c_dev->deglitch.scl_min_width_us * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
+
+	/* Noise suppression max output data delay width */
+	val = i2c_dev->deglitch.sda_min_width_us * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH_DATAOUT);
+}
+
+static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
+{
+	u32 sta;
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		sta = readl(i2c_dev->base + SSC_STA);
+		if (!(sta & SSC_STA_BUSY))
+			return 0;
+
+		usleep_range(2000, 4000);
+	}
+
+	dev_err(i2c_dev->dev, "bus not free (status = 0x%08x)\n", sta);
+
+	return -EBUSY;
+}
+
+/**
+ * st_i2c_write_tx_fifo() - Write a byte in the Tx FIFO
+ * @i2c_dev: Controller's private data
+ * @byte: Data to write in the Tx FIFO
+ */
+static inline void st_i2c_write_tx_fifo(struct st_i2c_dev *i2c_dev, u8 byte)
+{
+	u16 tbuf = byte << 1;
+
+	writel(tbuf | 1, i2c_dev->base + SSC_TBUF);
+}
+
+/**
+ * st_i2c_wr_fill_tx_fifo() - Fill the Tx FIFO in write mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with I2C transfert buffer when
+ * in write mode.
+ */
+static void st_i2c_wr_fill_tx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (c->count < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = c->count;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->count--, c->buf++)
+		st_i2c_write_tx_fifo(i2c_dev, *c->buf);
+}
+
+/**
+ * st_i2c_rd_fill_tx_fifo() - Fill the Tx FIFO in read mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with fixed pattern when
+ * in read mode to trigger clock.
+ */
+static void st_i2c_rd_fill_tx_fifo(struct st_i2c_dev *i2c_dev, int max)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (max < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = max;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->xfered++)
+		st_i2c_write_tx_fifo(i2c_dev, 0xff);
+}
+
+static void st_i2c_read_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 i, sta;
+	u16 rbuf;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_RIR) {
+		i = SSC_RXFIFO_SIZE;
+	} else {
+		i = readl(i2c_dev->base + SSC_RX_FSTAT);
+		i &= SSC_RX_FSTAT_STATUS;
+	}
+
+	for (; (i > 0) && (c->count > 0); i--, c->count--) {
+		rbuf = readl(i2c_dev->base + SSC_RBUF) >> 1;
+		*c->buf++ = (u8)rbuf & 0xff;
+	}
+
+	if (i) {
+		dev_err(i2c_dev->dev, "Unexpected %d bytes in rx fifo\n", i);
+		st_i2c_flush_rx_fifo(i2c_dev);
+	}
+}
+
+/**
+ * st_i2c_terminate_xfer() - Send either STOP or REPSTART condition
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_terminate_xfer(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_clr_bits(i2c_dev->base + SSC_IEN, SSC_IEN_TEEN);
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+
+	if (c->stop) {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_STOPEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+	} else {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_REPSTRTEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_REPSTRTG);
+	}
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO empty interrupt in case of write
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_write(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	if (!c->count)
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	else
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ien;
+
+	/* Trash the address read back */
+	if (!c->xfered) {
+		readl(i2c_dev->base + SSC_RBUF);
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
+	} else
+		st_i2c_read_rx_fifo(i2c_dev);
+
+	if (!c->count) {
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	} else if (c->count == 1) {
+		/* Penultimate byte to xfer, disable ACK gen. */
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
+
+		/* Last received byte is to be handled by NACK interrupt */
+		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
+		writel(ien, i2c_dev->base + SSC_IEN);
+
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
+	} else
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
+}
+
+/**
+ * st_i2c_isr() - Interrupt routine
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t st_i2c_isr(int irq, void *data)
+{
+	struct st_i2c_dev *i2c_dev = data;
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 sta, ien;
+	int it;
+
+	ien = readl(i2c_dev->base + SSC_IEN);
+	sta = readl(i2c_dev->base + SSC_STA);
+
+	/* Use __fls() to check error bits first */
+	it = __fls(sta & ien);
+	if (it < 0) {
+		dev_dbg(i2c_dev->dev, "spurious it (sta=0x%04x, ien=0x%04x)\n",
+				sta, ien);
+		return IRQ_NONE;
+	}
+
+	switch (1 << it) {
+	case SSC_STA_TE:
+		if (c->addr & I2C_M_RD)
+			st_i2c_handle_read(i2c_dev);
+		else
+			st_i2c_handle_write(i2c_dev);
+		break;
+
+	case SSC_STA_STOP:
+	case SSC_STA_REPSTRT:
+		writel(0, i2c_dev->base + SSC_IEN);
+		complete(&i2c_dev->complete);
+		break;
+
+	case SSC_STA_NACK:
+		writel(SSC_CLR_NACK, i2c_dev->base + SSC_CLR);
+
+		/* Last received byte handled by NACK interrupt */
+		if ((c->addr & I2C_M_RD) && (c->count == 1) && (c->xfered)) {
+			st_i2c_handle_read(i2c_dev);
+			break;
+		}
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	case SSC_STA_ARBL:
+		writel(SSC_CLR_SSCARBL, i2c_dev->base + SSC_CLR);
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	default:
+		dev_err(i2c_dev->dev,
+				"it %d unhandled (sta=0x%04x)\n", it, sta);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * st_i2c_xfer_msg() - Transfer a single I2C message
+ * @i2c_dev: Controller's private data
+ * @msg: I2C message to transfer
+ * @is_first: first message of the sequence
+ * @is_last: last message of the sequence
+ */
+static int st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
+			    bool is_first, bool is_last)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ctl, i2c, it;
+	unsigned long timeout;
+	int ret;
+
+	c->addr		= (u8)(msg->addr << 1);
+	c->addr		|= (msg->flags & I2C_M_RD);
+	c->buf		= msg->buf;
+	c->count	= msg->len;
+	c->xfered	= 0;
+	c->result	= 0;
+	c->stop		= is_last;
+
+	INIT_COMPLETION(i2c_dev->complete);
+
+	ctl = SSC_CTL_EN | SSC_CTL_MS |	SSC_CTL_EN_RX_FIFO | SSC_CTL_EN_TX_FIFO;
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, ctl);
+
+	i2c = SSC_I2C_TXENB;
+	if (c->addr & I2C_M_RD)
+		i2c |= SSC_I2C_ACKG;
+	st_i2c_set_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	/* Write slave address */
+	st_i2c_write_tx_fifo(i2c_dev, c->addr);
+
+	/* Pre-fill Tx fifo with data in case of write */
+	if (!(c->addr & I2C_M_RD))
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+
+	it = SSC_IEN_NACKEN | SSC_IEN_TEEN | SSC_IEN_ARBLEN;
+	writel(it, i2c_dev->base + SSC_IEN);
+
+	if (is_first) {
+		ret = st_i2c_wait_free_bus(i2c_dev);
+		if (ret)
+			return ret;
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+	}
+
+	timeout = wait_for_completion_timeout(&i2c_dev->complete,
+			i2c_dev->adap.timeout);
+	ret = c->result;
+
+	if (!timeout) {
+		dev_err(i2c_dev->dev, "Write to slave 0x%x timed out\n",
+				c->addr);
+		ret = -ETIMEDOUT;
+	}
+
+	i2c = SSC_I2C_STOPG | SSC_I2C_REPSTRTG;
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	writel(SSC_CLR_SSCSTOP | SSC_CLR_REPSTRT, i2c_dev->base + SSC_CLR);
+
+	return ret;
+}
+
+/**
+ * st_i2c_xfer() - Transfer a single I2C message
+ * @i2c_adap: Adapter pointer to the controller
+ * @msgs: Pointer to data to be written.
+ * @num: Number of messages to be executed
+ */
+static int st_i2c_xfer(struct i2c_adapter *i2c_adap,
+			struct i2c_msg msgs[], int num)
+{
+	struct st_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+	int ret, i;
+
+	i2c_dev->busy = true;
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+
+	st_i2c_hw_config(i2c_dev);
+
+	for (i = 0; (i < num) && !ret; i++)
+		ret = st_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0, i == num - 1);
+
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	clk_disable_unprepare(i2c_dev->clk);
+
+	i2c_dev->busy = false;
+
+	return (ret < 0) ? ret : i;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int st_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev =
+		container_of(dev, struct platform_device, dev);
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	if (i2c_dev->busy)
+		return -EBUSY;
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int st_i2c_resume(struct device *dev)
+{
+	pinctrl_pm_select_default_state(dev);
+	/* Go in idle state if available */
+	pinctrl_pm_select_idle_state(dev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(st_i2c_pm, st_i2c_suspend, st_i2c_resume);
+#define ST_I2C_PM	(&st_i2c_pm)
+#else
+#define ST_I2C_PM	NULL
+#endif
+
+static u32 st_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm st_i2c_algo = {
+	.master_xfer = st_i2c_xfer,
+	.functionality = st_i2c_func,
+};
+
+static int st_i2c_of_get_deglitch(struct device_node *np,
+		struct st_i2c_dev *i2c_dev)
+{
+	int ret;
+
+	ret = of_property_read_u32(np, "i2c-min-scl-pulse-width-us",
+			&i2c_dev->deglitch.scl_min_width_us);
+	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
+		dev_err(i2c_dev->dev, "i2c-min-scl-pulse-width-us invalid\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "i2c-min-sda-pulse-width-us",
+			&i2c_dev->deglitch.sda_min_width_us);
+	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
+		dev_err(i2c_dev->dev, "i2c-min-sda-pulse-width-us invalid\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int st_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct st_i2c_dev *i2c_dev;
+	struct resource *res;
+	u32 clk_rate;
+	struct i2c_adapter *adap;
+	int ret;
+
+	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
+	if (!i2c_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	i2c_dev->irq = irq_of_parse_and_map(np, 0);
+	if (!i2c_dev->irq) {
+		dev_err(&pdev->dev, "IRQ missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = of_clk_get_by_name(np, "ssc");
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(&pdev->dev, "Unable to request clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+
+	i2c_dev->mode = I2C_MODE_STANDARD;
+	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
+	if ((!ret) && (clk_rate == 400000))
+		i2c_dev->mode = I2C_MODE_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
+			pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+	/* In case idle state available, select it */
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	ret = st_i2c_of_get_deglitch(np, i2c_dev);
+	if (ret)
+		return ret;
+
+	adap = &i2c_dev->adap;
+	i2c_set_adapdata(adap, i2c_dev);
+	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
+	adap->owner = THIS_MODULE;
+	adap->timeout = 2 * HZ;
+	adap->retries = 0;
+	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
+	adap->algo = &st_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	init_completion(&i2c_dev->complete);
+
+	ret = i2c_add_adapter(adap);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add adapter\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
+
+	return 0;
+}
+
+static int st_i2c_remove(struct platform_device *pdev)
+{
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c_dev->adap);
+
+	return 0;
+}
+
+static struct of_device_id st_i2c_match[] = {
+	{ .compatible = "st,comms-ssc-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_i2c_match);
+
+static struct platform_driver st_i2c_driver = {
+	.driver = {
+		.name = "st-i2c",
+		.owner = THIS_MODULE,
+		.of_match_table = st_i2c_match,
+		.pm = ST_I2C_PM,
+	},
+	.probe = st_i2c_probe,
+	.remove = st_i2c_remove,
+};
+
+module_platform_driver(st_i2c_driver);
+
+MODULE_AUTHOR("Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>");
+MODULE_DESCRIPTION("STMicroelectronics I2C driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]     ` <1380623952-4252-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
@ 2013-10-01 20:45       ` Stephen Warren
  2013-10-02  8:36         ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Stephen Warren @ 2013-10-01 20:45 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Wolfram Sang, srinivas.kandagatla-qxv4g6HH51o, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, stephen.gallimore-qxv4g6HH51o,
	stuart.menefy-qxv4g6HH51o, Lee Jones,
	gabriel.fernandez-qxv4g6HH51o, kernel-F5mvAk5X5gdBDgjK7y7TUQ

On 10/01/2013 04:39 AM, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt

> +Required properties :

> +- clocks : phandle to the I2C clock source
> +- clock-names : from common clock binding: Shall be "ssc"

I'd prefer to define that as:

clock-names: Must contain "ssc".
clocks: Must contain an entry for each name in clock-names. See the
common clock bindings.

That way, it makes it clear that clock-names is the primary lookup
mechanism, rather than some auxiliary documentation.

> +Recommended properties :
> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise

s/Otherwise/If not specified,/

> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
> +  are supported, possible values are 100000 and 400000.

I think that's just optional. Since there's a well-defined sensible
default, there's no need to recommend it.

> +Optional properties :
> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is allowed
> +  through the deglitch circuit. In units of us.
> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is allowed
> +  through the deglitch circuit. In units of us.

Are those properties specific to this binding, or intended to be
generic? If specific to this binding, a vendor prefix should be present
in the property name. If not, you probably want to document the
properties in some common file.

> +Examples :

s/Examples/Example/ since there's just one.

> +i2c0: i2c@fed40000 {
> +	compatible	= "st,comms-ssc-i2c";
> +	reg		= <0xfed40000 0x110>;
> +	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +	clocks		= <&CLK_S_ICN_REG_0>;
> +	clock-names	= "ssc";
> +	clock-frequency = <400000>;
> +	pinctrl-names	= "default";
> +	pinctrl-0	= <&pinctrl_i2c0_default>;

That wasn't mentioned in the binding definition. You'd probably want to
document the requirement for those two properties by saying something like:

A pinctrl state named "default" must be defined, using the bindings in
../pinctrl/pinctrl-binding.txt.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-10-01 20:45       ` Stephen Warren
@ 2013-10-02  8:36         ` Maxime COQUELIN
  2013-10-02  9:02           ` Wolfram Sang
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-10-02  8:36 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Mark Rutland, devicetree@vger.kernel.org, Russell King,
	kernel@stlinux.com, Pawel Moll, Ian Campbell, Wolfram Sang,
	Srinivas KANDAGATLA, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, Rob Herring, Stephen GALLIMORE,
	Stuart MENEFY, linux-i2c@vger.kernel.org, Rob Landley,
	Grant Likely, Lee Jones, Gabriel FERNANDEZ,
	linux-arm-kernel@lists.infradead.org


On 10/01/2013 10:45 PM, Stephen Warren wrote:
> On 10/01/2013 04:39 AM, Maxime COQUELIN wrote:
>> This patch adds support to SSC (Synchronous Serial Controller)
>> I2C driver. This IP also supports SPI protocol, but this is not
>> the aim of this driver.
>>
>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>> supports I2C Standard and Fast modes.
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> +Required properties :
>> +- clocks : phandle to the I2C clock source
>> +- clock-names : from common clock binding: Shall be "ssc"
> I'd prefer to define that as:
>
> clock-names: Must contain "ssc".
> clocks: Must contain an entry for each name in clock-names. See the
> common clock bindings.
>
> That way, it makes it clear that clock-names is the primary lookup
> mechanism, rather than some auxiliary documentation.
OK. This will be done in next series.

>
>> +Recommended properties :
>> +- clock-frequency : Desired I2C bus clock frequency in Hz. Otherwise
> s/Otherwise/If not specified,/
Done.
>
>> +  the default 100 kHz frequency will be used. As only Normal and Fast modes
>> +  are supported, possible values are 100000 and 400000.
> I think that's just optional. Since there's a well-defined sensible
> default, there's no need to recommend it.
OK, I move it to optional.
>> +Optional properties :
>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is allowed
>> +  through the deglitch circuit. In units of us.
>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is allowed
>> +  through the deglitch circuit. In units of us.
> Are those properties specific to this binding, or intended to be
> generic? If specific to this binding, a vendor prefix should be present
> in the property name. If not, you probably want to document the
> properties in some common file.
Ok.
In last revision, I put this properties as specific to this binding.
Wolfram proposed to make this generic, but it looks like this IP is the 
only one
needing such properties.

Wolfram, what would you advise?
If you still prefer to make this properties generics, in which file should I
document it? I don't see any common i2c binding document for now.

>
>> +Examples :
> s/Examples/Example/ since there's just one.
Ok.
>
>> +i2c0: i2c@fed40000 {
>> +	compatible	= "st,comms-ssc-i2c";
>> +	reg		= <0xfed40000 0x110>;
>> +	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
>> +	clocks		= <&CLK_S_ICN_REG_0>;
>> +	clock-names	= "ssc";
>> +	clock-frequency = <400000>;
>> +	pinctrl-names	= "default";
>> +	pinctrl-0	= <&pinctrl_i2c0_default>;
> That wasn't mentioned in the binding definition. You'd probably want to
> document the requirement for those two properties by saying something like:
>
> A pinctrl state named "default" must be defined, using the bindings in
> ../pinctrl/pinctrl-binding.txt.
Ok. I will also add the "sleep" state in the optional definitions.

Thanks for the review Stephen.

Regards,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-10-02  8:36         ` Maxime COQUELIN
@ 2013-10-02  9:02           ` Wolfram Sang
  2013-10-02  9:35             ` Maxime COQUELIN
  0 siblings, 1 reply; 28+ messages in thread
From: Wolfram Sang @ 2013-10-02  9:02 UTC (permalink / raw)
  To: Maxime COQUELIN
  Cc: Stephen Warren, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	kernel@stlinux.com

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]


> >> +Optional properties :
> >> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is allowed
> >> +  through the deglitch circuit. In units of us.
> >> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is allowed
> >> +  through the deglitch circuit. In units of us.
> > Are those properties specific to this binding, or intended to be
> > generic? If specific to this binding, a vendor prefix should be present
> > in the property name. If not, you probably want to document the
> > properties in some common file.
> Ok.
> In last revision, I put this properties as specific to this binding.
> Wolfram proposed to make this generic, but it looks like this IP is the 
> only one
> needing such properties.
> 
> Wolfram, what would you advise?

It might be the only SoC now, but I could imagine that other will have
something similar in the future. I am not perfectly sure, though. So, I
asked for opinions from DT experts when I suggested those bindings. We
could start with vendor specific bindings and generalize them later if
similar ones appear. Yet my experience is that old drivers rarely get
converted to the new bindings.

> If you still prefer to make this properties generics, in which file should I
> document it? I don't see any common i2c binding document for now.

Yeah, it is missing sadly. That's on my todo-list, like many other
things...

Regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
  2013-10-02  9:02           ` Wolfram Sang
@ 2013-10-02  9:35             ` Maxime COQUELIN
       [not found]               ` <524BE8FB.40000-qxv4g6HH51o@public.gmane.org>
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime COQUELIN @ 2013-10-02  9:35 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Stephen Warren, Srinivas KANDAGATLA, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	kernel@stlinux.com


On 10/02/2013 11:02 AM, Wolfram Sang wrote:
>>>> +Optional properties :
>>>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is allowed
>>>> +  through the deglitch circuit. In units of us.
>>>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is allowed
>>>> +  through the deglitch circuit. In units of us.
>>> Are those properties specific to this binding, or intended to be
>>> generic? If specific to this binding, a vendor prefix should be present
>>> in the property name. If not, you probably want to document the
>>> properties in some common file.
>> Ok.
>> In last revision, I put this properties as specific to this binding.
>> Wolfram proposed to make this generic, but it looks like this IP is the
>> only one
>> needing such properties.
>>
>> Wolfram, what would you advise?
> It might be the only SoC now, but I could imagine that other will have
> something similar in the future. I am not perfectly sure, though. So, I
> asked for opinions from DT experts when I suggested those bindings. We
> could start with vendor specific bindings and generalize them later if
> similar ones appear. Yet my experience is that old drivers rarely get
> converted to the new bindings.
Ok.
But if I start with vendor specific bindings, we will have to support it
forever, right?
>
>> If you still prefer to make this properties generics, in which file should I
>> document it? I don't see any common i2c binding document for now.
> Yeah, it is missing sadly. That's on my todo-list, like many other
> things...
OK :-)

Thanks,

Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller
       [not found]               ` <524BE8FB.40000-qxv4g6HH51o@public.gmane.org>
@ 2013-10-02 13:56                 ` Maxime COQUELIN
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime COQUELIN @ 2013-10-02 13:56 UTC (permalink / raw)
  To: Pawel Moll
  Cc: Wolfram Sang, Stephen Warren, Srinivas KANDAGATLA, Rob Herring,
	Mark Rutland, Ian Campbell, Rob Landley, Russell King,
	Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Stephen GALLIMORE, Stuart MENEFY, Lee Jones, Gabriel FERNANDEZ,
	kernel-F5mvAk5X5gdBDgjK7y7TUQ@public.gmane.org

Hi Pawel,

On 10/02/2013 11:35 AM, Maxime Coquelin wrote:
>
> On 10/02/2013 11:02 AM, Wolfram Sang wrote:
>>>>> +Optional properties :
>>>>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width 
>>>>> that is allowed
>>>>> +  through the deglitch circuit. In units of us.
>>>>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width 
>>>>> that is allowed
>>>>> +  through the deglitch circuit. In units of us.
>>>> Are those properties specific to this binding, or intended to be
>>>> generic? If specific to this binding, a vendor prefix should be 
>>>> present
>>>> in the property name. If not, you probably want to document the
>>>> properties in some common file.
>>> Ok.
>>> In last revision, I put this properties as specific to this binding.
>>> Wolfram proposed to make this generic, but it looks like this IP is the
>>> only one
>>> needing such properties.
>>>
>>> Wolfram, what would you advise?
>> It might be the only SoC now, but I could imagine that other will have
>> something similar in the future. I am not perfectly sure, though. So, I
>> asked for opinions from DT experts when I suggested those bindings. We
>> could start with vendor specific bindings and generalize them later if
>> similar ones appear. Yet my experience is that old drivers rarely get
>> converted to the new bindings.
> Ok.
> But if I start with vendor specific bindings, we will have to support it
> forever, right?
I would be glad to have your opinion on this.

Since there are no other vendors currently having this feature,
should we put these properties vendor specific?
Or put them generic in case of someone has the same feature in the future?

Thanks,
Maxime

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2013-10-02 13:56 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-18 10:01 [PATCH 0/4] Add I2C support to ST SoCs Maxime COQUELIN
2013-09-18 10:01 ` [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller Maxime COQUELIN
2013-09-18 12:47   ` Gabriel FERNANDEZ
     [not found]     ` <5239A0ED.6010606-qxv4g6HH51o@public.gmane.org>
2013-09-23 20:55       ` Stephen Warren
     [not found]   ` <1379498483-4236-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
2013-09-23 21:06     ` Stephen Warren
     [not found]       ` <5240AD6E.4090905-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-09-24 15:38         ` Maxime COQUELIN
     [not found]           ` <5241B1FA.6020500-qxv4g6HH51o@public.gmane.org>
2013-09-24 15:59             ` Wolfram Sang
2013-09-26  9:30               ` Maxime COQUELIN
     [not found] ` <1379498483-4236-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
2013-09-18 10:01   ` [PATCH 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC Maxime COQUELIN
     [not found]     ` <1379498483-4236-3-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
2013-09-18 12:03       ` Lee Jones
2013-09-18 12:46         ` Maxime COQUELIN
     [not found]           ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD9578-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
2013-09-18 12:57             ` Srinivas KANDAGATLA
2013-09-19  7:16               ` Maxime COQUELIN
2013-09-19 12:59                 ` Srinivas KANDAGATLA
2013-09-19 15:22                   ` Maxime COQUELIN
     [not found]                     ` <84625B87D65BCF478CC1E9C886A4C314DEF1BD957D-+EwDPpWUVoSs+H57zxxw29BPR1lH4CV8@public.gmane.org>
2013-09-19 15:32                       ` Lee Jones
2013-09-18 10:01 ` [PATCH 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC Maxime COQUELIN
2013-09-18 12:00   ` Lee Jones
2013-09-18 12:38     ` Maxime COQUELIN
2013-09-18 10:01 ` [PATCH 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards Maxime COQUELIN
2013-09-18 11:40   ` Lee Jones
2013-09-18 12:36     ` Maxime COQUELIN
  -- strict thread matches above, loose matches on Subject: below --
2013-10-01 10:39 [PATCH v3 0/4] Add I2C support to ST SoCs Maxime COQUELIN
     [not found] ` <1380623952-4252-1-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
2013-10-01 10:39   ` [PATCH 1/4] i2c: busses: i2c-st: Add ST I2C controller Maxime COQUELIN
     [not found]     ` <1380623952-4252-2-git-send-email-maxime.coquelin-qxv4g6HH51o@public.gmane.org>
2013-10-01 20:45       ` Stephen Warren
2013-10-02  8:36         ` Maxime COQUELIN
2013-10-02  9:02           ` Wolfram Sang
2013-10-02  9:35             ` Maxime COQUELIN
     [not found]               ` <524BE8FB.40000-qxv4g6HH51o@public.gmane.org>
2013-10-02 13:56                 ` Maxime COQUELIN

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).