Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Tue, Jun 10, 2008 at 10:40:45PM -0400, Jon Smirl wrote:
> Convert i2c-mpc from a platform driver into an of_platform driver.
> This patch is much smaller since Jochen already added
> of_find_i2c_driver(). Versions of this have been posted before.
>
> Signed-ff-by: Jon Smirl <[EMAIL PROTECTED]>
Typo: Signed-off... (I'm curious, do such typos enforce resending the
patch?)
Tested-by: Wolfram Sang <[EMAIL PROTECTED]>
>
> --
>
> drivers/i2c/busses/i2c-mpc.c | 105
> +-
> 1 files changed, 63 insertions(+), 42 deletions(-)
>
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index a076129..76d8091 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -18,6 +18,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -25,13 +27,13 @@
> #include
> #include
>
> -#define MPC_I2C_ADDR 0x00
> +#define DRV_NAME "mpc-i2c"
> +
> #define MPC_I2C_FDR 0x04
> #define MPC_I2C_CR 0x08
> #define MPC_I2C_SR 0x0c
> #define MPC_I2C_DR 0x10
> #define MPC_I2C_DFSRR 0x14
> -#define MPC_I2C_REGION 0x20
>
> #define CCR_MEN 0x80
> #define CCR_MIEN 0x40
> @@ -315,102 +317,121 @@ static struct i2c_adapter mpc_ops = {
> .timeout = 1,
> };
>
> -static int fsl_i2c_probe(struct platform_device *pdev)
> +static int fsl_i2c_probe(struct of_device *op, const struct
> of_device_id *match)
The above two lines have to be concatenated, otherwise the patch will
not apply. Also, __devinit?
> {
> int result = 0;
> struct mpc_i2c *i2c;
> - struct fsl_i2c_platform_data *pdata;
> - struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> - pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
>
> i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> if (!i2c)
> return -ENOMEM;
>
> - i2c->irq = platform_get_irq(pdev, 0);
> - if (i2c->irq < 0)
> - i2c->irq = NO_IRQ; /* Use polling */
> + if (of_get_property(op->node, "dfsrr", NULL))
> + i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
>
> - i2c->flags = pdata->device_flags;
> - init_waitqueue_head(&i2c->queue);
> + if (of_device_is_compatible(op->node, "mpc5200-i2c"))
> + i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
>
> - i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
> + init_waitqueue_head(&i2c->queue);
>
> + i2c->base = of_iomap(op->node, 0);
> if (!i2c->base) {
> printk(KERN_ERR "i2c-mpc - failed to map controller\n");
> result = -ENOMEM;
> goto fail_map;
> }
>
> - if (i2c->irq != NO_IRQ)
> - if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> - IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> - printk(KERN_ERR
> -"i2c-mpc - failed to attach interrupt\n");
> - goto fail_irq;
> - }
> + i2c->irq = irq_of_parse_and_map(op->node, 0);
> + if (i2c->irq == NO_IRQ) {
> + result = -ENXIO;
Minor thing, but I wonder if -EINVAL is more appropriate?
> + goto fail_irq;
> + }
> +
> + result = request_irq(i2c->irq, mpc_i2c_isr,
> + IRQF_SHARED, "i2c-mpc", i2c);
> + if (result < 0) {
> + printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> + goto fail_request;
> + }
>
> mpc_i2c_setclock(i2c);
> - platform_set_drvdata(pdev, i2c);
> +
> + dev_set_drvdata(&op->dev, i2c);
>
> i2c->adap = mpc_ops;
> - i2c->adap.nr = pdev->id;
> i2c_set_adapdata(&i2c->adap, i2c);
> - i2c->adap.dev.parent = &pdev->dev;
> - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> + i2c->adap.dev.parent = &op->dev;
> +
> + result = i2c_add_adapter(&i2c->adap);
> + if (result < 0) {
> printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
> goto fail_add;
> }
> + of_register_i2c_devices(&i2c->adap, op->node);
>
> return result;
>
> - fail_add:
> - if (i2c->irq != NO_IRQ)
> - free_irq(i2c->irq, i2c);
> - fail_irq:
> + fail_add:
> + dev_set_drvdata(&op->dev, NULL);
> + free_irq(i2c->irq, i2c);
> + fail_request:
> + irq_dispose_mapping(i2c->irq);
> + fail_irq:
> iounmap(i2c->base);
> - fail_map:
> + fail_map:
> kfree(i2c);
> return result;
> };
>
> -static int fsl_i2c_remove(struct platform_device *pdev)
> +static int fsl_i2c_remove(struct of_device *op)
__devexit?
> {
> - struct mpc_i2c *i2c = platform_get_drvdata(pdev);
> + struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
>
> i2c_del_adapter(&i2c->adap);
> - platform_set_drvdata(pdev, NULL);
> + dev_set_drvdata(&op->dev, NULL);
>
> if (i2c->irq != NO_IRQ)
> fr
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On 6/11/08, Wolfram Sang <[EMAIL PROTECTED]> wrote:
> On Tue, Jun 10, 2008 at 10:40:45PM -0400, Jon Smirl wrote:
> > Convert i2c-mpc from a platform driver into an of_platform driver.
> > This patch is much smaller since Jochen already added
> > of_find_i2c_driver(). Versions of this have been posted before.
> >
> > Signed-ff-by: Jon Smirl <[EMAIL PROTECTED]>
>
>
> Typo: Signed-off... (I'm curious, do such typos enforce resending the
> patch?)
I just cut and pasted this version to get the comments. Next pass I
will send it using stgit which will add the right signed-off line and
fix the wrapping.
>
> Tested-by: Wolfram Sang <[EMAIL PROTECTED]>
>
>
> >
> > --
> >
> > drivers/i2c/busses/i2c-mpc.c | 105
> +-
> > 1 files changed, 63 insertions(+), 42 deletions(-)
> >
> >
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index a076129..76d8091 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -18,6 +18,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > #include
> > #include
> > @@ -25,13 +27,13 @@
> > #include
> > #include
> >
> > -#define MPC_I2C_ADDR 0x00
> > +#define DRV_NAME "mpc-i2c"
> > +
> > #define MPC_I2C_FDR 0x04
> > #define MPC_I2C_CR 0x08
> > #define MPC_I2C_SR 0x0c
> > #define MPC_I2C_DR 0x10
> > #define MPC_I2C_DFSRR 0x14
> > -#define MPC_I2C_REGION 0x20
> >
> > #define CCR_MEN 0x80
> > #define CCR_MIEN 0x40
> > @@ -315,102 +317,121 @@ static struct i2c_adapter mpc_ops = {
> > .timeout = 1,
> > };
> >
> > -static int fsl_i2c_probe(struct platform_device *pdev)
> > +static int fsl_i2c_probe(struct of_device *op, const struct
> > of_device_id *match)
>
>
> The above two lines have to be concatenated, otherwise the patch will
> not apply. Also, __devinit?
>
>
> > {
> > int result = 0;
> > struct mpc_i2c *i2c;
> > - struct fsl_i2c_platform_data *pdata;
> > - struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -
> > - pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
> >
> > i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> > if (!i2c)
> > return -ENOMEM;
> >
> > - i2c->irq = platform_get_irq(pdev, 0);
> > - if (i2c->irq < 0)
> > - i2c->irq = NO_IRQ; /* Use polling */
> > + if (of_get_property(op->node, "dfsrr", NULL))
> > + i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
> >
> > - i2c->flags = pdata->device_flags;
> > - init_waitqueue_head(&i2c->queue);
> > + if (of_device_is_compatible(op->node, "mpc5200-i2c"))
> > + i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
> >
> > - i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
> > + init_waitqueue_head(&i2c->queue);
> >
> > + i2c->base = of_iomap(op->node, 0);
> > if (!i2c->base) {
> > printk(KERN_ERR "i2c-mpc - failed to map controller\n");
> > result = -ENOMEM;
> > goto fail_map;
> > }
> >
> > - if (i2c->irq != NO_IRQ)
> > - if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> > - IRQF_SHARED, "i2c-mpc", i2c)) < 0)
> {
> > - printk(KERN_ERR
> > -"i2c-mpc - failed to attach interrupt\n");
> > - goto fail_irq;
> > - }
> > + i2c->irq = irq_of_parse_and_map(op->node, 0);
> > + if (i2c->irq == NO_IRQ) {
> > + result = -ENXIO;
>
>
> Minor thing, but I wonder if -EINVAL is more appropriate?
>
>
> > + goto fail_irq;
> > + }
> > +
> > + result = request_irq(i2c->irq, mpc_i2c_isr,
> > + IRQF_SHARED, "i2c-mpc", i2c);
> > + if (result < 0) {
> > + printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> > + goto fail_request;
> > + }
> >
> > mpc_i2c_setclock(i2c);
> > - platform_set_drvdata(pdev, i2c);
> > +
> > + dev_set_drvdata(&op->dev, i2c);
> >
> > i2c->adap = mpc_ops;
> > - i2c->adap.nr = pdev->id;
> > i2c_set_adapdata(&i2c->adap, i2c);
> > - i2c->adap.dev.parent = &pdev->dev;
> > - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> > + i2c->adap.dev.parent = &op->dev;
> > +
> > + result = i2c_add_adapter(&i2c->adap);
> > + if (result < 0) {
> > printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
> > goto fail_add;
> > }
> > + of_register_i2c_devices(&i2c->adap, op->node);
> >
> > return result;
> >
> > - fail_add:
> > - if (i2c->irq != NO_IRQ)
> > - free_irq(i2c->irq, i2c);
> > - fail_irq:
> > + fail_add:
> > + dev_set_drvdata(&op->dev, NULL);
> > + free_irq(i
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
Hi Jon, On Wed, 11 Jun 2008 12:14:08 -0400, Jon Smirl wrote: > On 6/11/08, Wolfram Sang <[EMAIL PROTECTED]> wrote: > > On Tue, Jun 10, 2008 at 10:40:45PM -0400, Jon Smirl wrote: > > > Convert i2c-mpc from a platform driver into an of_platform driver. > > > This patch is much smaller since Jochen already added > > > of_find_i2c_driver(). Versions of this have been posted before. > > > > > > Signed-ff-by: Jon Smirl <[EMAIL PROTECTED]> > > > > Typo: Signed-off... (I'm curious, do such typos enforce resending the > > patch?) In theory it should. The line means that you've read the "Developer's Certificate of Origin" and you agree with it, so it's somewhat binding. I guess a lawyer would argue that the line isn't worth anything if "Signed-off-by" isn't spelled properly. Thanks Wolfram for the review, BTW. > I just cut and pasted this version to get the comments. Next pass I > will send it using stgit which will add the right signed-off line and > fix the wrapping. That's doing things the wrong way around. If you want people to comment on your patch, the least you can do is make sure they can apply it. If I can't apply your patch, that means I can't verify if it applies properly, I can't run checkpatch.pl on it, I can't get quilt to color it, and I can't compare it to a previous version of the same patch. Also, it would be a good idea to keep Jochen Friedrich in the loop (Cc'd), as he proposed a similar patch based on a previous version of yours back in April. I suppose he has some interest in it. While mentioning Jochen's version of the patch: it was deleting 123 lines from arch/powerpc/sysdev/fsl_soc.c. Yours doesn't. Should it? http://lists.lm-sensors.org/pipermail/i2c/2008-April/003314.html -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
Hi Jon,
> Convert i2c-mpc from a platform driver into an of_platform driver.
> This patch is much smaller since Jochen already added
> of_find_i2c_driver(). Versions of this have been posted before.
>
> Signed-ff-by: Jon Smirl <[EMAIL PROTECTED]>
>
> --
This separator is supposed to be 3 dashes, NOT 2 dashes followed by a
space. The latter makes the patch look like your e-mail signature, and
e-mail clients usually strip that when one replies. Which is pretty
inconvenient when commenting on a patch ;)
>
> drivers/i2c/busses/i2c-mpc.c | 105
> +-
> 1 files changed, 63 insertions(+), 42 deletions(-)
>
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index a076129..76d8091 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -18,6 +18,8 @@
> #include
> #include
> #include
If the driver is no longer a platform driver, do you still need to
include ?
> +#include
> +#include
>
> #include
> #include
> @@ -25,13 +27,13 @@
> #include
> #include
>
> -#define MPC_I2C_ADDR 0x00
> +#define DRV_NAME "mpc-i2c"
> +
> #define MPC_I2C_FDR 0x04
> #define MPC_I2C_CR 0x08
> #define MPC_I2C_SR 0x0c
> #define MPC_I2C_DR 0x10
> #define MPC_I2C_DFSRR 0x14
> -#define MPC_I2C_REGION 0x20
>
> #define CCR_MEN 0x80
> #define CCR_MIEN 0x40
> @@ -315,102 +317,121 @@ static struct i2c_adapter mpc_ops = {
> .timeout = 1,
> };
>
> -static int fsl_i2c_probe(struct platform_device *pdev)
> +static int fsl_i2c_probe(struct of_device *op, const struct
> of_device_id *match)
> {
> int result = 0;
> struct mpc_i2c *i2c;
> - struct fsl_i2c_platform_data *pdata;
> - struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> - pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
>
> i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
> if (!i2c)
> return -ENOMEM;
>
> - i2c->irq = platform_get_irq(pdev, 0);
> - if (i2c->irq < 0)
> - i2c->irq = NO_IRQ; /* Use polling */
> + if (of_get_property(op->node, "dfsrr", NULL))
> + i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
>
> - i2c->flags = pdata->device_flags;
> - init_waitqueue_head(&i2c->queue);
> + if (of_device_is_compatible(op->node, "mpc5200-i2c"))
> + i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
>
> - i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
> + init_waitqueue_head(&i2c->queue);
>
> + i2c->base = of_iomap(op->node, 0);
> if (!i2c->base) {
> printk(KERN_ERR "i2c-mpc - failed to map controller\n");
> result = -ENOMEM;
> goto fail_map;
> }
>
> - if (i2c->irq != NO_IRQ)
> - if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> - IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> - printk(KERN_ERR
> -"i2c-mpc - failed to attach interrupt\n");
> - goto fail_irq;
> - }
> + i2c->irq = irq_of_parse_and_map(op->node, 0);
> + if (i2c->irq == NO_IRQ) {
> + result = -ENXIO;
> + goto fail_irq;
> + }
This is a significant functionality change. The original code supported
devices with no IRQ, now your enforce the use of an IRQ and fail if
it's not there. If you have any reason to do that, this must be in a
separate patch, and this must be done consistently (removing the
polling code in i2c_wait, etc.)
> +
> + result = request_irq(i2c->irq, mpc_i2c_isr,
> + IRQF_SHARED, "i2c-mpc", i2c);
> + if (result < 0) {
> + printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> + goto fail_request;
> + }
>
> mpc_i2c_setclock(i2c);
> - platform_set_drvdata(pdev, i2c);
> +
> + dev_set_drvdata(&op->dev, i2c);
>
> i2c->adap = mpc_ops;
> - i2c->adap.nr = pdev->id;
> i2c_set_adapdata(&i2c->adap, i2c);
> - i2c->adap.dev.parent = &pdev->dev;
> - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> + i2c->adap.dev.parent = &op->dev;
> +
> + result = i2c_add_adapter(&i2c->adap);
The driver was previously using i2c_add_numbered_adapter(), giving MPC
platform the possibility to use new-style i2c drivers:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44
You're breaking this, I doubt it's on purpose?
> + if (result < 0) {
> printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
> goto fail_add;
> }
> + of_register_i2c_devices(&i2c->adap, op->node);
>
> return result;
>
> - fail_add:
> - if (i2c->irq != NO_IRQ)
> - free_irq(i2c->irq, i2c);
> - fail_irq:
> + fail_add:
> + dev_set_drvdata(&op->dev, NULL);
> + free_irq(i2c->irq, i2c);
> + f
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On 6/25/08, Jean Delvare <[EMAIL PROTECTED]> wrote:
> >
> > i2c->adap = mpc_ops;
> > - i2c->adap.nr = pdev->id;
> > i2c_set_adapdata(&i2c->adap, i2c);
> > - i2c->adap.dev.parent = &pdev->dev;
> > - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> > + i2c->adap.dev.parent = &op->dev;
> > +
> > + result = i2c_add_adapter(&i2c->adap);
>
>
> The driver was previously using i2c_add_numbered_adapter(), giving MPC
> platform the possibility to use new-style i2c drivers:
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44
> You're breaking this, I doubt it's on purpose?
Grant, what do you want here? You're the one who converted it to
i2c_add_numbered_adapter. But in other posts you've said that the
device tree should have nothing to do with bus numbering.
Once this driver is converted to an OF one it shouldn't need bus ids
since all of the i2c devices will be children of the bus node. We can
just let the i2c subsystem assign a bus number.
Timur has some issues with the i2c bus number in his ALSA driver. The
problem is locating the i2c device when the i2s driver loads. Parsing
the device tree to extract an i2c bus and device number is not a good
solution.
codec-handle should give you the i2c device node. But then we can't
use of_find_device_by_node because the i2c device is not an of_device,
it's a cross platform i2c_device. Should of_find_device_by_node()
return a 'struct device' instead of a 'struct of_device' and leave it
as a user exercise to cast up? It is used nine times in the kernel,
mostly sparc.
[EMAIL PROTECTED] { /* PSC1 in i2s mode */
device_type = "sound";
compatible = "mpc5200b-psc-i2s","fsl,mpc5200b-psc-i2s";
cell-index = <0>;
reg = <0x2000 0x100>;
interrupts = <0x2 0x2 0x0>;
interrupt-parent = <&mpc5200_pic>;
codec-handle = <&tas0>;
};
[EMAIL PROTECTED] {
#address-cells = <1>;
#size-cells = <0>;
compatible =
"fsl,mpc5200b-i2c","fsl,mpc5200-i2c","fsl-i2c";
cell-index = <0>;
reg = <0x3d00 0x40>;
interrupts = <0x2 0xf 0x0>;
interrupt-parent = <&mpc5200_pic>;
fsl5200-clocking;
tas0:[EMAIL PROTECTED] {
compatible = "ti,tas5504";
reg = <0x1b>;
};
};
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sat, Jun 28, 2008 at 10:05:28PM -0400, Jon Smirl wrote:
> On 6/25/08, Jean Delvare <[EMAIL PROTECTED]> wrote:
> > >
> > > i2c->adap = mpc_ops;
> > > - i2c->adap.nr = pdev->id;
> > > i2c_set_adapdata(&i2c->adap, i2c);
> > > - i2c->adap.dev.parent = &pdev->dev;
> > > - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> > > + i2c->adap.dev.parent = &op->dev;
> > > +
> > > + result = i2c_add_adapter(&i2c->adap);
> >
> >
> > The driver was previously using i2c_add_numbered_adapter(), giving MPC
> > platform the possibility to use new-style i2c drivers:
> >
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44
> > You're breaking this, I doubt it's on purpose?
>
> Grant, what do you want here? You're the one who converted it to
> i2c_add_numbered_adapter. But in other posts you've said that the
> device tree should have nothing to do with bus numbering.
Yes, I did make that change, but that was when it was a platform bus
driver. Converting it to an of_platform bus driver entirely changes the
situation and it should go back to using i2c_add_adapter() with a parse
of the device tree for child nodes.
> Once this driver is converted to an OF one it shouldn't need bus ids
> since all of the i2c devices will be children of the bus node. We can
> just let the i2c subsystem assign a bus number.
Exactly.
> Timur has some issues with the i2c bus number in his ALSA driver. The
> problem is locating the i2c device when the i2s driver loads. Parsing
> the device tree to extract an i2c bus and device number is not a good
> solution.
The trick here is to store the pointer to the device node inside the
i2c device. I do this with SPI devices like this:
/* Store a pointer to the node in the device structure */
of_node_get(nc);
spi->dev.archdata.of_node = nc;
Then, when you've got a device_node pointer, you can parse through the
set of registered SPI devices and match against the one that has the
same device node pointer.
> codec-handle should give you the i2c device node. But then we can't
> use of_find_device_by_node because the i2c device is not an of_device,
> it's a cross platform i2c_device. Should of_find_device_by_node()
> return a 'struct device' instead of a 'struct of_device' and leave it
> as a user exercise to cast up? It is used nine times in the kernel,
> mostly sparc.
No, this doesn't work because I2C and SPI devices are not of_platform
devices. They aren't even platform devices. of_find_device_by_node()
only works for the of_platform bus.
Using archdata.of_node is part of the device structure and works for
every bus type (platform, of_platform, SPI, I2C, PCI, etc.)
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
Hi Jon, Grant,
On Sat, 28 Jun 2008 22:49:40 -0600, Grant Likely wrote:
> On Sat, Jun 28, 2008 at 10:05:28PM -0400, Jon Smirl wrote:
> > On 6/25/08, Jean Delvare <[EMAIL PROTECTED]> wrote:
> > > >
> > > > i2c->adap = mpc_ops;
> > > > - i2c->adap.nr = pdev->id;
> > > > i2c_set_adapdata(&i2c->adap, i2c);
> > > > - i2c->adap.dev.parent = &pdev->dev;
> > > > - if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> > > > + i2c->adap.dev.parent = &op->dev;
> > > > +
> > > > + result = i2c_add_adapter(&i2c->adap);
> > >
> > >
> > > The driver was previously using i2c_add_numbered_adapter(), giving MPC
> > > platform the possibility to use new-style i2c drivers:
> > >
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44
> > > You're breaking this, I doubt it's on purpose?
> >
> > Grant, what do you want here? You're the one who converted it to
> > i2c_add_numbered_adapter. But in other posts you've said that the
> > device tree should have nothing to do with bus numbering.
>
> Yes, I did make that change, but that was when it was a platform bus
> driver. Converting it to an of_platform bus driver entirely changes the
> situation and it should go back to using i2c_add_adapter() with a parse
> of the device tree for child nodes.
I am surprised and disappointed, as this sounds like a regression.
Registering the i2c buses with random numbers and parsing the device
tree later to figure out which devices are where, is what everybody was
doing before the new i2c device/driver matching model was implemented,
because there was no other way. I'm curious why you are going back to
this approach when i2c-core now offers something way cleaner and more
efficient.
But well, you'll know better. I'm not familiar with the platform, and
don't have the time to look into it anyway.
> > Once this driver is converted to an OF one it shouldn't need bus ids
> > since all of the i2c devices will be children of the bus node. We can
> > just let the i2c subsystem assign a bus number.
>
> Exactly.
>
> > Timur has some issues with the i2c bus number in his ALSA driver. The
> > problem is locating the i2c device when the i2s driver loads. Parsing
> > the device tree to extract an i2c bus and device number is not a good
> > solution.
>
> The trick here is to store the pointer to the device node inside the
> i2c device. I do this with SPI devices like this:
>
> /* Store a pointer to the node in the device structure */
> of_node_get(nc);
> spi->dev.archdata.of_node = nc;
>
> Then, when you've got a device_node pointer, you can parse through the
> set of registered SPI devices and match against the one that has the
> same device node pointer.
>
> > codec-handle should give you the i2c device node. But then we can't
> > use of_find_device_by_node because the i2c device is not an of_device,
> > it's a cross platform i2c_device. Should of_find_device_by_node()
> > return a 'struct device' instead of a 'struct of_device' and leave it
> > as a user exercise to cast up? It is used nine times in the kernel,
> > mostly sparc.
>
> No, this doesn't work because I2C and SPI devices are not of_platform
> devices. They aren't even platform devices. of_find_device_by_node()
> only works for the of_platform bus.
>
> Using archdata.of_node is part of the device structure and works for
> every bus type (platform, of_platform, SPI, I2C, PCI, etc.)
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sun, Jun 29, 2008 at 08:31:43AM +0200, Jean Delvare wrote: > Hi Jon, Grant, > > On Sat, 28 Jun 2008 22:49:40 -0600, Grant Likely wrote: > > On Sat, Jun 28, 2008 at 10:05:28PM -0400, Jon Smirl wrote: > > > > > > > > The driver was previously using i2c_add_numbered_adapter(), giving MPC > > > > platform the possibility to use new-style i2c drivers: > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44 > > > > You're breaking this, I doubt it's on purpose? > > > > > > Grant, what do you want here? You're the one who converted it to > > > i2c_add_numbered_adapter. But in other posts you've said that the > > > device tree should have nothing to do with bus numbering. > > > > Yes, I did make that change, but that was when it was a platform bus > > driver. Converting it to an of_platform bus driver entirely changes the > > situation and it should go back to using i2c_add_adapter() with a parse > > of the device tree for child nodes. > > I am surprised and disappointed, as this sounds like a regression. > Registering the i2c buses with random numbers and parsing the device > tree later to figure out which devices are where, is what everybody was > doing before the new i2c device/driver matching model was implemented, > because there was no other way. I'm curious why you are going back to > this approach when i2c-core now offers something way cleaner and more > efficient. Ah, but the whole random number parsing thing is no longer necessary because we ensure that registration of i2c devices always occurs after the i2c adapter is created (for device tree aware i2c adapter drivers. adapters that aren't device tree aware still need to assign a number). After the i2c adapter registers itself, of_register_i2c_devices() is called which walks through the child nodes of the i2c adapter node in the device tree. Each child node is an i2c device, and it immediately get registered with the adapter. Because this ensures that i2c device registration always happens after adapter registration, and since the pointer to the i2c_adapter is known, then i2c_new_device() can be used directly without ever needing to know the bus number. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sun, 29 Jun 2008 00:58:12 -0600, Grant Likely wrote: > On Sun, Jun 29, 2008 at 08:31:43AM +0200, Jean Delvare wrote: > > Hi Jon, Grant, > > > > On Sat, 28 Jun 2008 22:49:40 -0600, Grant Likely wrote: > > > On Sat, Jun 28, 2008 at 10:05:28PM -0400, Jon Smirl wrote: > > > > > > > > > > The driver was previously using i2c_add_numbered_adapter(), giving MPC > > > > > platform the possibility to use new-style i2c drivers: > > > > > > > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=1469fa263870acd890a4b9f6ef557acc5d673b44 > > > > > You're breaking this, I doubt it's on purpose? > > > > > > > > Grant, what do you want here? You're the one who converted it to > > > > i2c_add_numbered_adapter. But in other posts you've said that the > > > > device tree should have nothing to do with bus numbering. > > > > > > Yes, I did make that change, but that was when it was a platform bus > > > driver. Converting it to an of_platform bus driver entirely changes the > > > situation and it should go back to using i2c_add_adapter() with a parse > > > of the device tree for child nodes. > > > > I am surprised and disappointed, as this sounds like a regression. > > Registering the i2c buses with random numbers and parsing the device > > tree later to figure out which devices are where, is what everybody was > > doing before the new i2c device/driver matching model was implemented, > > because there was no other way. I'm curious why you are going back to > > this approach when i2c-core now offers something way cleaner and more > > efficient. > > Ah, but the whole random number parsing thing is no longer necessary > because we ensure that registration of i2c devices always occurs > after the i2c adapter is created (for device tree aware i2c adapter > drivers. adapters that aren't device tree aware still need to assign a > number). > > After the i2c adapter registers itself, of_register_i2c_devices() is called > which walks through the child nodes of the i2c adapter node in the device > tree. Each child node is an i2c device, and it immediately get > registered with the adapter. Because this ensures that i2c device > registration always happens after adapter registration, and since the > pointer to the i2c_adapter is known, then i2c_new_device() can be used > directly without ever needing to know the bus number. Ah, OK. If you use i2c_new_device() then it's alright. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sun, 29 Jun 2008 09:17:25 +0200
"Jean Delvare" <[EMAIL PROTECTED]> wrote:
> Ah, OK. If you use i2c_new_device() then it's alright.
Correct.
I have done the same thing for the i2c-ibm_iic.c driver. Jean, I think
you will like this. It gets rid of the index and the numbered drivers.
And the walking of the device tree is very clean because the dts knows
all the devices.
For example here is the relevant portion of the dts for the Warp:
IIC0: [EMAIL PROTECTED] {
compatible = "ibm,iic-440ep", "ibm,iic-440gp", "ibm,iic";
reg = ;
interrupt-parent = <&UIC0>;
interrupts = <2 4>;
#address-cells = <1>;
#size-cells = <0>;
[EMAIL PROTECTED] {
compatible = "adi,ad7414";
reg = <4a>;
interrupts = <19 8>;
interrupt-parent = <&UIC0>;
};
};
It clearly shows that first i2c controller (IIC0) contains one ad7414 device at
address 4A.
Cheers,
Sean
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sun, 29 Jun 2008 12:24:39 -0400, Sean MacLennan wrote:
> On Sun, 29 Jun 2008 09:17:25 +0200
> "Jean Delvare" <[EMAIL PROTECTED]> wrote:
>
> > Ah, OK. If you use i2c_new_device() then it's alright.
>
> Correct.
>
> I have done the same thing for the i2c-ibm_iic.c driver. Jean, I think
> you will like this. It gets rid of the index and the numbered drivers.
> And the walking of the device tree is very clean because the dts knows
> all the devices.
>
> For example here is the relevant portion of the dts for the Warp:
>
> IIC0: [EMAIL PROTECTED] {
> compatible = "ibm,iic-440ep", "ibm,iic-440gp", "ibm,iic";
> reg = ;
> interrupt-parent = <&UIC0>;
> interrupts = <2 4>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> [EMAIL PROTECTED] {
> compatible = "adi,ad7414";
> reg = <4a>;
> interrupts = <19 8>;
> interrupt-parent = <&UIC0>;
> };
> };
>
> It clearly shows that first i2c controller (IIC0) contains one ad7414 device
> at address 4A.
That's fine with me. I expected the dts to be converted to platform
initialization data (i2c_board_info structures) being registered with
i2c_register_board_info() and numbered adapters. But if you prefer
unnumbered adapters and the platform code or the bus driver itself
calls i2c_new_device() based on the dts, that should work too.
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sunday 29 June 2008, Jean Delvare wrote: > > > After the i2c adapter registers itself, of_register_i2c_devices() is called > > which walks through the child nodes of the i2c adapter node in the device > > tree. Each child node is an i2c device, and it immediately get > > registered with the adapter. Because this ensures that i2c device > > registration always happens after adapter registration, and since the > > pointer to the i2c_adapter is known, then i2c_new_device() can be used > > directly without ever needing to know the bus number. > > Ah, OK. If you use i2c_new_device() then it's alright. Right. Conceptually the way that the i2c core uses "numbered" adapters and registered board_info could be viewed as a way to let platforms avoid tracking that stuff themselves. Since the of_* framework is already tracking that, there's no big win in trying to have i2c-core track that too, on its behalf. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
