i2c: vt8500: Add support for Wondermedia I2C master-mode
Subsystems (2)
Hi Wolfram,
This patch has been floating around since 27 december with no replies so here is a pull-request for it.
Regards
Tony Prisk
The following changes since commit 9931faca02c604c22335f5a935a501bb2ace6e20:
Linux 3.8-rc3 (2013-01-09 18:59:55 -0800)
are available in the git repository at:
git://server.prisktech.co.nz/git/linuxwmt.git tags/i2c-3.9
for you to fetch changes up to c24a4dd6d6ad85e5d5cdb4e20c623103360ea7e0:
i2c: vt8500: Add support for Wondermedia I2C master-mode (2013-01-16 18:45:12 +1300)
----------------------------------------------------------------
i2c: vt8500: Add support for I2C on arch-vt8500
----------------------------------------------------------------
Tony Prisk (1):
i2c: vt8500: Add support for Wondermedia I2C master-mode
.../devicetree/bindings/i2c/i2c-vt8500.txt | 24 + drivers/i2c/busses/Kconfig | 7 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-wmt.c | 636 ++++++++++++++++++++ 4 files changed, 668 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-vt8500.txt create mode 100644 drivers/i2c/busses/i2c-wmt.c
A message that is not in the archive (sent off-list or not yet received): 1358315546-2201-1-git-send-email-linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org
PATCH 1/1 i2c: vt8500: Add support for Wondermedia I2C master-mode
This patch adds support for the I2C controller found on Wondermedia SoCs.
Due to the lack of pinmux support, GPIO pin alternate functions are configured by machine's compatible property, as are pullups.
---
cc: [email protected]
.../devicetree/bindings/i2c/i2c-vt8500.txt | 24 + drivers/i2c/busses/Kconfig | 7 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-wmt.c | 636 ++++++++++++++++++++ 4 files changed, 668 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-vt8500.txt create mode 100644 drivers/i2c/busses/i2c-wmt.c
Documentation/devicetree/bindings/i2c/i2c-vt8500.txt new file +24 -0
@@ -0,0 +1,24 @@ +* Wondermedia I2C Controller + +Required properties : + + - compatible : should be "wm,wm8505-i2c" + - reg : Offset and length of the register set for the device + - interrupts : <IRQ> where IRQ is the interrupt number + - clocks : phandle to the I2C clock source + +Optional properties : + + - clock-frequency : desired I2C bus clock frequency in Hz. + Valid values are 100000 and 400000. + Default to 100000 if not specified, or invalid value. + +Example : + + i2c_0: i2c@d8280000 { + compatible = "wm,wm8505-i2c"; + reg = <0xd8280000 0x1000>; + interrupts = <19>; + clocks = <&clki2c0>; + clock-frequency = <400000>; + };
drivers/i2c/busses/Kconfig +7 -0
@@ -700,6 +700,13 @@ config I2C_VERSATILE This driver can also be built as a module. If so, the module will be called i2c-versatile. +config I2C_WMT + tristate "Wondermedia I2C Controller support" + depends on ARCH_VT8500 + help + If you say yes to this option, support will be included for the + I2C controllers found in Wondermedia SoCs. + config I2C_OCTEON tristate "Cavium OCTEON I2C bus support" depends on CPU_CAVIUM_OCTEON
drivers/i2c/busses/Makefile +1 -0
@@ -69,6 +69,7 @@ obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o obj-$(CONFIG_I2C_STU300) += i2c-stu300.o obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o obj-$(CONFIG_I2C_VERSATILE) += i2c-versatile.o +obj-$(CONFIG_I2C_WMT) += i2c-wmt.o obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
drivers/i2c/busses/i2c-wmt.c new file +636 -0
636 lines not on this page · raw message
Signature · 1 line
1.7.9.5
A message that is not in the archive (sent off-list or not yet received): 1358315546-2201-2-git-send-email-linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org
Ho Tony,
here is my review.
Quoted 5 lines · Tony Prisk
This patch adds support for the I2C controller found on Wondermedia SoCs.
Due to the lack of pinmux support, GPIO pin alternate functions are configured by machine's compatible property, as are pullups.
Is there pinmux support planned/on its way? Maybe it is worth waiting for it? If not, I'd think the setup should be done in mach-code, not the driver.
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c new file +2
-0@@ -0,0 +1,636 @@ +/* +* Wondermedia I2C Master Mode Driver
drivers/i2c/busses/i2c-wmt.c line 12
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+* +* Copyright (C) 2012 Tony Prisk <[email protected]> +* +* Derived from GPL licensed source: +* - Copyright (C) 2008 WonderMedia Technologies, Inc. +* +* 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 +*/
Are the sources you derived from also V2 only? Otherwise you might be changing other people's license decision.
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c new file +8
-0+static int wmt_i2c_wait_bus_not_busy (struct wmt_i2c_dev *i2c_dev) +{ +u16 val; +int i; +int ret =0 ; + +for (i =0 ; i <10000000 ; i++) { + val =readw (i2c_dev->base +REG_CSR );
drivers/i2c/busses/i2c-wmt.c line 105
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+if (val &CSR_READY_MASK ) +break ; + +udelay (1 ); + } +if (i >=9999999 ) + ret = -EBUSY ; + +return ret; +}
That's 10 seconds (max) of busy looping? Nah, please don't hog the CPU for that.
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ +static int wmt_check_status (struct wmt_i2c_dev *i2c_dev) +{ +int ret =0 ; + +if (i2c_dev->cmd_status &ISR_NACK_ADDR ) + ret = -EIO ; + +if (i2c_dev->cmd_status &ISR_SCL_TIMEOUT ) + ret = -ETIMEDOUT ;
drivers/i2c/busses/i2c-wmt.c line 125
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ +return ret; +} + +static int wmt_i2c_write (struct i2c_adapter *adap,struct i2c_msg *pmsg, +int restart,int last) +{ +struct wmt_i2c_dev *i2c_dev =i2c_get_adapdata (adap); +u16 val; +u16 tcr_val;
I'd prefer to aggregate the last two lines (here and in general), but no strong opinion on that.
drivers/i2c/busses/i2c-wmt.c line 131
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+int ret; +int wait_result; +u32 xfer_len =0 ; + +if (pmsg->len <0 ) +return -EINVAL ;
len is a u16.
drivers/i2c/busses/i2c-wmt.c line 58
Quoted 2 lines
drivers/i2c/busses/i2c-wmt.c new file +2
-0+ +if (restart ==0 ) {
I think '!restart' is a tad more readable, but again, no strong opinion.
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c new file +8
-0+ ret =wmt_i2c_wait_bus_not_busy (i2c_dev); +if (ret <0 ) +return ret; + } + +if (pmsg->len ==0 ) +writew (0 , i2c_dev->base +REG_CDR ); +else
drivers/i2c/busses/i2c-wmt.c line 151
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+writew (pmsg->buf[0 ] &0xFF , i2c_dev->base +REG_CDR ); + +if (restart ==0 ) { + val =readw (i2c_dev->base +REG_CR ); + val &= ~CR_TX_END ; +writew (val, i2c_dev->base +REG_CR ); + + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR );
Just to make sure: Not possible to do both in one write?
Quoted 12 lines
drivers/i2c/busses/i2c-wmt.c new file +12
-0+ } + +init_completion (&i2c_dev->complete); + +if (i2c_dev->mode ==I2C_MODE_STANDARD ) + tcr_val =TCR_STANDARD_MODE ; +else + tcr_val =TCR_FAST_MODE ; + + tcr_val |= (TCR_MASTER_WRITE | (pmsg->addr &TCR_SLAVE_ADDR_MASK )); + +writew (tcr_val, i2c_dev->base +REG_TCR );
drivers/i2c/busses/i2c-wmt.c line 173
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ +if (restart ==1 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + + ret =0 ; + +for (;;) {
Have you tried 'while (xfer_len < pmsg->len) ...' and directly 'return err' on errors? Might have potential to be more concise.
drivers/i2c/busses/i2c-wmt.c line 100
Quoted 2 lines
drivers/i2c/busses/i2c-wmt.c new file +2
-0+ wait_result =wait_for_completion_interruptible_timeout ( + &i2c_dev->complete,500 *HZ /1000 );
Please don't use interruptible here. Most drivers had lots of problems with it and it seems you are not checking the signals anyhow.
Quoted 36 lines
drivers/i2c/busses/i2c-wmt.c new file +36
-0+ +if (wait_result ==0 ) { +dev_dbg (i2c_dev->dev,"wait timeout (tx)\n" ); + ret = -ETIMEDOUT ; +break ; + } + + ret =wmt_check_status (i2c_dev); +if (ret) +break ; + + xfer_len++; + + val =readw (i2c_dev->base +REG_CSR ); +if ((val &CSR_RCV_ACK_MASK ) ==CSR_RCV_NOT_ACK ) { +dev_dbg (i2c_dev->dev,"write RCV NACK error\n" ); + ret = -EIO ; +break ; + } + +if (pmsg->len ==0 ) { + val =CR_TX_END |CR_CPU_RDY |CR_ENABLE ; +writew (val, i2c_dev->base +REG_CR ); +break ; + } + +if (pmsg->len > xfer_len) { +writew (pmsg->buf[xfer_len] &0xFF , i2c_dev->base + +REG_CDR ); +writew (CR_CPU_RDY |CR_ENABLE , i2c_dev->base +REG_CR ); + }else if (pmsg->len == xfer_len) { +if (last !=1 ) +writew (CR_ENABLE , i2c_dev->base +REG_CR ); +break ; + }else { +dev_dbg (i2c_dev->dev,"unknown error (tx)\n" );
drivers/i2c/busses/i2c-wmt.c line 221
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ ret = -EIO ; +break ; + } + } + +return ret; +} + +static int wmt_i2c_read (struct i2c_adapter *adap,struct i2c_msg *pmsg, +int restart,int last)
Same comment as for the write function.
Quoted 49 lines
drivers/i2c/busses/i2c-wmt.c new file +49
-0+{ +struct wmt_i2c_dev *i2c_dev =i2c_get_adapdata (adap); +u16 val; +u16 tcr_val; +int ret; +int wait_result; +u32 xfer_len =0 ; + +if (pmsg->len <=0 ) +return -EINVAL ; + +if (restart ==0 ) { + ret =wmt_i2c_wait_bus_not_busy (i2c_dev); +if (ret <0 ) +return ret; + } + + val =readw (i2c_dev->base +REG_CR ); + val &= ~CR_TX_END ; +writew (val, i2c_dev->base +REG_CR ); + + val =readw (i2c_dev->base +REG_CR ); + val &= ~CR_TX_NEXT_NO_ACK ; +writew (val, i2c_dev->base +REG_CR ); + +if (restart ==0 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + +if (pmsg->len ==1 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_TX_NEXT_NO_ACK ; +writew (val, i2c_dev->base +REG_CR ); + } + +init_completion (&i2c_dev->complete); + +if (i2c_dev->mode ==I2C_MODE_STANDARD ) + tcr_val =TCR_STANDARD_MODE ; +else + tcr_val =TCR_FAST_MODE ; + + tcr_val |=TCR_MASTER_READ | (pmsg->addr &TCR_SLAVE_ADDR_MASK ); + +writew (tcr_val, i2c_dev->base +REG_TCR ); + +if (restart ==1 ) {
drivers/i2c/busses/i2c-wmt.c line 280
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + + ret =0 ; + +for (;;) { + wait_result =wait_for_completion_interruptible_timeout ( + &i2c_dev->complete,500 *HZ /1000 );
no interruptible
Quoted 53 lines
drivers/i2c/busses/i2c-wmt.c new file +53
-0+ +if (wait_result ==0 ) { +dev_dbg (i2c_dev->dev,"wait timeout (tx)\n" ); + ret = -ETIMEDOUT ; +break ; + } + + ret =wmt_check_status (i2c_dev); +if (ret) +break ; + + pmsg->buf[xfer_len] =readw (i2c_dev->base +REG_CDR ) >>8 ; + xfer_len++; + +if (pmsg->len > xfer_len) { +if (pmsg->len -1 == xfer_len) { + val =readw (i2c_dev->base +REG_CR ); + val |= (CR_TX_NEXT_NO_ACK |CR_CPU_RDY ); +writew (val, i2c_dev->base +REG_CR ); + }else { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + }else if (pmsg->len == xfer_len) { +break ; + }else { + ret = -EIO ; +break ; + } + } + +return ret; +} + +static int wmt_i2c_xfer (struct i2c_adapter *adap, +struct i2c_msg msgs[], +int num) +{ +struct i2c_msg *pmsg; +int i; +int ret =0 ; +int is_last; +int restart; + +for (i =0 ; ret >=0 && i < num; i++) { + is_last = ((i +1 ) == num); + restart = (i !=0 ); + + pmsg = &msgs[i]; +if (pmsg->flags &I2C_M_NOSTART ) + restart =1 ; +if (pmsg->flags &I2C_M_RD )
drivers/i2c/busses/i2c-wmt.c line 343
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ ret =wmt_i2c_read (adap, pmsg, restart, is_last); + else + ret =wmt_i2c_write (adap, pmsg, restart, is_last); + } + +if (ret <0 ) +return ret; +else +return i; +}
return ret < 0 ? ret : i;
drivers/i2c/busses/i2c-wmt.c line 347
Quoted 4 lines
drivers/i2c/busses/i2c-wmt.c new file +4
-0+ +static u32 wmt_i2c_func (struct i2c_adapter *adap) +{ +return I2C_FUNC_I2C |I2C_FUNC_SMBUS_EMUL ;
I2C_FUNC_NOSTART is missing.
Quoted 15 lines
drivers/i2c/busses/i2c-wmt.c new file +15
-0+} + +static const struct i2c_algorithm wmt_i2c_algo = { + .master_xfer = wmt_i2c_xfer, + .functionality = wmt_i2c_func, +}; + +static irqreturn_t wmt_i2c_isr (int irq,void *data) +{ +struct wmt_i2c_dev *i2c_dev = data; + +/* save the status and write-clear it */ + i2c_dev->cmd_status =readw (i2c_dev->base +REG_ISR ); +writew (i2c_dev->cmd_status, i2c_dev->base +REG_ISR ); +
drivers/i2c/busses/i2c-wmt.c line 372
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+complete (&i2c_dev->complete); + +return IRQ_HANDLED ; +} + +/* +* Due to a lack of pinmux functionality we need to manually configure +* the GPIO pullup. Once pinmux is implemented, this function should be +* removed. Pullup's are not available on vt8500 or wm8505 so we skip. +*/
As mentioned above, move to mach-code?
...
Quoted 24 lines
drivers/i2c/busses/i2c-wmt.c new file +24
-0+static int wmt_i2c_reset_hardware (struct wmt_i2c_dev *i2c_dev) +{ +int err; + + err =clk_prepare_enable (i2c_dev->clk); +if (err) { +dev_err (i2c_dev->dev,"failed to enable clock\n" ); +return err; + } + + err =clk_set_rate (i2c_dev->clk,20000000 ); +if (err) { +dev_err (i2c_dev->dev,"failed to set clock = 20Mhz\n" ); +return err; + } + + err =wmt_i2c_setup_gpio (i2c_dev); +if (err) +return err; + +writew (0 , i2c_dev->base +REG_CR ); +writew (12 , i2c_dev->base +REG_MCR ); +writew (ISR_WRITE_ALL , i2c_dev->base +REG_ISR ); +writew (IMR_ENABLE_ALL , i2c_dev->base +REG_IMR );
drivers/i2c/busses/i2c-wmt.c line 513
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+writew (CR_ENABLE , i2c_dev->base +REG_CR ); +readw (i2c_dev->base +REG_CSR );/* read clear */ +writew (ISR_WRITE_ALL , i2c_dev->base +REG_ISR ); + +if (i2c_dev->mode ==I2C_MODE_STANDARD ) +writew (0x8064 , i2c_dev->base +REG_TR ); +else +writew (0x8019 , i2c_dev->base +REG_TR ); + +return 0 ;
Too many magic numbers in this last block.
drivers/i2c/busses/i2c-wmt.c line 521
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c new file +8
-0+} + +static int wmt_i2c_probe (struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.of_node; +struct wmt_i2c_dev *i2c_dev; +struct i2c_adapter *adap; +struct resource *res;
No special alignment, please.
drivers/i2c/busses/i2c-wmt.c line 534
Quoted 13 lines
drivers/i2c/busses/i2c-wmt.c new file +13
-0+int err; +u32 clk_rate; + +if (!np) { +dev_err (&pdev->dev,"device node not found\n" ); +return -ENODEV ; + } + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"no memory resource defined\n" ); +return -ENODEV ; + }
devm_request_and_ioremap will check res for you.
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ + i2c_dev =devm_kzalloc (&pdev->dev,sizeof (*i2c_dev),GFP_KERNEL ); +if (!i2c_dev) { +dev_err (&pdev->dev,"device memory allocation failed\n" ); +return -ENOMEM ; + } + + i2c_dev->base =devm_request_and_ioremap (&pdev->dev, res); +if (!i2c_dev->base) { +dev_err (&pdev->dev,"memory region unavailable\n" );
drivers/i2c/busses/i2c-wmt.c line 554
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+return -ENOMEM ; + } + + 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 );
There is also devm_clk_get.
Quoted 18 lines
drivers/i2c/busses/i2c-wmt.c new file +18
-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 ; + err =of_property_read_u32 (np,"clock-frequency" , &clk_rate); +if ((!err) && (clk_rate ==400000 )) + i2c_dev->mode =I2C_MODE_FAST ; + + i2c_dev->dev = &pdev->dev; + + err =devm_request_irq (&pdev->dev, i2c_dev->irq, wmt_i2c_isr,0 , +"i2c" , i2c_dev); +if (err) { +dev_err (&pdev->dev,"failed to request irq %i\n" , i2c_dev->irq); +return err; + }
drivers/i2c/busses/i2c-wmt.c line 582
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ + adap = &i2c_dev->adapter; +i2c_set_adapdata (adap, i2c_dev); +strlcpy (adap->name,"WMT I2C adapter" ,sizeof (adap->name)); + adap->owner =THIS_MODULE ; + adap->class =I2C_CLASS_HWMON ; + adap->algo = &wmt_i2c_algo; + adap->dev.parent = &pdev->dev; + adap->dev.of_node = pdev->dev.of_node; + adap->nr =of_alias_get_id (pdev->dev.of_node,"i2c" );
No alignment with tabs, please.
Quoted 19 lines
drivers/i2c/busses/i2c-wmt.c new file +19
-0+ + err =wmt_i2c_reset_hardware (i2c_dev); +if (err) { +dev_err (&pdev->dev,"error initializing hardware\n" ); +return err; + } + +if (adap->nr <0 ) + err =i2c_add_adapter (adap); +else + err =i2c_add_numbered_adapter (adap); + +if (err) { +dev_err (&pdev->dev,"failed to add adapter\n" ); +return err; + } + +platform_set_drvdata (pdev, i2c_dev); +
drivers/i2c/busses/i2c-wmt.c line 611
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+of_i2c_register_devices (adap); + +return 0 ; +} + +static int wmt_i2c_remove (struct platform_device *pdev) +{ +struct wmt_i2c_dev *i2c_dev =platform_get_drvdata (pdev); + +i2c_del_adapter (&i2c_dev->adapter);
Is it guaranteed that interrupts cannot occur anymore? Otherwise OOPS might happen, since the adapter is cleared here already but devm will remove the interrupt only a little later.
Quoted 15 lines
drivers/i2c/busses/i2c-wmt.c new file +15
-0+ +return 0 ; +} + +static struct of_device_id wmt_i2c_dt_ids[] = { + { .compatible ="wm,wm8505-i2c" }, + {/* Sentinel */ }, +}; + +static struct platform_driver wmt_i2c_driver = { + .probe = wmt_i2c_probe, + .remove = wmt_i2c_remove, + .driver = { + .name ="wmt-i2c" , + .owner =THIS_MODULE ,
drivers/i2c/busses/i2c-wmt.c line 636
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ .of_match_table = wmt_i2c_dt_ids, + }, +}; + +module_platform_driver (wmt_i2c_driver); + +MODULE_DESCRIPTION ("Wondermedia I2C master-mode bus adapter" ); +MODULE_AUTHOR ("Tony Prisk <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DEVICE_TABLE (of, wmt_i2c_dt_ids);
Also, checkpatch found a trailing whitespace somewhere.
Thanks,
Wolfram
A message that is not in the archive (sent off-list or not yet received): 20130213201307.GA8795-8EAEigeeuNG034pCzgS/[email protected]
Thanks for the review.
My replies are inline. Mostly 'ok' and 'will dos'.
To save you trawling through it all, here's the two replies/comments that are *different*:
1) I am working on pinmux at the moment - will hold off on this driver until pinmux is completed.
2)
Quoted 3 lines
drivers/i2c/busses/i2c-wmt.c new file +1
-0+ i2c_dev->clk =of_clk_get (np,0 );There is also
devm_clk_get.
devm_clk_get requires a 'clock name'.
Since vt8500 is DT-only, and clock-names is an optional property in the clocks binding I preferred not to *require* an optional property.
If I am misunderstanding this, please correct me.
Regards
Tony Prisk
Quoted 8 lines · Wolfram Sang
Ho Tony,
here is my review.
Quoted 3 lines · Tony Prisk
This patch adds support for the I2C controller found on Wondermedia SoCs.
Quoted 6 lines
Quoted 2 lines
Due to the lack of pinmux support, GPIO pin alternate functions are configured by machine's compatible property, as are pullups.
Is there pinmux support planned/on its way? Maybe it is worth waiting for it? If not, I'd think the setup should be done in mach-code, not the driver.
I am actually working on pinmux at the moment so I will remove this gpio-setup code and get the pinmux code into mainline first before resubmitting this driver.
Quoted 14 lines
Quoted 14 lines
drivers/i2c/busses/i2c-wmt.c new file +7
-0@@ -0,0 +1,636 @@ +/* +* Wondermedia I2C Master Mode Driver +* +* Copyright (C) 2012 Tony Prisk <[email protected]> +* +* Derived from GPL licensed source: +* - Copyright (C) 2008 WonderMedia Technologies, Inc.
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 12
Quoted 5 lines
drivers/i2c/busses/i2c-wmt.c new file +5
-0+* +* 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 +*/ Are the sources you derived from also V2 only? Otherwise you might be changing other people's license decision.
Oh dear.. almost.
Original source was v2 or later. I will update the license text to indicate 'later' is also applicable.
Quoted 14 lines
Quoted 14 lines
drivers/i2c/busses/i2c-wmt.c new file +13
-0+static int wmt_i2c_wait_bus_not_busy (struct wmt_i2c_dev *i2c_dev) +{ +u16 val; +int i; +int ret =0 ; + +for (i =0 ; i <10000000 ; i++) { + val =readw (i2c_dev->base +REG_CSR ); +if (val &CSR_READY_MASK ) +break ; + +udelay (1 ); + }
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 105
Quoted 5 lines
drivers/i2c/busses/i2c-wmt.c new file +5
-0+if (i >=9999999 ) + ret = -EBUSY ; + +return ret; +}That's 10 seconds (max) of busy looping? Nah, please don't hog the CPU for that.
Eek. Not sure how I missed that. Will fix.
Quoted 16 lines
Quoted 16 lines
drivers/i2c/busses/i2c-wmt.c new file +15
-0+ +static int wmt_check_status (struct wmt_i2c_dev *i2c_dev) +{ +int ret =0 ; + +if (i2c_dev->cmd_status &ISR_NACK_ADDR ) + ret = -EIO ; + +if (i2c_dev->cmd_status &ISR_SCL_TIMEOUT ) + ret = -ETIMEDOUT ; + +return ret; +} + +static int wmt_i2c_write(struct i2c_adapter *adap, struct i2c_msg *pmsg,
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 125
Quoted 5 lines
drivers/i2c/busses/i2c-wmt.c new file +5
-0+ int restart, int last) +{ +struct wmt_i2c_dev *i2c_dev =i2c_get_adapdata (adap); +u16 val; +u16 tcr_val;I'd prefer to aggregate the last two lines (here and in general), but no strong opinion on that.
My preference is for one variable per line but I'm happy to go with the majority (if there is one).
Quoted 9 lines
drivers/i2c/busses/i2c-wmt.c line 131
Quoted 7 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+int ret; +int wait_result; +u32 xfer_len =0 ; + +if (pmsg->len <0 ) +return -EINVAL ;len is a u16.
Sorry - copied verbatim from the vendor source.. I should know better by now.
Quoted 5 lines
drivers/i2c/busses/i2c-wmt.c line 58
Quoted 3 lines
drivers/i2c/busses/i2c-wmt.c new file +2
-0+ +if (restart ==0 ) {I think '!restart' is a tad more readable, but again, no strong opinion.
Agree - will change.
Quoted 13 lines
Quoted 13 lines
drivers/i2c/busses/i2c-wmt.c new file +12
-0+ ret =wmt_i2c_wait_bus_not_busy (i2c_dev); +if (ret <0 ) +return ret; + } + +if (pmsg->len ==0 ) +writew (0 , i2c_dev->base +REG_CDR ); +else +writew (pmsg->buf[0 ] &0xFF , i2c_dev->base +REG_CDR ); + +if (restart ==0 ) { + val =readw (i2c_dev->base +REG_CR );
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 151
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+ val &= ~CR_TX_END ; +writew (val, i2c_dev->base +REG_CR ); + + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR );Just to make sure: Not possible to do both in one write?
I don't know the answer to that question. The original vendor source did it with two separate read-modify-write calls, so I kept it the same.
I can test it, although at first glance, I would guess it's done this way because val |= CR_CPU_RDY will start the transfer immediately and the val &= ~CR_TX_END may not have been processed within the hardware?!?!
Quoted 18 lines
Quoted 18 lines
drivers/i2c/busses/i2c-wmt.c new file +17
-0+ } + +init_completion (&i2c_dev->complete); + +if (i2c_dev->mode ==I2C_MODE_STANDARD ) + tcr_val =TCR_STANDARD_MODE ; +else + tcr_val =TCR_FAST_MODE ; + + tcr_val |= (TCR_MASTER_WRITE | (pmsg->addr &TCR_SLAVE_ADDR_MASK )); + +writew (tcr_val, i2c_dev->base +REG_TCR ); + +if (restart ==1 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR );
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 173
Quoted 5 lines
drivers/i2c/busses/i2c-wmt.c new file +5
-0+ } + + ret =0 ; + +for (;;) {Have you tried 'while (
xfer_len<pmsg->len) ...' and directly 'return err' on errors? Might have potential to be more concise.
Can do.
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c line 100
Quoted 3 lines
drivers/i2c/busses/i2c-wmt.c new file +2
-0+ wait_result =wait_for_completion_interruptible_timeout ( + &i2c_dev->complete,500 *HZ /1000 );Please don't use interruptible here. Most drivers had lots of problems with it and it seems you are not checking the signals anyhow.
Again, taken from vendor source - will change.
Quoted 178 lines
Quoted 37 lines
drivers/i2c/busses/i2c-wmt.c new file +36
-0+ +if (wait_result ==0 ) { +dev_dbg (i2c_dev->dev,"wait timeout (tx)\n" ); + ret = -ETIMEDOUT ; +break ; + } + + ret =wmt_check_status (i2c_dev); +if (ret) +break ; + + xfer_len++; + + val =readw (i2c_dev->base +REG_CSR ); +if ((val &CSR_RCV_ACK_MASK ) ==CSR_RCV_NOT_ACK ) { +dev_dbg (i2c_dev->dev,"write RCV NACK error\n" ); + ret = -EIO ; +break ; + } + +if (pmsg->len ==0 ) { + val =CR_TX_END |CR_CPU_RDY |CR_ENABLE ; +writew (val, i2c_dev->base +REG_CR ); +break ; + } + +if (pmsg->len > xfer_len) { +writew (pmsg->buf[xfer_len] &0xFF , i2c_dev->base + +REG_CDR ); +writew (CR_CPU_RDY |CR_ENABLE , i2c_dev->base +REG_CR ); + }else if (pmsg->len == xfer_len) { +if (last !=1 ) +writew (CR_ENABLE , i2c_dev->base +REG_CR ); +break ; + }else { +dev_dbg (i2c_dev->dev,"unknown error (tx)\n" );drivers/i2c/busses/i2c-wmt.c line 221
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ ret = -EIO ; +break ; + } + } + +return ret; +} + +static int wmt_i2c_read (struct i2c_adapter *adap,struct i2c_msg *pmsg, +int restart,int last)Same comment as for the write function.
Quoted 49 lines
drivers/i2c/busses/i2c-wmt.c new file +49
-0+{ +struct wmt_i2c_dev *i2c_dev =i2c_get_adapdata (adap); +u16 val; +u16 tcr_val; +int ret; +int wait_result; +u32 xfer_len =0 ; + +if (pmsg->len <=0 ) +return -EINVAL ; + +if (restart ==0 ) { + ret =wmt_i2c_wait_bus_not_busy (i2c_dev); +if (ret <0 ) +return ret; + } + + val =readw (i2c_dev->base +REG_CR ); + val &= ~CR_TX_END ; +writew (val, i2c_dev->base +REG_CR ); + + val =readw (i2c_dev->base +REG_CR ); + val &= ~CR_TX_NEXT_NO_ACK ; +writew (val, i2c_dev->base +REG_CR ); + +if (restart ==0 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + +if (pmsg->len ==1 ) { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_TX_NEXT_NO_ACK ; +writew (val, i2c_dev->base +REG_CR ); + } + +init_completion (&i2c_dev->complete); + +if (i2c_dev->mode ==I2C_MODE_STANDARD ) + tcr_val =TCR_STANDARD_MODE ; +else + tcr_val =TCR_FAST_MODE ; + + tcr_val |=TCR_MASTER_READ | (pmsg->addr &TCR_SLAVE_ADDR_MASK ); + +writew (tcr_val, i2c_dev->base +REG_TCR ); + +if (restart ==1 ) {drivers/i2c/busses/i2c-wmt.c line 280
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + + ret =0 ; + +for (;;) { + wait_result =wait_for_completion_interruptible_timeout ( + &i2c_dev->complete,500 *HZ /1000 );no interruptible
Quoted 53 lines
drivers/i2c/busses/i2c-wmt.c new file +53
-0+ +if (wait_result ==0 ) { +dev_dbg (i2c_dev->dev,"wait timeout (tx)\n" ); + ret = -ETIMEDOUT ; +break ; + } + + ret =wmt_check_status (i2c_dev); +if (ret) +break ; + + pmsg->buf[xfer_len] =readw (i2c_dev->base +REG_CDR ) >>8 ; + xfer_len++; + +if (pmsg->len > xfer_len) { +if (pmsg->len -1 == xfer_len) { + val =readw (i2c_dev->base +REG_CR ); + val |= (CR_TX_NEXT_NO_ACK |CR_CPU_RDY ); +writew (val, i2c_dev->base +REG_CR ); + }else { + val =readw (i2c_dev->base +REG_CR ); + val |=CR_CPU_RDY ; +writew (val, i2c_dev->base +REG_CR ); + } + }else if (pmsg->len == xfer_len) { +break ; + }else { + ret = -EIO ; +break ; + } + } + +return ret; +} + +static int wmt_i2c_xfer (struct i2c_adapter *adap, +struct i2c_msg msgs[], +int num) +{ +struct i2c_msg *pmsg; +int i; +int ret =0 ; +int is_last; +int restart; + +for (i =0 ; ret >=0 && i < num; i++) { + is_last = ((i +1 ) == num); + restart = (i !=0 ); + + pmsg = &msgs[i]; +if (pmsg->flags &I2C_M_NOSTART ) + restart =1 ; +if (pmsg->flags &I2C_M_RD )drivers/i2c/busses/i2c-wmt.c line 343
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ ret =wmt_i2c_read (adap, pmsg, restart, is_last); + else + ret =wmt_i2c_write (adap, pmsg, restart, is_last); + } + +if (ret <0 ) +return ret; +else +return i; +}return ret < 0 ? ret : i;
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c line 347
Quoted 4 lines
drivers/i2c/busses/i2c-wmt.c new file +4
-0+ +static u32 wmt_i2c_func (struct i2c_adapter *adap) +{ +return I2C_FUNC_I2C |I2C_FUNC_SMBUS_EMUL ;
I2C_FUNC_NOSTARTis missing.
Again - taken from vendor source as is. Will read the book (to find out what it does) and add.
Quoted 20 lines
Quoted 20 lines
drivers/i2c/busses/i2c-wmt.c new file +19
-0+} + +static const struct i2c_algorithm wmt_i2c_algo = { + .master_xfer = wmt_i2c_xfer, + .functionality = wmt_i2c_func, +}; + +static irqreturn_t wmt_i2c_isr (int irq,void *data) +{ +struct wmt_i2c_dev *i2c_dev = data; + +/* save the status and write-clear it */ + i2c_dev->cmd_status =readw (i2c_dev->base +REG_ISR ); +writew (i2c_dev->cmd_status, i2c_dev->base +REG_ISR ); + +complete (&i2c_dev->complete); + +return IRQ_HANDLED ; +}
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 372
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+ +/* +* Due to a lack of pinmux functionality we need to manually configure +* the GPIO pullup. Once pinmux is implemented, this function should be +* removed. Pullup's are not available on vt8500 or wm8505 so we skip. +*/ As mentioned above, move to mach-code?
Will drop before v2.
Quoted 31 lines
...
Quoted 28 lines
drivers/i2c/busses/i2c-wmt.c new file +28
-0+static int wmt_i2c_reset_hardware (struct wmt_i2c_dev *i2c_dev) +{ +int err; + + err =clk_prepare_enable (i2c_dev->clk); +if (err) { +dev_err (i2c_dev->dev,"failed to enable clock\n" ); +return err; + } + + err =clk_set_rate (i2c_dev->clk,20000000 ); +if (err) { +dev_err (i2c_dev->dev,"failed to set clock = 20Mhz\n" ); +return err; + } + + err =wmt_i2c_setup_gpio (i2c_dev); +if (err) +return err; + +writew (0 , i2c_dev->base +REG_CR ); +writew (12 , i2c_dev->base +REG_MCR ); +writew (ISR_WRITE_ALL , i2c_dev->base +REG_ISR ); +writew (IMR_ENABLE_ALL , i2c_dev->base +REG_IMR ); +writew (CR_ENABLE , i2c_dev->base +REG_CR ); +readw (i2c_dev->base +REG_CSR );/* read clear */ +writew (ISR_WRITE_ALL , i2c_dev->base +REG_ISR ); +
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 513
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+if (i2c_dev->mode ==I2C_MODE_STANDARD ) +writew (0x8064 , i2c_dev->base +REG_TR ); +else +writew (0x8019 , i2c_dev->base +REG_TR ); + +return 0 ;Too many magic numbers in this last block.
When I wrote this piece of code I had no idea what these magic numbers meant. I've since figured it out and will redo.
Quoted 11 lines
drivers/i2c/busses/i2c-wmt.c line 521
Quoted 9 lines
drivers/i2c/busses/i2c-wmt.c new file +8
-0+} + +static int wmt_i2c_probe (struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.of_node; +struct wmt_i2c_dev *i2c_dev; +struct i2c_adapter *adap; +struct resource *res;No special alignment, please.
Ok.
Quoted 16 lines
Quoted 4 lines
drivers/i2c/busses/i2c-wmt.c new file +3
-0+int err; +u32 clk_rate; +drivers/i2c/busses/i2c-wmt.c line 534
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+if (!np) { +dev_err (&pdev->dev,"device node not found\n" ); +return -ENODEV ; + } + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) { +dev_err (&pdev->dev,"no memory resource defined\n" ); +return -ENODEV ; + }
devm_request_and_ioremapwill check res for you.
Ok. Will drop the error check.
Quoted 15 lines
Quoted 15 lines
drivers/i2c/busses/i2c-wmt.c new file +14
-0+ + i2c_dev =devm_kzalloc (&pdev->dev,sizeof (*i2c_dev),GFP_KERNEL ); +if (!i2c_dev) { +dev_err (&pdev->dev,"device memory allocation failed\n" ); +return -ENOMEM ; + } + + i2c_dev->base =devm_request_and_ioremap (&pdev->dev, res); +if (!i2c_dev->base) { +dev_err (&pdev->dev,"memory region unavailable\n" ); +return -ENOMEM ; + } + + i2c_dev->irq =irq_of_parse_and_map (np,0 );
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 554
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+if (!i2c_dev->irq) { +dev_err (&pdev->dev,"irq missing or invalid\n" ); +return -EINVAL ; + } + + i2c_dev->clk =of_clk_get (np,0 );There is also
devm_clk_get.
devm_clk_get requires a 'clock name'.
Since vt8500 is DT-only, and clock-names is an optional property in the clocks binding I preferred not to *require* an optional property.
If I am misunderstanding this, please correct me.
Quoted 23 lines
Quoted 23 lines
drivers/i2c/busses/i2c-wmt.c new file +22
-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 ; + err =of_property_read_u32 (np,"clock-frequency" , &clk_rate); +if ((!err) && (clk_rate ==400000 )) + i2c_dev->mode =I2C_MODE_FAST ; + + i2c_dev->dev = &pdev->dev; + + err =devm_request_irq (&pdev->dev, i2c_dev->irq, wmt_i2c_isr,0 , +"i2c" , i2c_dev); +if (err) { +dev_err (&pdev->dev,"failed to request irq %i\n" , i2c_dev->irq); +return err; + } + + adap = &i2c_dev->adapter; +i2c_set_adapdata (adap, i2c_dev); +strlcpy (adap->name,"WMT I2C adapter" ,sizeof (adap->name));
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 582
Quoted 6 lines
drivers/i2c/busses/i2c-wmt.c new file +6
-0+ adap->owner =THIS_MODULE ; + adap->class =I2C_CLASS_HWMON ; + adap->algo = &wmt_i2c_algo; + adap->dev.parent = &pdev->dev; + adap->dev.of_node = pdev->dev.of_node; + adap->nr =of_alias_get_id (pdev->dev.of_node,"i2c" );No alignment with tabs, please.
Ok.
Quoted 26 lines
Quoted 26 lines
drivers/i2c/busses/i2c-wmt.c new file +25
-0+ + err =wmt_i2c_reset_hardware (i2c_dev); +if (err) { +dev_err (&pdev->dev,"error initializing hardware\n" ); +return err; + } + +if (adap->nr <0 ) + err =i2c_add_adapter (adap); +else + err =i2c_add_numbered_adapter (adap); + +if (err) { +dev_err (&pdev->dev,"failed to add adapter\n" ); +return err; + } + +platform_set_drvdata (pdev, i2c_dev); + +of_i2c_register_devices (adap); + +return 0 ; +} + +static int wmt_i2c_remove (struct platform_device *pdev)
Quoted 8 lines
drivers/i2c/busses/i2c-wmt.c line 611
Quoted 4 lines
drivers/i2c/busses/i2c-wmt.c new file +4
-0+{ +struct wmt_i2c_dev *i2c_dev =platform_get_drvdata (pdev); + +i2c_del_adapter (&i2c_dev->adapter);Is it guaranteed that interrupts cannot occur anymore? Otherwise OOPS might happen, since the adapter is cleared here already but devm will remove the interrupt only a little later.
Hmm.. good point. I will add some hw-stop code in here as well.
Quoted 32 lines
Quoted 16 lines
drivers/i2c/busses/i2c-wmt.c new file +15
-0+ +return 0 ; +} + +static struct of_device_id wmt_i2c_dt_ids[] = { + { .compatible ="wm,wm8505-i2c" }, + {/* Sentinel */ }, +}; + +static struct platform_driver wmt_i2c_driver = { + .probe = wmt_i2c_probe, + .remove = wmt_i2c_remove, + .driver = { + .name ="wmt-i2c" , + .owner =THIS_MODULE ,drivers/i2c/busses/i2c-wmt.c line 636
Quoted 10 lines
drivers/i2c/busses/i2c-wmt.c new file +10
-0+ .of_match_table = wmt_i2c_dt_ids, + }, +}; + +module_platform_driver (wmt_i2c_driver); + +MODULE_DESCRIPTION ("Wondermedia I2C master-mode bus adapter" ); +MODULE_AUTHOR ("Tony Prisk <[email protected]>" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_DEVICE_TABLE (of, wmt_i2c_dt_ids);Also, checkpatch found a trailing whitespace somewhere.
Thanks,
Wolfram
Hi Tony,
Quoted 2 lines
1) I am working on pinmux at the moment - will hold off on this driver until pinmux is completed.
Cool, great work!
Quoted 7 lines
2)
Quoted 3 lines
drivers/i2c/busses/i2c-wmt.c new file +1
-0+ i2c_dev->clk =of_clk_get (np,0 );There is also
devm_clk_get.
devm_clk_getrequires a 'clock name'.
Since vt8500 is DT-only, and clock-names is an optional property in the clocks binding I preferred not to *require* an optional property.
OK then. Just wanted to mention it.
Thanks,
Wolfram
Quoted 3 lines
This patch has been floating around since 27 december with no replies so here is a pull-request for it.
I'll review it tomorrow.