Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood <scottwood@freescale.com>]

Jon Smirl wrote:
> This is my first pass at reworking the Freescale i2c driver. It
> switches the driver from being a platform driver to an open firmware
> one. I've checked it out on my hardware and it is working.

We may want to hold off on this until arch/ppc goes away (or at least 
all users of this driver in arch/ppc).

> You can specific i2c devices on a bus by adding child nodes for them
> in the device tree. It does not know how to autoload the i2c chip
> driver modules.
> 
> i2c@3d40 {
> 	device_type = "i2c";
> 	compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";

dtc supports the "mpc5200b-i2c", "mpc5200-i2c", "fsl-i2c" syntax.

> 	cell-index = <1>;

What is cell-index for?

> 	fsl5200-clocking;

As Matt pointed out, this is redundant.

> 	rtc@32 {
> 		device_type = "rtc";

This isn't really necessary.

> One side effect is that legacy style i2c drivers won't work anymore.

If you mean legacy-style client drivers, why not?

> The driver contains a table for mapping device tree style names to
> linux kernel ones.

Can we please put this in common code instead?

> +static struct i2c_driver_device i2c_devices[] = {
> +	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> +	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> +	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> +	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> +	{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
> +};

At the very least, include the entries that are already being used with 
this driver in fsl_soc.c.

> I'd like to get rid of this table.  There are two obvious solutions,
> use names in the device tree that match up with the linux device
> driver names.

This was proposed and rejected a while ago.  For one thing, some drivers 
handle multiple chips, and we shouldn't base device tree bindings on 
what groupings Linux happens to make in what driver supports what.

> Or push these strings into the chip drivers and modify
> i2c-core to also match on the device tree style names.

That would be ideal; it just hasn't been done yet.

A middle ground for now would be to keep one table in drivers/of or 
something.

> Without one of
> these changes the table is going to need a mapping for every i2c
> device on the market.

Nah, only one for every i2c device Linux supports. :-)

> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 1cf29c9..6f80216 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -306,122 +306,6 @@ err:
> 
>  arch_initcall(gfar_of_init);
> 
> -#ifdef CONFIG_I2C_BOARDINFO
> -#include <linux/i2c.h>
> -struct i2c_driver_device {
> -	char	*of_device;
> -	char	*i2c_driver;
> -	char	*i2c_type;
> -};
> -
> -static struct i2c_driver_device i2c_devices[] __initdata = {
> -	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> -	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> -	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> -	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> -};

This is obviously not based on head-of-tree -- there are several more 
entries in this table.

> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index d8de4ac..5ceb936 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -17,7 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/init.h>
> -#include <linux/platform_device.h>
> +#include <asm/of_platform.h>

Should be linux/of_platform.h

> @@ -180,7 +182,7 @@ static void mpc_i2c_stop(struct mpc_i2c *i2c)
>  static int mpc_write(struct mpc_i2c *i2c, int target,
>  		     const u8 * data, int length, int restart)
>  {
> -	int i;
> +	int i, result;
>  	unsigned timeout = i2c->adap.timeout;
>  	u32 flags = restart ? CCR_RSTA : 0;
> 
> @@ -192,15 +194,15 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
>  	/* Write target byte */
>  	writeb((target << 1), i2c->base + MPC_I2C_DR);
> 
> -	if (i2c_wait(i2c, timeout, 1) < 0)
> -		return -1;
> +	if ((result = i2c_wait(i2c, timeout, 1)) < 0)
> +		return result;
> 
>  	for (i = 0; i < length; i++) {
>  		/* Write data byte */
>  		writeb(data[i], i2c->base + MPC_I2C_DR);
> 
> -		if (i2c_wait(i2c, timeout, 1) < 0)
> -			return -1;
> +		if ((result = i2c_wait(i2c, timeout, 1)) < 0)
> +			return result;
>  	}
> 
>  	return 0;

This is a separate change from the OF-ization -- at least note it in the 
changelog.

> +	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> +		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> +			continue;
> +		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
> +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> +		printk("i2c driver is %s\n", info->driver_name);

Should be KERN_DEBUG, if it stays at all.

> +static void of_register_i2c_devices(struct i2c_adapter *adap, struct
> device_node *adap_node)
> +{
> +	struct device_node *node = NULL;
> +
> +	while ((node = of_get_next_child(adap_node, node))) {
> +		struct i2c_board_info info;
> +		const u32 *addr;
> +		int len;
> +
> +		addr = of_get_property(node, "reg", &len);
> +		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> +			printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
> +			continue;
> +		}
> +
> +		info.irq = irq_of_parse_and_map(node, 0);
> +		if (info.irq == NO_IRQ)
> +			info.irq = -1;
> +
> +		if (of_find_i2c_driver(node, &info) < 0)
> +			continue;
> +
> +		info.platform_data = NULL;
> +		info.addr = *addr;
> +
> +		i2c_new_device(adap, &info);
> +	}
> +}

Most of this code should be made generic and placed in drivers/of.

> +	index = of_get_property(op->node, "cell-index", NULL);
> +    if (!index || *index > 5) {
> +            printk(KERN_ERR "mpc_i2c_probe: Device node %s has invalid "
> +                            "cell-index property\n", op->node->full_name);
> +            return -EINVAL;
> +    }
> +

We might as well just use i2c_new_device() instead of messing around 
with bus numbers.  Note that this is technically no longer platform 
code, so it's harder to justify claiming the static numberspace.

> +	i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
>  	if (!i2c->base) {
>  		printk(KERN_ERR "i2c-mpc - failed to map controller\n");

Use of_iomap().

>  	if (i2c->irq != 0)

if (i2c->irq != NO_IRQ)

> +static struct of_device_id mpc_i2c_of_match[] = {
> +	{
> +		.type		= "i2c",
> +		.compatible	= "fsl-i2c",
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);

Let's take this opportunity to stop matching on device_type here 
(including updating booting-without-of.txt).

> +static struct of_platform_driver mpc_i2c_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= DRV_NAME,
> +	.match_table	= mpc_i2c_of_match,
> +	.probe		= mpc_i2c_probe,
> +	.remove		= __devexit_p(mpc_i2c_remove),
> +	.driver		= {
> +		.name	= DRV_NAME,
>  	},
>  };

Do we still need .name if we have .driver.name?

> diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> index 0242d80..b778d35 100644
> --- a/drivers/rtc/rtc-pcf8563.c
> +++ b/drivers/rtc/rtc-pcf8563.c

This should be a separate patch.

-Scott