On Mon, Sep 17, 2007 at 07:34:08AM +0200, Stefan Roese wrote:
> On Sunday 16 September 2007, Eugene Surovegin wrote:
> > Hmm, I just noticed that you basically added a copy of existing
> > driver with small changes to support OF while keeping OCP one.
> >
> > Why not just add OF support to the existing code (under some ifdef),
> > and then remove OCP support as soon as ppc -> powerpc transition is
> > finished? Why have two almost identical code in the tree?
>
> My understanding was, that adding many #ifdef's into the code was not the
> preferred way. I could of course change this patch to not add an additional
> driver but extend the existing driver with a bunch of #ifdef's to support
> both versions.
#ifdefs are yucky, but so is duplication. I'm not sure which is the
lesser evil in this case.
> This approach of multiple drivers seems to be common in the kernel right now:
>
> drivers/mtd/maps/physmap.c
> drivers/mtd/maps/physmap_of.c
Not a relevant example. Despite the names, physmap and physmap_of
don't really do the same thing at all. I've been meaning to rename
physmap_of...
>
> or
>
> drivers/usb/host/ohci-ppc-soc.c
> drivers/usb/host/ohci-ppc-of.c
Also ibm_emac vs. ibm_new_emac (not merged yet).
> Any other opinions on this? How should this be handled to get accepted
> upstream? Two different drivers with removing the "old" one later when
> arch/ppc is gone, or one driver which supports both versions and removing the
> ocp support in this driver later?
>
> > I also personally don't like this _iic -> _of name change (you
> > removed peripheral name and added something which has nothing to do
> > with iic, I never heard of OF peripheral in 4xx chips). Whether you
> > use OCP or OF to pass a little information is quite irrelevant to the
> > iic driver operation.
>
> The "old" name "i2c-ibm_iic" is kind of redundant. Nearly all bus drivers are
> named "i2c-platform". Perhaps a better name would be "i2c-ppc4xx" then.
> This "of" name was borrowed from already existing device-tree aware drivers
> like drivers/mtd/maps/physmap_of.c or drivers/usb/host/ohci-ppc-of.c.
'ibm' is not specific enough - it's not like it's used on even a very
large fraction of ibm platforms - and 'of' is verging on misleading
(since OF != device tree, although they're related). 'iic' isn't
arbitrary - it comes from the name used in the documentation.
Although 'i2c-ppc4xx' probably is a better name, in any case.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
