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

[Scott Wood <scottwood@freescale.com>]

Jon Smirl wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
>> 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).
> 
> How about renaming the old driver file and leaving it hooked to ppc?
> Then it would get deleted when ppc goes away. That would let work
> progress on the powerpc version.

Or we could have one driver that has two probe methods.  I don't like 
forking the driver.

> I'm working on this because I'm adding codecs under powerpc that use
> i2c and I want consistent device tree use.

We already support i2c clients in the device tree, via the code in 
fsl_soc.c.

>>>       cell-index = <1>;
>> What is cell-index for?
> 
> I was using it to control the bus number, is that the wrong attribute?

It shouldn't be specified at all -- the hardware has no concept of a 
device number.

>>>       rtc@32 {
>>>               device_type = "rtc";
>> This isn't really necessary.
> 
> Code doesn't access it. I copied it from a pre-existing device tree.

I'm just trying to keep the damage from spreading. :-)

>>> One side effect is that legacy style i2c drivers won't work anymore.
>> If you mean legacy-style client drivers, why not?
> 
> i2c_new_device() doesn't work with legacy-style client drivers.

No, but they should still work the old way.

>>> 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.
> 
> This is not hard to do but the i2c people will have to agree. I need
> to change the i2c_driver structure to include the additional names.

I got a fair bit of resistance from them on the topic of multiple match 
names for i2c clients.

>> Most of this code should be made generic and placed in drivers/of.
> 
> How so, it is specific to adding i2c drivers.

I meant generic with respect to the type of i2c controller, not generic 
to all device types. :-)

It could be drivers/of/i2c.c, or drivers/i2c/of.c, etc.

>> 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.
> 
> I was allowing control of the bus number with "cell-index" and
> i2c_add_numbered_adapter().
> Should I get rid of this and switch to i2c_add_adapter()?

Yes.

>>> +     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().
> 
> I didn't write this, how should it be done? MPC_I2C_REGION can be
> eliminated by using mem.start - mem.end.

i2c->base = of_iomap(op->node, 0);

of_address_to_resource() and ioremap() are combined into one.

>> Let's take this opportunity to stop matching on device_type here
>> (including updating booting-without-of.txt).
> 
> Remove the .type line and leave .compatible?

Yes.

>>> +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?
> 
> Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
> to be changed to use mpc_i2c_driver.driver.name.

How can i2c-core be matching on a field in an OF-specific struct?

-Scott