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

[Peter Tyser]

On Fri, 2011-05-27 at 11:01 +0200, Jean Delvare wrote:
> Hi Grant,
> 
> On Fri, 27 May 2011 00:42:38 -0600, Grant Likely wrote:
> > On Wed, Apr 20, 2011 at 11:35:54AM -0500, Peter Tyser wrote:
> > > This driver works on many Intel chipsets, including the ICH6, ICH7,
> > > ICH8, ICH9, ICH10, 3100, Series 5/3400 (Ibex Peak), Series 6/C200
> > > (Cougar Point), and NM10 (Tiger Point).
> > > 
> > > Additional Intel chipsets should be easily supported if needed, eg the
> > > ICH1-5, EP80579, etc.
> > > 
> > > Tested on a QM57 (Ibex Peak), 3100 (Whitmore Lake) , and
> > > NM10 (Tiger Point).
> > > 
> > > Cc: Alek Du <alek.du@xxxxxxxxx>
> > > Cc: Samuel Ortiz <sameo@xxxxxxxxxxxxxxx>
> > > Cc: Eric Miao <eric.y.miao@xxxxxxxxx>
> > > Cc: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > Cc: Joe Perches <joe@xxxxxxxxxxx>
> > > Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
> > > Cc: Syed S Azam <Syed.Azam@xxxxxx>
> > > Signed-off-by: Peter Tyser <ptyser@xxxxxxxxxxx>
> > > Signed-off-by: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
> > > Tested-by: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
> > 
> > Hmmm, I merged a patch from Jean Delvare adding support for Intel
> > 82801 gpio pins[1].  Does this driver support the same hardware?  I see
> > the same PCI ids.
> > 
> > [1] https://lkml.org/lkml/2011/4/19/170
> 
> There is indeed a common range in the supported devices: ICH6 to ICH10.
> My driver also supports older ICH chips (ICH to ICH5), while Peter's
> support newer devices my driver does not (basically everything after
> the ICH10).
> 
> Another key difference is that my driver is a simple PCI driver, while
> Peter's is a platform driver. It makes some sense to have a platform
> driver because the PCI device is a multifunction device so other
> drivers may want to bind to it. However, I suspect that the other
> functions (ACPI?) will never need a driver (not in the Linux device
> driver binding model sense of the term at least) which is why I did not
> bother. Peter, what was you reason to go for a platform driver? If you
> really want to it go that route, you'll have to follow the standard MFD
> model (see drivers/mfd/lpc_sch.c for an example.)
> 
> The only device I really care to see supported at the moment is the
> ICH10, and it is supported by both drivers, so I don't care too much
> which driver is picked.

I went with a platform driver because the PCI device IDs we're searching
for don't contain the "GPIO registers" - they just have pointer to the
GPIO registers which reside in IO space, in addition to other non-GPIO
registers as Jean mentioned.  So conceptually a platform driver made
more sense to me since we're not claiming or using the PCI device, we're
just using it to get a pointer.

More importantly, the PCI device that contains the pointer to the GPIO
IO space also has other uses that prevent it from being claimed.  Eg the
drivers/mtd/maps/esb2rom.c driver uses the same PCI device to configure
firmware hub registers.  The esb2rom.c driver isn't a PCI driver either,
so it wouldn't cause a conflict, but it seemed to support the idea that
we shouldn't be using a PCI driver.

The drivers/wdt/iTCO_wdt.c file also uses the same PCI device to get the
ACPI base (similar to us getting the GPIO base), and it is implemented
as a platform driver.

What determines if the driver should use the mfd or platform model?  Eg
the iTCO_wdt.c that I used as an example doesn't use the mfd model
despite using the PCI device the same as we are.

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.

Best,
Peter

* The original work was done on an Intel 3100 with PCI device ID
PCI_DEVICE_ID_INTEL_ESB2_0, datasheet at
http://download.intel.com/design/intarch/datashts/31345803.pdf, see
section 16.2.


--
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/