"Syed Rafiuddin" <rafiuddin.syed@xxxxxx> writes:
> This patch Updates I2C register offset addresses and adds support for OMAP4430
> development platform.
>
> Signed-off-by: Syed Rafiuddin <rafiuddin.syed@xxxxxx>
First fix checkpatch problems:
WARNING: suspect code indent for conditional statements (24, 24)
#199: FILE: drivers/i2c/busses/i2c-omap.c:330:
+ if (!cpu_is_omap44xx())
omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
WARNING: line over 80 characters
#278: FILE: drivers/i2c/busses/i2c-omap.c:768:
+ "XDR IRQ while no "
WARNING: line over 80 characters
#279: FILE: drivers/i2c/busses/i2c-omap.c:769:
+ "data to send\n");
total: 0 errors, 3 warnings, 265 lines checked
Second, please also report test results on OMAP3 since you are changing
common code.
More comments below...
> ---
> arch/arm/mach-omap2/board-4430sdp.c | 9 +++-
> arch/arm/plat-omap/i2c.c | 21 +++++++--
> drivers/i2c/busses/i2c-omap.c | 78 +++++++++++++++++++++++++-----------
> 3 files changed, 80 insertions(+), 28 deletions(-)
>
> Index: omap4_dev/arch/arm/mach-omap2/board-4430sdp.c
> ===================================================================
> --- omap4_dev.orig/arch/arm/mach-omap2/board-4430sdp.c
> +++ omap4_dev/arch/arm/mach-omap2/board-4430sdp.c
> @@ -66,10 +66,17 @@ static void __init omap_4430sdp_init_irq
> gic_init_irq();
> omap_gpio_init();
> }
> -
> +static int __init omap4_i2c_init(void)
> +{
> + omap_register_i2c_bus(1, 2600, NULL, 0);
> + omap_register_i2c_bus(2, 400, NULL, 0);
> + omap_register_i2c_bus(3, 400, NULL, 0);
> + return 0;
> +}
>
> static void __init omap_4430sdp_init(void)
> {
> + omap4_i2c_init();
> platform_add_devices(sdp4430_devices, ARRAY_SIZE(sdp4430_devices));
> omap_board_config = sdp4430_config;
> omap_board_config_size = ARRAY_SIZE(sdp4430_config);
> Index: omap4_dev/arch/arm/plat-omap/i2c.c
> ===================================================================
> --- omap4_dev.orig/arch/arm/plat-omap/i2c.c
> +++ omap4_dev/arch/arm/plat-omap/i2c.c
> @@ -53,9 +53,15 @@ static struct resource i2c_resources[][2
> #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_24XX_I2C2_IRQ) },
> #endif
> +#if defined(CONFIG_ARCH_OMAP4)
> + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE2, INT_44XX_I2C2_IRQ) },
> +#endif
> #if defined(CONFIG_ARCH_OMAP34XX)
> { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_34XX_I2C3_IRQ) },
> #endif
> +#if defined(CONFIG_ARCH_OMAP4)
> + { I2C_RESOURCE_BUILDER(OMAP2_I2C_BASE3, INT_44XX_I2C3_IRQ) },
> +#endif
> };
>
> #define I2C_DEV_BUILDER(bus_id, res, data) \
> @@ -72,10 +78,11 @@ static struct resource i2c_resources[][2
> static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
> I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> -#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> +#if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX) || \
> +defined(CONFIG_ARCH_OMAP4)
> I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> #endif
> -#if defined(CONFIG_ARCH_OMAP34XX)
> +#if defined(CONFIG_ARCH_OMAP34XX) || defined(CONFIG_ARCH_OMAP4)
> I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> #endif
> };
> @@ -88,7 +95,7 @@ static const int omap24xx_pins[][2] = {
> #else
> static const int omap24xx_pins[][2] = {};
> #endif
> -#if defined(CONFIG_ARCH_OMAP34XX)
> +#if defined(CONFIG_ARCH_OMAP34XX) || defined(CONFIG_ARCH_OMAP4)
> static const int omap34xx_pins[][2] = {
> { K21_34XX_I2C1_SCL, J21_34XX_I2C1_SDA},
> { AF15_34XX_I2C2_SCL, AE15_34XX_I2C2_SDA},
> @@ -110,7 +117,7 @@ static void __init omap_i2c_mux_pins(int
> } else if (cpu_is_omap24xx()) {
> scl = omap24xx_pins[bus][0];
> sda = omap24xx_pins[bus][1];
> - } else if (cpu_is_omap34xx()) {
> + } else if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
> scl = omap34xx_pins[bus][0];
> sda = omap34xx_pins[bus][1];
> } else {
> @@ -129,7 +136,7 @@ static int __init omap_i2c_nr_ports(void
> ports = 1;
> else if (cpu_is_omap24xx())
> ports = 2;
> - else if (cpu_is_omap34xx())
> + else if (cpu_is_omap34xx() || cpu_is_omap44xx())
> ports = 3;
>
> return ports;
> @@ -151,6 +158,10 @@ static int __init omap_i2c_add_bus(int b
> base = OMAP2_I2C_BASE1;
> irq = INT_24XX_I2C1_IRQ;
> }
> + if (cpu_is_omap44xx()) {
> + base = OMAP2_I2C_BASE1;
> + irq = INT_44XX_I2C1_IRQ;
> + }
> res[0].start = base;
> res[0].end = base + OMAP_I2C_SIZE;
> res[1].start = irq;
> Index: omap4_dev/drivers/i2c/busses/i2c-omap.c
> ===================================================================
> --- omap4_dev.orig/drivers/i2c/busses/i2c-omap.c
> +++ omap4_dev/drivers/i2c/busses/i2c-omap.c
> @@ -27,7 +27,6 @@
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
> -
Please separate out the multiple whitespace cleanups here into a
separate patch.
> #include <linux/module.h>
> #include <linux/delay.h>
> #include <linux/i2c.h>
> @@ -48,12 +47,36 @@
> /* timeout waiting for the controller to respond */
> #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
>
> +#define OMAP_I2C_WE_REG 0x0c
> +#ifdef CONFIG_ARCH_OMAP4
> +#define OMAP_I2C_REVNB_LO 0x00
> +#define OMAP_I2C_REVNB_HI 0x04
> +#define OMAP_I2C_IV_REG 0x34
> +#define OMAP_I2C_IRQSTATUS_RAW 0x24
> +#define OMAP_I2C_STAT_REG 0x28
> +#define OMAP_I2C_IRQENABLE_SET 0x2C
> +#define OMAP_I2C_IRQENABLE_CLR 0x30
> +#define OMAP_I2C_SYSS_REG 0x90
> +#define OMAP_I2C_BUF_REG 0x94
> +#define OMAP_I2C_CNT_REG 0x98
> +#define OMAP_I2C_DATA_REG 0x9C
> +#define OMAP_I2C_SYSC_REG 0x10
> +#define OMAP_I2C_CON_REG 0xA4
> +#define OMAP_I2C_OA_REG 0xA8
> +#define OMAP_I2C_SA_REG 0xAC
> +#define OMAP_I2C_PSC_REG 0xB0
> +#define OMAP_I2C_SCLL_REG 0xB4
> +#define OMAP_I2C_SCLH_REG 0xB8
> +#define OMAP_I2C_SYSTEST_REG 0xBC
> +#define OMAP_I2C_BUFSTAT_REG 0xC0
> +#define OMAP_I2C_IE_REG OMAP_I2C_IRQENABLE_SET
> +#define OMAP_I2C_REV_REG OMAP_I2C_REVNB_HI
> +#else
A few things are broken here:
- This will not compile for multi-omap. You use #ifdef her and cpu_is_* below.
- use tabs instead of spaces as the rest of the code does.
- style is to use lower-case for hex values (e.g. 0xbc instead of 0xBC)
Rather than use an #ifdef here, you should define the omap4 specific
registers after the current list with an OMAP4 prefix.
> #define OMAP_I2C_REV_REG 0x00
> #define OMAP_I2C_IE_REG 0x04
> #define OMAP_I2C_STAT_REG 0x08
> #define OMAP_I2C_IV_REG 0x0c
> /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
> -#define OMAP_I2C_WE_REG 0x0c
> #define OMAP_I2C_SYSS_REG 0x10
> #define OMAP_I2C_BUF_REG 0x14
> #define OMAP_I2C_CNT_REG 0x18
> @@ -67,7 +90,8 @@
> #define OMAP_I2C_SCLH_REG 0x38
> #define OMAP_I2C_SYSTEST_REG 0x3c
> #define OMAP_I2C_BUFSTAT_REG 0x40
> -
> +#define OMAP_I2C_IRQENABLE_CLR OMAP_I2C_IE_REG
Why are you defining a new regname for !omap4? You're only
using this reg in omap4 conditional code below.
> +#endif
> /* I2C Interrupt Enable Register (OMAP_I2C_IE): */
> #define OMAP_I2C_IE_XDR (1 << 14) /* TX Buffer drain int enable */
> #define OMAP_I2C_IE_RDR (1 << 13) /* RX Buffer drain int enable */
> @@ -242,7 +266,10 @@ static void omap_i2c_idle(struct omap_i2
> WARN_ON(dev->idle);
>
> dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> - omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
> + if (cpu_is_omap44xx())
> + omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR, 1);
> + else
> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
> if (dev->rev < OMAP_I2C_REV_2) {
> iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
> } else {
> @@ -282,10 +309,8 @@ static int omap_i2c_init(struct omap_i2c
>
> /* SYSC register is cleared by the reset; rewrite it */
> if (dev->rev == OMAP_I2C_REV_ON_2430) {
> -
> omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> SYSC_AUTOIDLE_MASK);
> -
> } else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
> u32 v;
>
> @@ -302,6 +327,7 @@ static int omap_i2c_init(struct omap_i2c
> * WFI instruction.
> * REVISIT: Some wkup sources might not be needed.
> */
> + if (!cpu_is_omap44xx())
> omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> OMAP_I2C_WE_ALL);
>
> @@ -331,11 +357,14 @@ static int omap_i2c_init(struct omap_i2c
> psc = fclk_rate / 12000000;
> }
>
> - if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> + if (cpu_is_omap2430() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
>
> /* HSI2C controller internal clk rate should be 19.2 Mhz */
> internal_clk = 19200;
> - fclk_rate = clk_get_rate(dev->fclk) / 1000;
> + if (cpu_is_omap44xx())
> + fclk_rate = 96000;
Comment this with a 'FIXME' for when the clock framework is in place.
> + else
> + fclk_rate = clk_get_rate(dev->fclk) / 1000;
>
> /* Compute prescaler divisor */
> psc = fclk_rate / internal_clk;
> @@ -650,14 +679,12 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev_warn(dev->dev, "Too much work in one IRQ\n");
> break;
> }
> -
> omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat);
> -
Again, whitespace changes should be separated out.
> err = 0;
> if (stat & OMAP_I2C_STAT_NACK) {
> err |= OMAP_I2C_STAT_NACK;
> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> - OMAP_I2C_CON_STP);
> + OMAP_I2C_CON_STP);
Ditto, although this change should just be dropped.
> }
> if (stat & OMAP_I2C_STAT_AL) {
> dev_err(dev->dev, "Arbitration lost\n");
> @@ -683,7 +710,8 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev->buf_len--;
> /* Data reg from 2430 is 8 bit wide */
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx()
> + && !cpu_is_omap44xx()) {
Again, pay attention to whitespace and alignment.
> if (dev->buf_len) {
> *dev->buf++ = w >> 8;
> dev->buf_len--;
> @@ -710,9 +738,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> if (dev->fifo_size) {
> if (stat & OMAP_I2C_STAT_XRDY)
> num_bytes = dev->fifo_size;
> - else
> + else {
> num_bytes = omap_i2c_read_reg(dev,
> OMAP_I2C_BUFSTAT_REG);
> + }
> }
> while (num_bytes) {
> num_bytes--;
> @@ -722,7 +751,8 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev->buf_len--;
> /* Data reg from 2430 is 8 bit wide */
> if (!cpu_is_omap2430() &&
> - !cpu_is_omap34xx()) {
> + !cpu_is_omap34xx() &&
> + !cpu_is_omap44xx()) {
> if (dev->buf_len) {
> w |= *dev->buf++ << 8;
> dev->buf_len--;
> @@ -733,10 +763,10 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev_err(dev->dev,
> "XRDY IRQ while no "
> "data to send\n");
> - if (stat & OMAP_I2C_STAT_XDR)
> - dev_err(dev->dev,
> - "XDR IRQ while no "
> - "data to send\n");
> + if (stat & OMAP_I2C_STAT_XDR)
> + dev_err(dev->dev,
> + "XDR IRQ while no "
> + "data to send\n");
Again, a whitespace only change.
> break;
> }
> omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> @@ -754,7 +784,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
> dev->cmd_err |= OMAP_I2C_STAT_XUDF;
> }
> }
> -
again
> return count ? IRQ_HANDLED : IRQ_NONE;
> }
>
> @@ -822,7 +851,7 @@ omap_i2c_probe(struct platform_device *p
>
> dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
>
> - if (cpu_is_omap2430() || cpu_is_omap34xx()) {
> + if (cpu_is_omap2430() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
> u16 s;
>
> /* Set up the fifo size - Get total size */
> @@ -834,8 +863,13 @@ omap_i2c_probe(struct platform_device *p
> * size. This is to ensure that we can handle the status on int
> * call back latencies.
> */
> - dev->fifo_size = (dev->fifo_size / 2);
> - dev->b_hw = 1; /* Enable hardware fixes */
> + if (cpu_is_omap44xx()) {
> + dev->fifo_size = 0;
> + dev->b_hw = 0; /* Enable hardware fixes */
> + } else {
> + dev->fifo_size = (dev->fifo_size / 2);
> + dev->b_hw = 1; /* Enable hardware fixes */
> + }
The omap4 code added here clearly does not match the comment above. Please
update comments or add omap4-specific comments as to what you are doing here.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
