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.
>
> [EMAIL PROTECTED] {
> 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.
> [EMAIL PROTECTED] {
> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
