[PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Grant Likely]

Move the i2c-mpc driver over to using the new i2c infrastructure.
Specifically, it now uses i2c_add_numberd_adapter so that the bus number
can be determined ahead of time and used to register i2c clients before
the bus is instantiated.

Tested on an MPC5200 based board

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
This patch will need to be tested to make sure it does not break any 8xxx
board ports.

Work still to be done (in another patch): support for pulling i2c client
registrations out of the device tree.

 drivers/i2c/busses/i2c-mpc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index c6b6898..a769efc 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -327,9 +327,10 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, 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_adapter(&i2c->adap)) < 0) {
+	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
 		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
 		goto fail_add;
 	}

Re: [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Grant Likely]

On 5/14/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> Work still to be done (in another patch): support for pulling i2c client
> registrations out of the device tree.

I had also fogotten about Scott Woods patches from last Nov. which
take care of this.

g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Jean Delvare]

On Mon, 14 May 2007 13:11:23 -0600, Grant Likely wrote:
> Move the i2c-mpc driver over to using the new i2c infrastructure.
> Specifically, it now uses i2c_add_numberd_adapter so that the bus number

i2c_add_numbered_adapter (missing e)

> can be determined ahead of time and used to register i2c clients before
> the bus is instantiated.
> 
> Tested on an MPC5200 based board
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jean Delvare <khali@linux-fr.org>

> ---
> This patch will need to be tested to make sure it does not break any 8xxx
> board ports.
> 
> Work still to be done (in another patch): support for pulling i2c client
> registrations out of the device tree.
> 
>  drivers/i2c/busses/i2c-mpc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index c6b6898..a769efc 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -327,9 +327,10 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, 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_adapter(&i2c->adap)) < 0) {
> +	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
>  		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
>  		goto fail_add;
>  	}

-- 
Jean Delvare

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[David Brownell]

On Tuesday 15 May 2007, Jean Delvare wrote:
> On Mon, 14 May 2007 13:11:23 -0600, Grant Likely wrote:

> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -327,9 +327,10 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, i2c);
> >  
> >  	i2c->adap = mpc_ops;
> > +	i2c->adap.nr = pdev->id;

By the way:  mpc_ops is a static i2c_adapter, so given that
the reason for using pdev->id that way was that there might
be more than one such platform device ... shouldn't allocation
of the adapter be moved into allocation of the "i2c->" object?

Or at least, add a check to ensure that the static mpc_ops
structure isn't in use before progressing this probe().

- Dave


> >  	i2c_set_adapdata(&i2c->adap, i2c);
> >  	i2c->adap.dev.parent = &pdev->dev;
> > -	if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
> > +	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> >  		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
> >  		goto fail_add;
> >  	}
> 
> -- 
> Jean Delvare
> 

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Grant Likely]

On 5/15/07, David Brownell <david-b@pacbell.net> wrote:
> On Tuesday 15 May 2007, Jean Delvare wrote:
> > On Mon, 14 May 2007 13:11:23 -0600, Grant Likely wrote:
>
> > > --- a/drivers/i2c/busses/i2c-mpc.c
> > > +++ b/drivers/i2c/busses/i2c-mpc.c
> > > @@ -327,9 +327,10 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> > >     platform_set_drvdata(pdev, i2c);
> > >
> > >     i2c->adap = mpc_ops;
> > > +   i2c->adap.nr = pdev->id;
>
> By the way:  mpc_ops is a static i2c_adapter, so given that
> the reason for using pdev->id that way was that there might
> be more than one such platform device ... shouldn't allocation
> of the adapter be moved into allocation of the "i2c->" object?

Take another look; this is a funny quirk of the driver.  The
assignment is 'i2c->adap = mpc_ops'; not 'i2c->adap = &mpc_ops'.  And
in struct mpc_i2c, the field is declared as 'struct i2c_adapter adap',
not 'struct i2c_adapter *adap'.  The driver instance gets a copy of
the mpc_ops structure to initialize it, not a pointer to the staticly
defined structure.  I got bitten by the same thing when I was looking
at the code.

Not to mention that mpc_ops is poorly named.

Cheers,
g.


-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[David Brownell]

On Tuesday 15 May 2007, Grant Likely wrote:
> 
> Take another look; this is a funny quirk of the driver.  The
> assignment is 'i2c->adap = mpc_ops'; not 'i2c->adap = &mpc_ops'.  And
> in struct mpc_i2c, the field is declared as 'struct i2c_adapter adap',
> not 'struct i2c_adapter *adap'.  The driver instance gets a copy of
> the mpc_ops structure to initialize it, not a pointer to the staticly
> defined structure.  I got bitten by the same thing when I was looking
> at the code.

I see -- you're right.  That "template" idiom is a good one to get
rid of, FWIW ... not only is it confusing, but it also wastes space.

- Dave

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Jean Delvare]

On Tue, 15 May 2007 09:05:20 -0700, David Brownell wrote:
> On Tuesday 15 May 2007, Grant Likely wrote:
> > 
> > Take another look; this is a funny quirk of the driver.  The
> > assignment is 'i2c->adap = mpc_ops'; not 'i2c->adap = &mpc_ops'.  And
> > in struct mpc_i2c, the field is declared as 'struct i2c_adapter adap',
> > not 'struct i2c_adapter *adap'.  The driver instance gets a copy of
> > the mpc_ops structure to initialize it, not a pointer to the staticly
> > defined structure.  I got bitten by the same thing when I was looking
> > at the code.
> 
> I see -- you're right.  That "template" idiom is a good one to get
> rid of, FWIW ... not only is it confusing, but it also wastes space.

Well, maybe you can submit a patch fixing this one?

-- 
Jean Delvare

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Grant Likely]

On 5/16/07, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 15 May 2007 09:05:20 -0700, David Brownell wrote:
> > On Tuesday 15 May 2007, Grant Likely wrote:
> > >
> > > Take another look; this is a funny quirk of the driver.  The
> > > assignment is 'i2c->adap = mpc_ops'; not 'i2c->adap = &mpc_ops'.  And
> > > in struct mpc_i2c, the field is declared as 'struct i2c_adapter adap',
> > > not 'struct i2c_adapter *adap'.  The driver instance gets a copy of
> > > the mpc_ops structure to initialize it, not a pointer to the staticly
> > > defined structure.  I got bitten by the same thing when I was looking
> > > at the code.
> >
> > I see -- you're right.  That "template" idiom is a good one to get
> > rid of, FWIW ... not only is it confusing, but it also wastes space.
>
> Well, maybe you can submit a patch fixing this one?

Heh, oops.

I had replied to David saying that I would do this bit; but I forgot
to cc the list.

I'll take care of this.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [i2c] [PATCH] Make i2c-mpc driver use i2c_add_numbered_adapter

[Scott Wood]

David Brownell wrote:
> On Tuesday 15 May 2007, Jean Delvare wrote:
> 
>>On Mon, 14 May 2007 13:11:23 -0600, Grant Likely wrote:
> 
> 
>>>--- a/drivers/i2c/busses/i2c-mpc.c
>>>+++ b/drivers/i2c/busses/i2c-mpc.c
>>>@@ -327,9 +327,10 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>>> 	platform_set_drvdata(pdev, i2c);
>>> 
>>> 	i2c->adap = mpc_ops;
>>>+	i2c->adap.nr = pdev->id;
> 
> 
> By the way:  mpc_ops is a static i2c_adapter, so given that
> the reason for using pdev->id that way was that there might
> be more than one such platform device ... shouldn't allocation
> of the adapter be moved into allocation of the "i2c->" object?
> 
> Or at least, add a check to ensure that the static mpc_ops
> structure isn't in use before progressing this probe().

The mpc_ops struct is only used as a template; it gets copied into 
i2c->adap for each adapter instance.

-Scott