Re: [PATCH v6] gpio: Add support for Intel ICHx/3100/Series[56]GPIO

[Peter Tyser]

On Fri, 2011-05-27 at 17:54 -0600, Grant Likely wrote:
> On Fri, May 27, 2011 at 3:29 PM, Peter Tyser <ptyser@xxxxxxxxxxx> wrote:
> >> > My own (completely biased) preference would be to use my driver as a
> >> > starting point, primarily because it supports newer chips that require
> >> > different access methods, as well as the older-style chips Jean's
> >> > supports.  Its also been hanging out for about 6 months without many
> >> > complaints, a tested-by, etc.
> >>
> >> I like the /structure/ of Jean's driver better, particularly that it doesn't
> >> play games with the device model by registering devices at module load
> >> time.
> >
> > But the driver shouldn't be a PCI driver - that's worse in my opinion
> > than a platform driver.  Multiple drivers currently use the same PCI
> > device ID as Jean's - they can't all claim the PCI device, so why should
> > Jean's GPIO driver?  It seems incorrect, and likely to break things in
> > the future.  Mine has its warts too, but it doesn't have the possibility
> > of negatively affect other drivers at least, as well as supports more
> > hardware.
> 
> Does it conflict with other drivers in the kernel right now?  If so,
> then I'll drop it.

It'd take some searching of where all the PCI vendor IDs in the new
patch are used.  I did a quick search of ICH7 and ESB2 IDs, and it
looked like:
Conflict:
- drivers/leds/leds-ss4200.c (also uses a PCI driver, also uses the GPIO
functionality of the chip)

Uses the same device, but no conflict:
- drivers/char/sonypi.c (uses platform driver)
- drivers/watchdog/iTCO_wdt.c (uses platform driver)
- drivers/mtd/maps/esb2rom.c (has PCI driver support commented out)

The same search would have to be performed for the other IDs to
determine if there are conflicts.  So there is one conflict, but its
probably not a huge deal because someone using leds-ss4200.c likely
wouldn't be using the ich GPIO driver as well.  leds-ss4200.c should
probably be converted to use the standard GPIO driver in the future.

However, if the above non-conflicting drivers did use the same PCI
driver model as the accepted GPIO patch, there would be conflicts -
that's what I don't care for.  The drivers above are considerate of the
fact that the device being used is a MFD while the Jean's driver is not.

I followed the iTCO_wdt.c and sonypi.c models for my driver in an
attempt to play nice with other drivers that might require the same PCI
device.

> Also, you're directly describing the use case for an MFD type device.
> I certainly expect either this driver or Jean's driver to be reworked
> into an MFD style device in the next cycle.

I was following the very "popular" iTCO_wdt.c driver model.  I'd hope
the current driver could be merged as it follows the current ICHx usage
pattern, and then they (and possibly other drivers) could all be
reworked at the same time into an MFD style driver.

> >> If one device performs more than one function, then it should
> >> register all the interfaces from a single probe, or go in the
> >> direction of MFD devices and register a bunch of child
> >> platform_devices.
> >
> > In this case its not really "one device performing more than one
> > function", its one device specifying the location of other devices.  The
> > device being claimed is used for FWH and IRQ configuration and happens
> > to have a pointer to a GPIO device (among other things).  Claiming it
> > for a GPIO device seems very wrong - it has very little to do with GPIO
> > at all.
> 
> Are you sure?  My reading of the ICH10 manual is that the GPIO
> controller is very much behind the LPC bridge, and that it is entirely
> appropriate for the GPIO controller device to be registered by the PCI
> driver as a child of the PCI device, or to be registered directly by
> the PCI driver.

I was a bit overzealous in my comment - you are correct that the GPIO
controller is behind the LPC bridge.  However, there is a lot of other
stuff in the same device, from the Intel series 5 datasheet : "The LPC
bridge function of the PCH resides in PCI Device 31:Function 0. This
function contains many other functional units, such as DMA and Interrupt
controllers, Timers, Power Management, System Management, GPIO, RTC, and
LPC Configuration Registers."
 
Thus I still don't think its correct to claim the device as only a GPIO
device - its much more, as can be seen by the current drivers which use
the same device.  I'm having a tough time seeing how an monopolistic PCI
driver is considered better than a platform driver that follows the
structure of other drivers in use.

> >
> >> I'm going to merge Jean's driver and leave this one out for now
> >> (sorry), but I reserve the right to replace Jean's with your driver in
> >> the next merge window.  This is pretty much an arbitrary decision, but
> >> I may as well merge at least one of them now instead of kicking both
> >> out for another cycle.  It really seams to me like there is still
> >> a few architectural issues to sort out in both drivers and to make
> >> sure that one driver will be useful for both of you.
> >
> > I disagree with the logic, and its a bit frustrating to have a patch
> > hanging out for 6 months to be superseded by a more recent change that
> > seems to be more likely to cause conflicts.
> 
> Think about it this way; your patch wasn't going to go in this cycle
> anyway due to the current concerns I have about structure,

I attempted to follow the driver model of the existing iTCO_wdt.c file,
which at a minimum will not negatively affect other drivers unlike the
potential of the accepted driver.  Any comments about the platform
driver GPIO approach can also be made to the iTCO_wdt.c driver, as well
as the sonypi.c driver.  Since cleanup needs to be done anyway on those
drivers, my hope would be that the platform GPIO driver would be
accepted since it follows the status quo, and then cleanup on them all
would occur.

Best,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/