From: Scott Wood <scottwood@freescale•com>
To: Jon Smirl <jonsmirl@gmail•com>
Cc: Tjernlund <tjernlund@tjernlund•se>,
linuxppc-dev@ozlabs•org, Jean Delvare <khali@linux-fr•org>,
i2c@lm-sensors•org
Subject: Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Date: Mon, 05 Nov 2007 13:43:03 -0600 [thread overview]
Message-ID: <472F7247.9070106@freescale.com> (raw)
In-Reply-To: <9e4733910711050714l2aa3a5eeqf5327c3e0d8ca490@mail.gmail.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
next prev parent reply other threads:[~2007-11-05 19:50 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-11-05 15:14 [RFC] Rework of i2c-mpc.c - Freescale i2c driver Jon Smirl
2007-11-05 19:22 ` Matt Sealey
2007-11-05 19:51 ` Jon Smirl
2007-11-05 19:55 ` Scott Wood
2007-11-05 20:04 ` Jon Smirl
2007-11-05 20:06 ` Scott Wood
2007-11-05 20:11 ` Grant Likely
2007-11-05 19:43 ` Scott Wood [this message]
2007-11-05 20:30 ` Jon Smirl
2007-11-05 20:51 ` Scott Wood
2007-11-05 21:52 ` Matt Sealey
2007-11-05 21:55 ` Scott Wood
2007-11-05 23:03 ` Grant Likely
2007-11-06 17:32 ` Jean Delvare
2007-11-06 18:53 ` Matt Sealey
2007-11-06 20:31 ` Jean Delvare
2007-11-06 21:06 ` Matt Sealey
2007-11-05 22:46 ` Grant Likely
2007-11-06 0:33 ` Jon Smirl
2007-11-06 22:20 ` David Gibson
2007-11-06 0:41 ` Jon Smirl
2007-11-06 17:02 ` Scott Wood
2007-11-06 4:25 ` Jon Smirl
2007-11-06 4:40 ` Stephen Rothwell
2007-11-06 19:02 ` Jon Smirl
2007-11-06 22:22 ` David Gibson
2007-11-06 17:29 ` Jean Delvare
2007-11-06 17:36 ` Scott Wood
2007-11-06 18:10 ` Jean Delvare
2007-11-06 18:26 ` Grant Likely
2007-11-06 18:26 ` Grant Likely
2007-11-06 19:34 ` Jean Delvare
2007-11-06 18:29 ` Scott Wood
2007-11-06 17:45 ` Jon Smirl
2007-11-06 18:17 ` Jean Delvare
2007-11-06 19:07 ` Jon Smirl
2007-11-06 1:34 ` Jon Smirl
2007-11-06 2:28 ` Stephen Rothwell
2007-11-05 20:03 ` Grant Likely
2007-11-05 20:41 ` Jon Smirl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=472F7247.9070106@freescale.com \
--to=scottwood@freescale$(echo .)com \
--cc=i2c@lm-sensors$(echo .)org \
--cc=jonsmirl@gmail$(echo .)com \
--cc=khali@linux-fr$(echo .)org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=tjernlund@tjernlund$(echo .)se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox