gpio: Add PMIC GPIO block support
PATCH 1/1 gpio: Add PMIC GPIO block support
drivers/gpio is not for platform specific devices apparently so you get this one instead. Actually that makes a lot of sense as the GPIO in question is via the SCU IPC interface which is already in platform/x96
From: Alek Du <[email protected]>
Moorestown has PMIC chip which contains GPIO blocks. The PMIC chip is connected to Langwell by SPI interface. So this GPIO driver will be regarded as SPI GPIO expander though the actual GPIO access is through IPC and SRAM. The SPI master contoller will probe this device driver by parsing SPIB table.
Cleaned up for new IPC, GPE removed and some printk and other tidying by Alan Cox.
---
drivers/platform/x86/Kconfig | 7 + drivers/platform/x86/Makefile | 2 drivers/platform/x86/intel_pmic_gpio.c | 341 ++++++++++++++++++++++++++++++++ include/linux/intel_pmic_gpio.h | 15 + 4 files changed, 365 insertions(+), 0 deletions(-) create mode 100644 drivers/platform/x86/intel_pmic_gpio.c create mode 100644 include/linux/intel_pmic_gpio.h
drivers/platform/x86/Kconfig +7 -0
@@ -539,6 +539,13 @@ config INTEL_SCU_IPC some embedded Intel x86 platforms. This is not needed for PC-type machines. +config GPIO_INTEL_PMIC + bool "Intel PMIC GPIO support" + depends on INTEL_SCU_IPC && GPIOLIB + help + Say Y here to support GPIO via the SCU IPC interface + on Intel MID platforms. + config RAR_REGISTER bool "Restricted Access Region Register Driver" depends on PCI && X86_MRST
drivers/platform/x86/Makefile +2 -0
@@ -28,3 +28,5 @@ obj-$(CONFIG_TOSHIBA_BT_RFKILL) += toshiba_bluetooth.o obj-$(CONFIG_INTEL_SCU_IPC) += intel_scu_ipc.o obj-$(CONFIG_RAR_REGISTER) += intel_rar_register.o obj-$(CONFIG_INTEL_IPS) += intel_ips.o +obj-$(CONFIG_GPIO_INTEL_PMIC) += intel_pmic_gpio.o +
drivers/platform/x86/intel_pmic_gpio.c new file +341 -0
341 lines not on this page · raw message
include/linux/intel_pmic_gpio.h new file +15 -0
@@ -0,0 +1,15 @@ +#ifndef LINUX_INTEL_PMIC_H +#define LINUX_INTEL_PMIC_H + +struct intel_pmic_gpio_platform_data { +/* the first IRQ of the chip */ +unsigned irq_base; +/* number assigned to the first GPIO */ +unsigned gpio_base; +/* sram address for gpiointr register, the langwell chip will map +* the PMIC spi GPIO expander's GPIOINTR register in sram. +*/ +unsigned gpiointr; +}; + +#endif
drivers/platform/x86/intel_pmic_gpio.c line 151
Quoted 5 lines · Alan Cox
drivers/platform/x86/intel_pmic_gpio.c new file +5
-0+else if (offset <14 )/* it is GPOSW */ + rc =intel_scu_ipc_update_register (GPOSWCTL0 + offset -8 , +GPOSW_DRV |GPOSW_DOU |GPOSW_RDRV , +GPOSW_DRV | (value ?GPOSW_DOU :0 )); +else if (offset >15 && offset <24 )/* it is GPO */
What happened to gpios 14 and 15?
drivers/platform/x86/intel_pmic_gpio.c line 170
Quoted 8 lines
drivers/platform/x86/intel_pmic_gpio.c new file +8
-0+ +static int pmic_gpio_get (struct gpio_chip *chip,unsigned offset) +{ +u8 r; + +/* we only have 8 GPIO pins we can use as input */ +if (offset >8 ) +return -1 ;
Is there any chance of this getting passed up to userspace? Using something more descriptive would be nice (ditto for a couple of other places)
drivers/platform/x86/intel_pmic_gpio.c line 172
Quoted 2 lines
drivers/platform/x86/intel_pmic_gpio.c new file +2
-0+if (intel_scu_ipc_ioread8 (GPIO0 + offset, &r)) +return -1 ;
...especially since you get the same error for two different failure modes :)
drivers/platform/x86/intel_pmic_gpio.c line 198
Quoted 7 lines
drivers/platform/x86/intel_pmic_gpio.c new file +7
-0+static int pmic_irq_type (unsigned irq,unsigned type) +{ +struct pmic_gpio *pg =get_irq_chip_data (irq); +u32 gpio = irq - pg->irq_base; +unsigned long flags; + +if (gpio <0 || gpio > pg->chip.ngpio)
It's unsigned, don't need to check that it's < 0.
drivers/platform/x86/intel_pmic_gpio.c line 253
Quoted 5 lines
drivers/platform/x86/intel_pmic_gpio.c new file +5
-0+static int __devinit platform_pmic_gpio_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +int irq =platform_get_irq (pdev,0 ); +struct intel_pmic_gpio_platform_data *pdata = dev->platform_data;
I don't think it's really necessary to indicate that the driver's loaded here.
drivers/platform/x86/intel_pmic_gpio.c line 270
Quoted 4 lines
drivers/platform/x86/intel_pmic_gpio.c new file +4
-0+if (!pdata || !pdata->gpio_base || !pdata->irq_base) { +dev_dbg (dev,"incorrect or missing platform data\n" ); +return -EINVAL ; + }
Shouldn't there be an is_mrst() check before trying to dereference this?
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 10 lines · On Thu, 8 Jul 2010 20:37:08 +0100 Matthew Garrett <[email protected]> wrote:
drivers/platform/x86/intel_pmic_gpio.c line 151
Quoted 7 lines · Alan Cox
(quoted diff) +2
-0+ else if (offset < 14)/* it is GPOSW */ + rc = intel_scu_ipc_update_register(GPOSWCTL0 +offset - 8,
(quoted diff) +2
-0+ GPOSW_DRV | GPOSW_DOU | GPOSW_RDRV, + GPOSW_DRV | (value ? GPOSW_DOU :0));
drivers/platform/x86/intel_pmic_gpio.c new file +1
-0+else if (offset >15 && offset <24 )/* it is GPO */ What happened to gpios 14 and 15?
Good question. I will double check this is correct but since everyone is using the driver its probably right
Quoted 4 lines
Quoted 4 lines
drivers/platform/x86/intel_pmic_gpio.c new file +4
-0+ +static int pmic_gpio_get (struct gpio_chip *chip,unsigned offset) +{ +u8 r;
Quoted 8 lines
drivers/platform/x86/intel_pmic_gpio.c line 170
Quoted 4 lines
drivers/platform/x86/intel_pmic_gpio.c new file +4
-0+ +/* we only have 8 GPIO pins we can use as input */ +if (offset >8 ) +return -1 ;Is there any chance of this getting passed up to userspace? Using something more descriptive would be nice (ditto for a couple of other places)
No but it can get back to the caller. Right now gpiolib isn't really internally sure what to do about gpio reads failing. Given that I might as well hand back -Efoo codes.
Quoted 6 lines
drivers/platform/x86/intel_pmic_gpio.c line 172
Quoted 2 lines
drivers/platform/x86/intel_pmic_gpio.c new file +2
-0+if (intel_scu_ipc_ioread8 (GPIO0 + offset, &r)) +return -1 ;...especially since you get the same error for two different failure modes :)
Quoted 9 lines
drivers/platform/x86/intel_pmic_gpio.c line 198
Quoted 7 lines
drivers/platform/x86/intel_pmic_gpio.c new file +7
-0+static int pmic_irq_type (unsigned irq,unsigned type) +{ +struct pmic_gpio *pg =get_irq_chip_data (irq); +u32 gpio = irq - pg->irq_base; +unsigned long flags; + +if (gpio <0 || gpio > pg->chip.ngpio)It's unsigned, don't need to check that it's < 0.
Fixed
Quoted 2 lines
I don't think it's really necessary to indicate that the driver's loaded here.
Agreed, fixed
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +2
-0+ if (!pdata || !pdata->gpio_base || !pdata->irq_base) { + dev_dbg(dev, "incorrect or missing platformdata\n");
(quoted diff) +2
-0+ return -EINVAL; + }Shouldn't there be an
is_mrst()check before trying to dereference this?
We check pdata == NULL so in the theoretical case someone loaded the driver on the wrong platform and cooked up a device match you'd still be ok
Alan