[OpenWrt-Devel] [PATCH 2/2] ralink: MT7621 add i2c controller driver on linux kernel 3.18
John Crispin
blogic at openwrt.org
Sat Mar 21 16:54:22 EDT 2015
Hi,
please see comments inline. also please the checkpatch.pl script on this
patch before sending a V2
John
On 21/03/2015 06:06, wengbj wrote:
> ralink i2c driver is not working on MT7621 platform. Porting a new drivers from MTK's source code.
>
> Signed-off-by: wengbj <fl.service at t-firefly.com>
> ---
> .../0111-i2c-MIPS-add-mt7621-I2C-driver.patch | 390 ++++++++++++++++++++
> 1 file changed, 390 insertions(+)
> create mode 100755 target/linux/ramips/patches-3.18/0111-i2c-MIPS-add-mt7621-I2C-driver.patch
>
> diff --git a/target/linux/ramips/patches-3.18/0111-i2c-MIPS-add-mt7621-I2C-driver.patch b/target/linux/ramips/patches-3.18/0111-i2c-MIPS-add-mt7621-I2C-driver.patch
> new file mode 100755
> index 0000000..ec897ee
> --- /dev/null
> +++ b/target/linux/ramips/patches-3.18/0111-i2c-MIPS-add-mt7621-I2C-driver.patch
> @@ -0,0 +1,390 @@
> +Index: linux-3.18.9/drivers/i2c/busses/Kconfig
> +===================================================================
> +--- linux-3.18.9.orig/drivers/i2c/busses/Kconfig 2015-03-21 11:47:27.387652879 +0800
> ++++ linux-3.18.9/drivers/i2c/busses/Kconfig 2015-03-21 11:47:27.587652870 +0800
> +@@ -714,6 +714,10 @@
> + tristate "Ralink I2C Controller"
> + select OF_I2C
> +
> ++config I2C_MT7621
> ++ tristate "Mt7621 I2C Controller"
> ++ select OF_I2C
> ++
> + config HAVE_S3C2410_I2C
> + bool
> + help
> +Index: linux-3.18.9/drivers/i2c/busses/Makefile
> +===================================================================
> +--- linux-3.18.9.orig/drivers/i2c/busses/Makefile 2015-03-21 11:47:27.387652879 +0800
> ++++ linux-3.18.9/drivers/i2c/busses/Makefile 2015-03-21 11:47:27.587652870 +0800
> +@@ -67,6 +67,7 @@
> + obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> + obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
> + obj-$(CONFIG_I2C_RALINK) += i2c-ralink.o
> ++obj-$(CONFIG_I2C_MT7621) += i2c-mt7621.o
> + obj-$(CONFIG_I2C_QUP) += i2c-qup.o
> + obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
> + obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
> +Index: linux-3.18.9/drivers/i2c/busses/i2c-mt7621.c
> +===================================================================
> +--- /dev/null 1970-01-01 00:00:00.000000000 +0000
> ++++ linux-3.18.9/drivers/i2c/busses/i2c-mt7621.c 2015-03-21 11:48:58.883648615 +0800
> +@@ -0,0 +1,358 @@
> ++/*
> ++ * drivers/i2c/busses/i2c-mt7621.c
> ++ *
> ++ * Copyright (C) 2013 Steven Liu <steven_liu at mediatek.com>
> ++ *
> ++ * Improve driver for i2cdetect from i2c-tools to detect i2c devices on the bus.
> ++ * (C) 2014 Sittisak <sittisaks at hotmail.com>
> ++ *
> ++ * This software is licensed under the terms of the GNU General Public
> ++ * License version 2, as published by the Free Software Foundation, and
> ++ * may be copied, distributed, and modified under those terms.
> ++ *
> ++ * This program is distributed in the hope that it will be useful,
> ++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> ++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> ++ * GNU General Public License for more details.
> ++ *
> ++ */
> ++
> ++#include <linux/interrupt.h>
> ++#include <linux/kernel.h>
> ++#include <linux/module.h>
> ++#include <linux/reset.h>
> ++#include <linux/delay.h>
> ++#include <linux/slab.h>
> ++#include <linux/init.h>
> ++#include <linux/errno.h>
> ++#include <linux/platform_device.h>
> ++#include <linux/i2c.h>
> ++#include <linux/io.h>
> ++#include <linux/err.h>
> ++
> ++#include <asm/mach-ralink/ralink_regs.h>
> ++
> ++#define REG_CONFIG_REG 0x00
> ++#define REG_CLKDIV_REG 0x04
> ++#define REG_DEVADDR_REG 0x08
> ++#define REG_ADDR_REG 0x0C
> ++#define REG_DATAOUT_REG 0x10
> ++#define REG_DATAIN_REG 0x14
> ++#define REG_STATUS_REG 0x18
> ++#define REG_STARTXFR_REG 0x1C
> ++#define REG_BYTECNT_REG 0x20
> ++#define REG_SM0_IS_AUTOMODE 0x28
> ++#define REG_SM0CTL0 0x40
> ++
> ++
> ++#define I2C_STARTERR 0x10
> ++#define I2C_ACKERR 0x08
> ++#define I2C_DATARDY 0x04
> ++#define I2C_SDOEMPTY 0x02
> ++#define I2C_BUSY 0x01
> ++
> ++/* I2C_CFG register bit field */
> ++#define I2C_CFG_ADDRLEN_8 (7<<5) /* 8 bits */
> ++#define I2C_CFG_DEVADLEN_7 (6<<2)
> ++#define I2C_CFG_ADDRDIS (1<<1)
> ++#define I2C_CFG_DEVADDIS (1<<0)
please use the BIT() macro instead of x<<y syntax
> ++
> ++#define I2C_CFG_DEFAULT (I2C_CFG_ADDRLEN_8 | \
> ++ I2C_CFG_DEVADLEN_7 | \
> ++ I2C_CFG_ADDRDIS)
> ++
> ++#define I2C_RETRY 0x1000
> ++
> ++#define CLKDIV_VALUE 333
> ++#define i2c_busy_loop (CLKDIV_VALUE*30)
> ++
> ++#define READ_CMD 0x01
> ++#define WRITE_CMD 0x00
> ++#define READ_BLOCK 16
> ++
> ++#define I2C_OFFSET 0x900
> ++#define RSTCTRL_OFFSET 0x34
> ++
> ++#define MT7621_REG(x) (*((volatile u32 *)(x)))
> ++
please use the ioread32() api
> ++struct i2c_algo_mt7621_data{
> ++ u32 ioaddr;
> ++ wait_queue_head_t waitq;
> ++ spinlock_t lock;
> ++ int id;
> ++};
leading spaces instead of tabs. ideally run script/checkpatch.pl on the
patch and fix all warning it gives you
> ++/***********************************************************/
> ++
> ++static unsigned long clkdiv_value = CLKDIV_VALUE;
> ++
> ++static void __iomem *memsysctlbase;
> ++static void __iomem *membase;
> ++static struct i2c_adapter *adapter;
> ++static int i2c_id;
> ++
> ++static void rt_i2c_w32(u32 val, unsigned reg)
> ++{
> ++ iowrite32(val, membase + reg);
> ++}
> ++
> ++static u32 rt_i2c_r32(unsigned reg)
> ++{
> ++ return ioread32(membase + reg);
> ++}
> ++
> ++static void mt7621_i2c_reset(void)
> ++{
> ++ u32 val;
> ++ val = MT7621_REG(RSTCTRL_OFFSET+memsysctlbase);
> ++ val = val | (1<<16);
> ++ MT7621_REG(RSTCTRL_OFFSET+memsysctlbase) = val;
> ++ val = val & ~(1<<16);
> ++ MT7621_REG(RSTCTRL_OFFSET+memsysctlbase) = val;
> ++ udelay(500);
please use the reset-control api here ( device_reset(&pdev->dev); )
> ++}
> ++static void mt7621_i2c_enable(struct i2c_msg *msg)
> ++{
> ++ rt_i2c_w32(msg->addr,REG_DEVADDR_REG);
> ++ rt_i2c_w32(0,REG_ADDR_REG);
> ++}
> ++
> ++static void i2c_master_init(void)
> ++{
> ++ u32 value;
> ++ /*set mt7621 i2c mode*/
> ++ (*((volatile u32*)(memsysctlbase+0x60))) &=~0x4;
> ++
this is the GPIOMODE register. please remove it and make sure pinctrl is
setup properlz
> ++ mt7621_i2c_reset();
> ++ rt_i2c_w32(I2C_CFG_DEFAULT,REG_CONFIG_REG);
> ++
> ++ value = 1<< 31;
> ++ value |= 1<<28;
> ++ value |= clkdiv_value <<16;
> ++ value |= 1<<6;
> ++ value |= 1<<1;
use BIT() please and also use #define to give these magic values a name
> ++ rt_i2c_w32(value,REG_SM0CTL0);
> ++ rt_i2c_w32(1,REG_SM0_IS_AUTOMODE);//auto mode
comments should be above the line of code and not behind, alos /* ... */
is the prefered syntax. i also think this comment is not needed
> ++}
> ++
> ++
> ++static inline int rt_i2c_wait_rx_done(void)
> ++{
> ++ int i=0;
> ++ while((!(rt_i2c_r32(REG_STATUS_REG) & I2C_DATARDY)) && (i<i2c_busy_loop))
> ++ i++;
> ++ if(i>=i2c_busy_loop){
> ++ pr_err("err,wait for idle timeout");
> ++ return -ETIMEDOUT;
> ++ }
> ++ return 0;
> ++}
> ++
> ++static inline int rt_i2c_wait_idle(void)
> ++{
> ++ int i=0;
> ++ while((rt_i2c_r32(REG_STATUS_REG) & I2C_BUSY) && (i<i2c_busy_loop))
> ++ i++;
> ++ if(i>=i2c_busy_loop){
> ++ pr_err("err,wait for idle timeout");
> ++ return -ETIMEDOUT;
> ++ }
> ++ return 0;
> ++}
> ++
> ++static inline int rt_i2c_wait_tx_done(void)
> ++{
> ++ int i=0;
> ++ while((!(rt_i2c_r32(REG_STATUS_REG) & I2C_SDOEMPTY)) && (i<i2c_busy_loop))
> ++ i++;
> ++ if(i>=i2c_busy_loop){
> ++ pr_err("err,wait for idle timeout");
> ++ return -ETIMEDOUT;
> ++ }
> ++ return 0;
> ++}
> ++
> ++static int rt_i2c_handle_msg(struct i2c_adapter *a, struct i2c_msg* msg)
> ++{
> ++ int i = 0, j = 0, pos = 0;
> ++ int nblock = msg->len / READ_BLOCK;
> ++ int rem = msg->len % READ_BLOCK;
> ++
> ++ if (msg->flags & I2C_M_TEN) {
> ++ printk("10 bits addr not supported\n");
> ++ return -EINVAL;
> ++ }
> ++
> ++ if (msg->flags & I2C_M_RD) {
> ++ for (i = 0; i < nblock; i++) {
> ++ if (rt_i2c_wait_idle())
> ++ goto ERR_TIMEOUT;
> ++ rt_i2c_w32(READ_BLOCK - 1, REG_BYTECNT_REG);
> ++ rt_i2c_w32(READ_CMD, REG_STARTXFR_REG);
> ++ for (j = 0; j < READ_BLOCK; j++) {
> ++ if (rt_i2c_wait_rx_done())
> ++ goto ERR_TIMEOUT;
> ++ msg->buf[pos++] = rt_i2c_r32(REG_DATAIN_REG);
> ++ }
> ++ }
> ++
> ++ if (rt_i2c_wait_idle()) {
> ++ goto ERR_TIMEOUT;
> ++ }
checkpatch.pl will complain here aswell about the {} not being needed if
only 1 line follows the if clause.
> ++
> ++ rt_i2c_w32(rem - 1, REG_BYTECNT_REG);
> ++ rt_i2c_w32(READ_CMD, REG_STARTXFR_REG);
> ++
> ++ for (i = 0; i < rem; i++) {
> ++ if (rt_i2c_wait_rx_done())
> ++ goto ERR_TIMEOUT;
> ++ msg->buf[pos++] = rt_i2c_r32(REG_DATAIN_REG);
^ indented 1 tab too many
> ++ }
> ++ } else {
> ++ if (rt_i2c_wait_idle()) {
> ++ goto ERR_TIMEOUT;
> ++ }
{} not needed here
> ++ rt_i2c_w32(msg->len - 1, REG_BYTECNT_REG);
> ++ for (i = 0; i < msg->len; i++) {
> ++ rt_i2c_w32(msg->buf[i], REG_DATAOUT_REG);
> ++ if(i == 0)
> ++ rt_i2c_w32(WRITE_CMD, REG_STARTXFR_REG);
> ++
> ++ if (rt_i2c_wait_tx_done())
> ++ goto ERR_TIMEOUT;
> ++ }
> ++ }
> ++
> ++ return 0;
> ++ERR_TIMEOUT:
normally the goto tags are written in lower case (err_timeout:)
> ++ return -ETIMEDOUT;
> ++}
> ++
> ++static int rt_i2c_master_xfer(struct i2c_adapter *a, struct i2c_msg *m, int n)
> ++{
> ++ int i = 0;
> ++ int ret = 0;
> ++ i2c_master_init();
> ++ mt7621_i2c_enable(m);
> ++
> ++ for (i = 0; i != n && ret==0; i++) {
> ++ ret = rt_i2c_handle_msg(a, &m[i]);
> ++
> ++ if (ret) {
> ++ return ret;
> ++ }
> ++ }
> ++
> ++ return i;
> ++}
> ++
> ++static u32 rt_i2c_func(struct i2c_adapter *a)
> ++{
> ++ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> ++}
> ++
> ++static const struct i2c_algorithm rt_i2c_algo = {
> ++ .master_xfer = rt_i2c_master_xfer,
> ++ .functionality = rt_i2c_func,
> ++};
> ++
> ++static int rt_i2c_probe(struct platform_device *pdev)
> ++{
> ++ struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> ++ int ret;
> ++ struct i2c_algo_mt7621_data *adapter_data;
> ++
> ++ adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> ++ if (!adapter) {
> ++ dev_err(&pdev->dev, "failed to allocate i2c_adapter\n");
> ++ ret = -ENOMEM;
> ++ goto out;
> ++ }
> ++ adapter_data = kzalloc(sizeof(struct i2c_algo_mt7621_data), GFP_KERNEL);
> ++ if (!adapter_data) {
> ++ ret = -ENOMEM;
> ++ goto free_adapter;
> ++ }
> ++
please use devm_kzalloc() it will save you the whole kfree() logic at
the end of the driver
> ++ membase = devm_ioremap_resource(&pdev->dev, res);
> ++ if (IS_ERR(membase)){
> ++ ret = -EBUSY;
> ++ goto free_both;
> ++ }
you can add a return -EBUSY here instead of using goto as once you
canged the code above to use devm_kzalloc()
> ++ memsysctlbase = membase - I2C_OFFSET;
not needed when using rt_sysc_w32
> ++
> ++ adapter_data->id = i2c_id++;
> ++ strlcpy(adapter->name, dev_name(&pdev->dev), sizeof(adapter->name));
> ++
it would suprise me if the this is correct. please chek other i2c
drivers to see how they do this
> ++ adapter->owner = THIS_MODULE;
> ++ adapter->nr = pdev->id;
> ++ adapter->timeout = HZ;
> ++ adapter->algo = &rt_i2c_algo;
> ++ adapter->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> ++ adapter->dev.parent = &pdev->dev;
> ++ adapter->dev.of_node = pdev->dev.of_node;
> ++
> ++ init_waitqueue_head(&adapter_data->waitq);
> ++ spin_lock_init(&adapter_data->lock);
> ++
> ++ platform_set_drvdata(pdev, adapter);
> ++ adapter->algo_data = adapter_data;
> ++
> ++ ret = i2c_add_numbered_adapter(adapter);
> ++ if (ret)
> ++ return ret;
> ++
> ++ printk("MT7621A i2c add adapter is ok\n");
> ++
please use pr_info() instead of printk
> ++ return 0;
> ++free_both:
> ++ kfree(adapter_data);
> ++ free_adapter:
> ++ kfree(adapter);
> ++out:
> ++ return ret;
> ++}
> ++
> ++static int rt_i2c_remove(struct platform_device *pdev)
> ++{
> ++ struct i2c_algo_mt7621_data *adapter_data = (struct i2c_algo_mt7621_data *)adapter->algo_data;
> ++ release_mem_region((resource_size_t)membase,0x100);
> ++ kfree(adapter_data);
> ++ kfree(adapter);
> ++ platform_set_drvdata(pdev, NULL);
> ++
all the free calls can go once all allocs are changend to the devm_* api
> ++ return 0;
> ++}
> ++
> ++static const struct of_device_id i2c_rt_dt_ids[] = {
> ++ { .compatible = "ralink,i2c-mt7621", },
> ++ { /* sentinel */ }
> ++};
> ++
> ++MODULE_DEVICE_TABLE(of, i2c_rt_dt_ids);
> ++
> ++static struct platform_driver rt_i2c_driver = {
> ++ .probe = rt_i2c_probe,
> ++ .remove = rt_i2c_remove,
> ++ .driver = {
> ++ .owner = THIS_MODULE,
> ++ .name = "i2c-mt7621",
> ++ .of_match_table = i2c_rt_dt_ids,
> ++ },
> ++};
> ++
> ++static int __init i2c_rt_init (void)
> ++{
> ++ return platform_driver_register(&rt_i2c_driver);
> ++}
> ++
> ++static void __exit i2c_rt_exit (void)
> ++{
> ++ platform_driver_unregister(&rt_i2c_driver);
> ++}
> ++module_init (i2c_rt_init);
> ++module_exit (i2c_rt_exit);
> ++
> ++MODULE_AUTHOR("Steven Liu <steven_liu at mediatek.com>");
> ++MODULE_DESCRIPTION("MT7621 I2c host driver");
> ++MODULE_LICENSE("GPL");
> ++MODULE_ALIAS("platform:MT7621-I2C");
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list