From: Guenter Roeck <guenter.roeck@ericsson.com>
To: Aaron Sierra <asierra@xes-inc.com>
Cc: Jean Delvare <khali@linux-fr.org>,
Grant Likely <grant.likely@secretlab.ca>,
LKML <linux-kernel@vger.kernel.org>,
Peter Tyser <ptyser@xes-inc.com>
Subject: Re: [PATCH 1/3] mfd: Add LPC driver for Intel ICH chipsets
Date: Fri, 3 Feb 2012 11:14:22 -0800 [thread overview]
Message-ID: <1328296462.2261.193.camel@groeck-laptop> (raw)
In-Reply-To: <484bd1ed-f456-40b5-b5c2-c0e50f2787d5@zimbra>
On Thu, 2012-02-02 at 18:25 -0500, Aaron Sierra wrote:
> This driver currently creates resources for use by a forthcoming ICH
> chipset GPIO driver. It could be expanded to created the resources for
> converting the esb2rom (mtd) and iTCO_wdt (wdt), and potentially more,
> drivers to use the mfd model.
>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
Hi Aaron,
looks pretty good. Couple of comments below.
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/lpc_ich.c | 531 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/lpc_ich.h | 33 +++
> 4 files changed, 574 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/lpc_ich.c
> create mode 100644 include/linux/mfd/lpc_ich.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6ca938a..18eca82 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -636,6 +636,15 @@ config LPC_SCH
> LPC bridge function of the Intel SCH provides support for
> System Management Bus and General Purpose I/O.
>
> +config LPC_ICH
> + tristate "Intel ICH LPC"
> + depends on PCI
> + select MFD_CORE
> + help
> + The LPC bridge function of the Intel ICH provides support for
> + many functional units. This driver provides needed support for
> + other drivers to control these functions, currently GPIO.
> +
> config MFD_RDC321X
> tristate "Support for RDC-R321x southbridge"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d7d47d2..d479882 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_MFD_DB5500_PRCMU) += db5500-prcmu.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> +obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
> obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> new file mode 100644
> index 0000000..a288c74
> --- /dev/null
> +++ b/drivers/mfd/lpc_ich.c
> @@ -0,0 +1,531 @@
> +/*
> + * lpc_ich.c - LPC interface for Intel ICH
> + *
> + * LPC bridge function of the Intel ICH contains many other
> + * functional units, such as Interrupt controllers, Timers,
> + * Power Management, System Management, GPIO, RTC, and LPC
> + * Configuration Registers.
> + *
> + * This driver is derived from lpc_sch.
> +
> + * Copyright (c) 2011 Extreme Engineering Solution, Inc.
> + * Author: Aaron Sierra <asierra@xes-inc.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; see the file COPYING. If not, write to
> + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
It might make sense to list the Intel document numbers, like iTCO_wdt.c.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/acpi.h>
> +#include <linux/pci.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/lpc_ich.h>
> +
> +/* Convenient wrapper to make our PCI ID table */
> +#define ICHX_PDEV(dev, idx) \
> + .vendor = PCI_VENDOR_ID_INTEL, \
> + .device = dev, \
> + .subvendor = PCI_ANY_ID, \
> + .subdevice = PCI_ANY_ID, \
> + .class = 0, \
> + .class_mask = 0, \
> + .driver_data = idx
> +
Later kernel versions have a macro PCI_VDEVICE which can be used instead
(and is now used by iTCO_wdt.c).
> +#define ACPIBASE 0x40
> +#define ACPIBASE_GPE_OFF 0x20
> +#define ACPIBASE_GPE_END 0x2f
> +#define ACPICTRL 0x44
> +
> +#define GPIOBASE 0x48
> +#define GPIOCTRL 0x4C
> +#define GPIOBASE_IO_SIZE 0x80
> +
> +static u8 lpc_ich_acpi_save, lpc_ich_gpio_save;
> +
> +static struct resource gpio_ich_res[] = {
> + /* BASE */
> + {
> + .flags = IORESOURCE_IO,
> + },
> + /* ACPI */
> + {
> + .flags = IORESOURCE_IO,
> + },
> +};
> +
> +static struct mfd_cell lpc_ich_cells[] = {
> + {
> + .name = "ich_gpio",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
> + },
> +};
> +
> +/* TCO related info */
s/TCO/chipset/
> +enum lpc_chipsets {
> + LPC_ICH = 0, /* ICH */
> + LPC_ICH0, /* ICH0 */
> + LPC_ICH2, /* ICH2 */
> + LPC_ICH2M, /* ICH2-M */
> + LPC_ICH3, /* ICH3-S */
> + LPC_ICH3M, /* ICH3-M */
> + LPC_ICH4, /* ICH4 */
> + LPC_ICH4M, /* ICH4-M */
> + LPC_CICH, /* C-ICH */
> + LPC_ICH5, /* ICH5 & ICH5R */
> + LPC_6300ESB, /* 6300ESB */
> + LPC_ICH6, /* ICH6 & ICH6R */
> + LPC_ICH6M, /* ICH6-M */
> + LPC_ICH6W, /* ICH6W & ICH6RW */
> + LPC_631XESB, /* 631xESB/632xESB */
> + LPC_ICH7, /* ICH7 & ICH7R */
> + LPC_ICH7DH, /* ICH7DH */
> + LPC_ICH7M, /* ICH7-M & ICH7-U */
> + LPC_ICH7MDH, /* ICH7-M DH */
> + LPC_NM10, /* NM10 */
> + LPC_ICH8, /* ICH8 & ICH8R */
> + LPC_ICH8DH, /* ICH8DH */
> + LPC_ICH8DO, /* ICH8DO */
> + LPC_ICH8M, /* ICH8M */
> + LPC_ICH8ME, /* ICH8M-E */
> + LPC_ICH9, /* ICH9 */
> + LPC_ICH9R, /* ICH9R */
> + LPC_ICH9DH, /* ICH9DH */
> + LPC_ICH9DO, /* ICH9DO */
> + LPC_ICH9M, /* ICH9M */
> + LPC_ICH9ME, /* ICH9M-E */
> + LPC_ICH10, /* ICH10 */
> + LPC_ICH10R, /* ICH10R */
> + LPC_ICH10D, /* ICH10D */
> + LPC_ICH10DO, /* ICH10DO */
> + LPC_PCH, /* PCH Desktop Full Featured */
> + LPC_PCHM, /* PCH Mobile Full Featured */
> + LPC_P55, /* P55 */
> + LPC_PM55, /* PM55 */
> + LPC_H55, /* H55 */
> + LPC_QM57, /* QM57 */
> + LPC_H57, /* H57 */
> + LPC_HM55, /* HM55 */
> + LPC_Q57, /* Q57 */
> + LPC_HM57, /* HM57 */
> + LPC_PCHMSFF, /* PCH Mobile SFF Full Featured */
> + LPC_QS57, /* QS57 */
> + LPC_3400, /* 3400 */
> + LPC_3420, /* 3420 */
> + LPC_3450, /* 3450 */
> + LPC_EP80579, /* EP80579 */
> + LPC_CPT1, /* Cougar Point */
> + LPC_CPT2, /* Cougar Point Desktop */
> + LPC_CPT3, /* Cougar Point Mobile */
> + LPC_CPT4, /* Cougar Point */
> + LPC_CPT5, /* Cougar Point */
> + LPC_CPT6, /* Cougar Point */
> + LPC_CPT7, /* Cougar Point */
> + LPC_CPT8, /* Cougar Point */
> + LPC_CPT9, /* Cougar Point */
> + LPC_CPT10, /* Cougar Point */
> + LPC_CPT11, /* Cougar Point */
> + LPC_CPT12, /* Cougar Point */
> + LPC_CPT13, /* Cougar Point */
> + LPC_CPT14, /* Cougar Point */
> + LPC_CPT15, /* Cougar Point */
> + LPC_CPT16, /* Cougar Point */
> + LPC_CPT17, /* Cougar Point */
> + LPC_CPT18, /* Cougar Point */
> + LPC_CPT19, /* Cougar Point */
> + LPC_CPT20, /* Cougar Point */
> + LPC_CPT21, /* Cougar Point */
> + LPC_CPT22, /* Cougar Point */
> + LPC_CPT23, /* Cougar Point */
> + LPC_CPT24, /* Cougar Point */
> + LPC_CPT25, /* Cougar Point */
> + LPC_CPT26, /* Cougar Point */
> + LPC_CPT27, /* Cougar Point */
> + LPC_CPT28, /* Cougar Point */
> + LPC_CPT29, /* Cougar Point */
> + LPC_CPT30, /* Cougar Point */
> + LPC_CPT31, /* Cougar Point */
> + LPC_PBG1, /* Patsburg */
> + LPC_PBG2, /* Patsburg */
> + LPC_DH89XXCC, /* DH89xxCC */
> + LPC_PPT0, /* Panther Point */
> + LPC_PPT1, /* Panther Point */
> + LPC_PPT2, /* Panther Point */
> + LPC_PPT3, /* Panther Point */
> + LPC_PPT4, /* Panther Point */
> + LPC_PPT5, /* Panther Point */
> + LPC_PPT6, /* Panther Point */
> + LPC_PPT7, /* Panther Point */
> + LPC_PPT8, /* Panther Point */
> + LPC_PPT9, /* Panther Point */
> + LPC_PPT10, /* Panther Point */
> + LPC_PPT11, /* Panther Point */
> + LPC_PPT12, /* Panther Point */
> + LPC_PPT13, /* Panther Point */
> + LPC_PPT14, /* Panther Point */
> + LPC_PPT15, /* Panther Point */
> + LPC_PPT16, /* Panther Point */
> + LPC_PPT17, /* Panther Point */
> + LPC_PPT18, /* Panther Point */
> + LPC_PPT19, /* Panther Point */
> + LPC_PPT20, /* Panther Point */
> + LPC_PPT21, /* Panther Point */
> + LPC_PPT22, /* Panther Point */
> + LPC_PPT23, /* Panther Point */
> + LPC_PPT24, /* Panther Point */
> + LPC_PPT25, /* Panther Point */
> + LPC_PPT26, /* Panther Point */
> + LPC_PPT27, /* Panther Point */
> + LPC_PPT28, /* Panther Point */
> + LPC_PPT29, /* Panther Point */
> + LPC_PPT30, /* Panther Point */
> + LPC_PPT31, /* Panther Point */
> +};
> +
I was always irritated by those duplicate defines and array entries in
iTCO_wdt.c. Apparently someone else too, because it is gone in the
latest version of iTCO_wdt.c. Should do the same here.
> +struct lpc_ich_info lpc_chipset_info[] __devinitdata = {
__devinitdata is not a good idea for a to-be-exported data structure.
Though it does not need to be exported (see below), so that is really a
moot point.
> + {"ICH", 0},
> + {"ICH0", 0},
> + {"ICH2", 0},
> + {"ICH2-M", 0},
> + {"ICH3-S", 0},
> + {"ICH3-M", 0},
> + {"ICH4", 0},
> + {"ICH4-M", 0},
> + {"C-ICH", 0},
> + {"ICH5 or ICH5R", 0},
> + {"6300ESB", 0},
> + {"ICH6 or ICH6R", 0x0601},
> + {"ICH6-M", 0x0601},
> + {"ICH6W or ICH6RW", 0x0601},
> + {"631xESB/632xESB", 0x0601},
> + {"ICH7 or ICH7R", 0x0701},
> + {"ICH7DH", 0x0701},
> + {"ICH7-M or ICH7-U", 0x0701},
> + {"ICH7-M DH", 0x0701},
> + {"NM10", 0},
> + {"ICH8 or ICH8R", 0x0701},
> + {"ICH8DH", 0x0701},
> + {"ICH8DO", 0x0701},
> + {"ICH8M", 0x0701},
> + {"ICH8M-E", 0x0701},
> + {"ICH9", 0x0801},
> + {"ICH9R", 0x0801},
> + {"ICH9DH", 0x0801},
> + {"ICH9DO", 0x0801},
> + {"ICH9M", 0x0801},
> + {"ICH9M-E", 0x0801},
> + {"ICH10", 0x0a11},
> + {"ICH10R", 0x0a11},
> + {"ICH10D", 0x0a01},
> + {"ICH10DO", 0x0a01},
> + {"PCH Desktop Full Featured", 0x0501},
> + {"PCH Mobile Full Featured", 0x0501},
> + {"P55", 0x0501},
> + {"PM55", 0x0501},
> + {"H55", 0x0501},
> + {"QM57", 0x0501},
> + {"H57", 0x0501},
> + {"HM55", 0x0501},
> + {"Q57", 0x0501},
> + {"HM57", 0x0501},
> + {"PCH Mobile SFF Full Featured", 0x0501},
> + {"QS57", 0x0501},
> + {"3400", 0x0501},
> + {"3420", 0x0501},
> + {"3450", 0x0501},
> + {"EP80579", 0},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Cougar Point", 0x0501},
> + {"Patsburg", 0},
> + {"Patsburg", 0},
> + {"DH89xxCC", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {"Panther Point", 0},
> + {NULL, 0}
The last (empty) entry is not needed here, since the table is indexed by
enum lpc_chipsets. Also, it might be better to use explicit
initialization such as
[LPC_PPT] = {"Panther Point", 0},
since that would ensure that entries can not get out of sync.
> +};
> +EXPORT_SYMBOL(lpc_chipset_info);
> +
No need to export the table. See below.
> +static DEFINE_PCI_DEVICE_TABLE(lpc_ich_ids) = {
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801AA_0, LPC_ICH)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801AB_0, LPC_ICH0)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801BA_0, LPC_ICH2)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801BA_10, LPC_ICH2M)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801CA_0, LPC_ICH3)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801CA_12, LPC_ICH3M)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801DB_0, LPC_ICH4)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801DB_12, LPC_ICH4M)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801E_0, LPC_CICH)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_82801EB_0, LPC_ICH5)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ESB_1, LPC_6300ESB)},
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH6_0, LPC_ICH6) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH6_1, LPC_ICH6M) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH6_2, LPC_ICH6W) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ESB2_0, LPC_631XESB) },
The latest version if iTCO_wdt.c got rid of the symbolic defines. Maybe
follow its lead.
> + { ICHX_PDEV(0x2671, LPC_631XESB) },
> + { ICHX_PDEV(0x2672, LPC_631XESB) },
> + { ICHX_PDEV(0x2673, LPC_631XESB) },
> + { ICHX_PDEV(0x2674, LPC_631XESB) },
> + { ICHX_PDEV(0x2675, LPC_631XESB) },
> + { ICHX_PDEV(0x2676, LPC_631XESB) },
> + { ICHX_PDEV(0x2677, LPC_631XESB) },
> + { ICHX_PDEV(0x2678, LPC_631XESB) },
> + { ICHX_PDEV(0x2679, LPC_631XESB) },
> + { ICHX_PDEV(0x267a, LPC_631XESB) },
> + { ICHX_PDEV(0x267b, LPC_631XESB) },
> + { ICHX_PDEV(0x267c, LPC_631XESB) },
> + { ICHX_PDEV(0x267d, LPC_631XESB) },
> + { ICHX_PDEV(0x267e, LPC_631XESB) },
> + { ICHX_PDEV(0x267f, LPC_631XESB) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH7_0, LPC_ICH7) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH7_30, LPC_ICH7DH) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH7_1, LPC_ICH7M) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH7_31, LPC_ICH7MDH) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_TGP_LPC, LPC_NM10) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH8_0, LPC_ICH8) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH8_2, LPC_ICH8DH) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH8_3, LPC_ICH8DO) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH8_4, LPC_ICH8M) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH8_1, LPC_ICH8ME) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_8, LPC_ICH9) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_7, LPC_ICH9R) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_2, LPC_ICH9DH) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_4, LPC_ICH9DO) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_5, LPC_ICH9M) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH9_1, LPC_ICH9ME) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH10_2, LPC_ICH10) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH10_1, LPC_ICH10R) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH10_3, LPC_ICH10D) },
> + { ICHX_PDEV(PCI_DEVICE_ID_INTEL_ICH10_0, LPC_ICH10DO) },
> + { ICHX_PDEV(0x3b00, LPC_PCH) },
> + { ICHX_PDEV(0x3b01, LPC_PCHM) },
> + { ICHX_PDEV(0x3b02, LPC_P55) },
> + { ICHX_PDEV(0x3b03, LPC_PM55) },
> + { ICHX_PDEV(0x3b06, LPC_H55) },
> + { ICHX_PDEV(0x3b07, LPC_QM57) },
> + { ICHX_PDEV(0x3b08, LPC_H57) },
> + { ICHX_PDEV(0x3b09, LPC_HM55) },
> + { ICHX_PDEV(0x3b0a, LPC_Q57) },
> + { ICHX_PDEV(0x3b0b, LPC_HM57) },
> + { ICHX_PDEV(0x3b0d, LPC_PCHMSFF) },
> + { ICHX_PDEV(0x3b0f, LPC_QS57) },
> + { ICHX_PDEV(0x3b12, LPC_3400) },
> + { ICHX_PDEV(0x3b14, LPC_3420) },
> + { ICHX_PDEV(0x3b16, LPC_3450) },
> + { ICHX_PDEV(0x5031, LPC_EP80579) },
> + { ICHX_PDEV(0x1c41, LPC_CPT1) },
> + { ICHX_PDEV(0x1c42, LPC_CPT2) },
> + { ICHX_PDEV(0x1c43, LPC_CPT3) },
> + { ICHX_PDEV(0x1c44, LPC_CPT4) },
> + { ICHX_PDEV(0x1c45, LPC_CPT5) },
> + { ICHX_PDEV(0x1c46, LPC_CPT6) },
> + { ICHX_PDEV(0x1c47, LPC_CPT7) },
> + { ICHX_PDEV(0x1c48, LPC_CPT8) },
> + { ICHX_PDEV(0x1c49, LPC_CPT9) },
> + { ICHX_PDEV(0x1c4a, LPC_CPT10) },
> + { ICHX_PDEV(0x1c4b, LPC_CPT11) },
> + { ICHX_PDEV(0x1c4c, LPC_CPT12) },
> + { ICHX_PDEV(0x1c4d, LPC_CPT13) },
> + { ICHX_PDEV(0x1c4e, LPC_CPT14) },
> + { ICHX_PDEV(0x1c4f, LPC_CPT15) },
> + { ICHX_PDEV(0x1c50, LPC_CPT16) },
> + { ICHX_PDEV(0x1c51, LPC_CPT17) },
> + { ICHX_PDEV(0x1c52, LPC_CPT18) },
> + { ICHX_PDEV(0x1c53, LPC_CPT19) },
> + { ICHX_PDEV(0x1c54, LPC_CPT20) },
> + { ICHX_PDEV(0x1c55, LPC_CPT21) },
> + { ICHX_PDEV(0x1c56, LPC_CPT22) },
> + { ICHX_PDEV(0x1c57, LPC_CPT23) },
> + { ICHX_PDEV(0x1c58, LPC_CPT24) },
> + { ICHX_PDEV(0x1c59, LPC_CPT25) },
> + { ICHX_PDEV(0x1c5a, LPC_CPT26) },
> + { ICHX_PDEV(0x1c5b, LPC_CPT27) },
> + { ICHX_PDEV(0x1c5c, LPC_CPT28) },
> + { ICHX_PDEV(0x1c5d, LPC_CPT29) },
> + { ICHX_PDEV(0x1c5e, LPC_CPT30) },
> + { ICHX_PDEV(0x1c5f, LPC_CPT31) },
> + { ICHX_PDEV(0x1d40, LPC_PBG1) },
> + { ICHX_PDEV(0x1d41, LPC_PBG2) },
> + { ICHX_PDEV(0x2310, LPC_DH89XXCC) },
> + { ICHX_PDEV(0x1e40, LPC_PPT0) },
> + { ICHX_PDEV(0x1e41, LPC_PPT1) },
> + { ICHX_PDEV(0x1e42, LPC_PPT2) },
> + { ICHX_PDEV(0x1e43, LPC_PPT3) },
> + { ICHX_PDEV(0x1e44, LPC_PPT4) },
> + { ICHX_PDEV(0x1e45, LPC_PPT5) },
> + { ICHX_PDEV(0x1e46, LPC_PPT6) },
> + { ICHX_PDEV(0x1e47, LPC_PPT7) },
> + { ICHX_PDEV(0x1e48, LPC_PPT8) },
> + { ICHX_PDEV(0x1e49, LPC_PPT9) },
> + { ICHX_PDEV(0x1e4a, LPC_PPT10) },
> + { ICHX_PDEV(0x1e4b, LPC_PPT11) },
> + { ICHX_PDEV(0x1e4c, LPC_PPT12) },
> + { ICHX_PDEV(0x1e4d, LPC_PPT13) },
> + { ICHX_PDEV(0x1e4e, LPC_PPT14) },
> + { ICHX_PDEV(0x1e4f, LPC_PPT15) },
> + { ICHX_PDEV(0x1e50, LPC_PPT16) },
> + { ICHX_PDEV(0x1e51, LPC_PPT17) },
> + { ICHX_PDEV(0x1e52, LPC_PPT18) },
> + { ICHX_PDEV(0x1e53, LPC_PPT19) },
> + { ICHX_PDEV(0x1e54, LPC_PPT20) },
> + { ICHX_PDEV(0x1e55, LPC_PPT21) },
> + { ICHX_PDEV(0x1e56, LPC_PPT22) },
> + { ICHX_PDEV(0x1e57, LPC_PPT23) },
> + { ICHX_PDEV(0x1e58, LPC_PPT24) },
> + { ICHX_PDEV(0x1e59, LPC_PPT25) },
> + { ICHX_PDEV(0x1e5a, LPC_PPT26) },
> + { ICHX_PDEV(0x1e5b, LPC_PPT27) },
> + { ICHX_PDEV(0x1e5c, LPC_PPT28) },
> + { ICHX_PDEV(0x1e5d, LPC_PPT29) },
> + { ICHX_PDEV(0x1e5e, LPC_PPT30) },
> + { ICHX_PDEV(0x1e5f, LPC_PPT31) },
ITCO_wdt.c now also supports Lynx Point.
> + { 0, },
> +};
> +MODULE_DEVICE_TABLE(pci, lpc_ich_ids);
> +
> +static int __devinit lpc_ich_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + u32 base_addr_cfg;
> + u32 base_addr;
> + int i;
> +
> + /* Setup power management base register */
> + pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
> + base_addr = base_addr_cfg & 0x0000ff80;
> + if (!base_addr) {
> + dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
> + return -ENODEV;
> + }
> +
> + gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
> + gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
> +
> + /* Enable LPC ACPI space */
> + pci_read_config_byte(dev, ACPICTRL, &lpc_ich_acpi_save);
> + pci_write_config_byte(dev, ACPICTRL, lpc_ich_acpi_save | 0x10);
> +
> + /* Setup GPIO base register */
> + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> + base_addr = base_addr_cfg & 0x0000ff80;
> + if (!base_addr) {
> + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> + return -ENODEV;
> + }
> +
> + gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE - 1;
> +
> + /* Enable LPC GPIO space */
> + pci_read_config_byte(dev, GPIOCTRL, &lpc_ich_gpio_save);
> + pci_write_config_byte(dev, GPIOCTRL, lpc_ich_gpio_save | 0x10);
> +
> + for (i=0; i < ARRAY_SIZE(lpc_ich_cells); i++)
checkpatch error (should be 'i = 0').
> + lpc_ich_cells[i].id = id->driver_data;
> +
It would be better to use
lpc_ich_cells[i].platform_data = &lpc_chipset_info[id->driver_data];
lpc_ich_cells[i].pdata_size = sizeof(struct lpc_ich_info);
ie pass the structure in the cell's platform_data. This way you can
avoid the need to export lpc_chipset_info, and you don't have to keep it
around either.
You don't need mfd_cell.platform_data to pass the pci_device pointer,
since the child driver can get it using
"to_pci_dev(platform_device->dev.parent)".
I ended up making all the changes suggested above (plus the matching
changes needed in the subsequent patches) while I rebased the code to
3.3-rc2. Right now I have an (untested) patchset which applies to
3.3-rc2. Please let me know how you would like to proceed; I can send it
to you if you like, or to the list.
Thanks,
Guenter
next prev parent reply other threads:[~2012-02-03 19:16 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-19 12:53 [PATCH] gpio: New driver for the Intel 82801 (ICH) GPIO pins Jean Delvare
2011-04-19 14:44 ` Grant Likely
2011-04-19 14:54 ` Alan Cox
2011-04-19 15:05 ` Grant Likely
2011-04-19 15:57 ` Alan Cox
2011-04-19 16:40 ` Anton Vorontsov
2011-04-19 17:08 ` Alan Cox
2011-04-19 20:30 ` Anton Vorontsov
2011-04-19 21:16 ` Alan Cox
2011-04-19 21:20 ` Alan Cox
2011-04-23 13:45 ` Jean Delvare
2011-04-23 14:47 ` Alan Cox
2011-05-19 11:33 ` Jean Delvare
2011-05-27 3:09 ` Grant Likely
2012-02-02 2:31 ` Guenter Roeck
2012-02-02 7:49 ` Jean Delvare
2012-02-02 17:35 ` Guenter Roeck
2012-02-02 19:56 ` Peter Tyser
2012-02-02 22:02 ` Guenter Roeck
2012-02-02 23:25 ` [PATCH 1/3] mfd: Add LPC driver for Intel ICH chipsets Aaron Sierra
2012-02-03 6:43 ` Guenter Roeck
2012-02-03 15:34 ` Aaron Sierra
2012-02-03 19:14 ` Guenter Roeck [this message]
2012-02-03 19:35 ` Aaron Sierra
2012-02-03 19:45 ` Guenter Roeck
2012-02-03 22:50 ` Aaron Sierra
2012-02-04 8:45 ` Jean Delvare
2012-02-04 16:45 ` Guenter Roeck
2012-02-07 19:56 ` [PATCH 1/3 v2] " Aaron Sierra
2012-02-07 20:15 ` Guenter Roeck
2012-02-07 20:31 ` Jean Delvare
2012-02-07 20:43 ` Guenter Roeck
2012-02-07 21:00 ` Aaron Sierra
2012-02-07 21:09 ` Guenter Roeck
2012-02-02 23:27 ` [PATCH 2/3] gpio: Add support for Intel ICHx/3100/Series[56] GPIO Aaron Sierra
2012-02-03 20:19 ` Guenter Roeck
2012-02-07 19:58 ` [PATCH 2/3 v2] " Aaron Sierra
2012-02-07 20:42 ` Guenter Roeck
2012-02-07 22:07 ` Jean Delvare
2012-02-07 23:25 ` Aaron Sierra
2012-02-02 23:29 ` [PATCH 3/3] watchdog: Convert iTCO_wdt driver to mfd model Aaron Sierra
2012-02-07 19:59 ` [PATCH 3/3 v2] " Aaron Sierra
2012-02-07 21:07 ` Guenter Roeck
2012-02-08 17:48 ` Aaron Sierra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1328296462.2261.193.camel@groeck-laptop \
--to=guenter.roeck@ericsson.com \
--cc=asierra@xes-inc.com \
--cc=grant.likely@secretlab.ca \
--cc=khali@linux-fr.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ptyser@xes-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.