[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