Re: [PATCH] gpio: Add support for Intel SoC PMIC (Crystal Cove)

["Zhu, Lejun" <lejun.zhu@linux.intel.com>]

On 5/17/2014 10:37 PM, Alexandre Courbot wrote:
> On Thu, May 15, 2014 at 12:44 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
>>
>> Signed-off-by: Yang, Bin <bin.yang@intel.com>
>> Signed-off-by: Zhu, Lejun <lejun.zhu@linux.intel.com>
>> ---
>>  drivers/gpio/Kconfig            |   9 ++
>>  drivers/gpio/Makefile           |   1 +
>>  drivers/gpio/gpio-crystalcove.c | 323 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 333 insertions(+)
>>  create mode 100644 drivers/gpio/gpio-crystalcove.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index a86c49a..95bb5a0 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -440,6 +440,15 @@ config GPIO_ARIZONA
>>         help
>>           Support for GPIOs on Wolfson Arizona class devices.
>>
>> +config GPIO_INTEL_SOC_PMIC
>> +       bool "GPIO on Intel SoC PMIC"
>> +       depends on INTEL_SOC_PMIC
>> +       help
>> +         Support for GPIO pins on Intel SoC PMIC, such as Crystal
>> +         Cove.
>> +         Say Y if you have a tablet with Intel SoC (e.g. Baytrail)
>> +         inside.
>> +
>>  config GPIO_LP3943
>>         tristate "TI/National Semiconductor LP3943 GPIO expander"
>>         depends on MFD_LP3943
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 6309aff..5380608 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_GPIO_F7188X)     += gpio-f7188x.o
>>  obj-$(CONFIG_GPIO_GE_FPGA)     += gpio-ge.o
>>  obj-$(CONFIG_GPIO_GRGPIO)      += gpio-grgpio.o
>>  obj-$(CONFIG_GPIO_ICH)         += gpio-ich.o
>> +obj-$(CONFIG_GPIO_INTEL_SOC_PMIC)      += gpio-crystalcove.o
>>  obj-$(CONFIG_GPIO_IOP)         += gpio-iop.o
>>  obj-$(CONFIG_GPIO_IT8761E)     += gpio-it8761e.o
>>  obj-$(CONFIG_GPIO_JANZ_TTL)    += gpio-janz-ttl.o
>> diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
>> new file mode 100644
>> index 0000000..974f9b8
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-crystalcove.c
>> @@ -0,0 +1,323 @@
>> +/*
>> + * gpio-crystalcove.c - Intel Crystal Cove GPIO Driver
>> + *
>> + * Copyright (C) 2012, 2014 Intel Corporation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version
>> + * 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.
>> + *
>> + * Author: Yang, Bin <bin.yang@intel.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/sched.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/gpio.h>
>> +
>> +#define NUM_GPIO               16
>> +
>> +#define UPDATE_TYPE            (1 << 0)
>> +#define UPDATE_MASK            (1 << 1)
>> +
>> +#define GPIO0IRQ               0x0b
>> +#define GPIO1IRQ               0x0c
>> +#define MGPIO0IRQS0            0x19
>> +#define MGPIO1IRQS0            0x1a
>> +#define MGPIO0IRQSX            0x1b
>> +#define MGPIO1IRQSX            0x1c
>> +#define GPIO0P0CTLO            0x2b
>> +#define GPIO0P0CTLI            0x33
>> +#define GPIO1P0CTLO            0x3b
>> +#define GPIO1P0CTLI            0x43
>> +
>> +#define CTLI_INTCNT_NE         (1 << 1)
>> +#define CTLI_INTCNT_PE         (2 << 1)
>> +#define CTLI_INTCNT_BE         (3 << 1)
>> +
>> +#define CTLO_DIR_OUT           (1 << 5)
>> +#define CTLO_DRV_CMOS          (0 << 4)
>> +#define CTLO_DRV_OD            (1 << 4)
>> +#define CTLO_DRV_REN           (1 << 3)
>> +#define CTLO_RVAL_2KDW         (0)
>> +#define CTLO_RVAL_2KUP         (1 << 1)
>> +#define CTLO_RVAL_50KDW                (2 << 1)
>> +#define CTLO_RVAL_50KUP                (3 << 1)
>> +
>> +#define CTLO_INPUT_DEF (CTLO_DRV_CMOS | CTLO_DRV_REN | CTLO_RVAL_2KUP)
>> +#define CTLO_OUTPUT_DEF        (CTLO_DIR_OUT | CTLO_INPUT_DEF)
>> +
>> +struct crystalcove_gpio {
>> +       struct mutex            buslock; /* irq_bus_lock */
>> +       struct gpio_chip        chip;
>> +       int                     irq;
>> +       int                     irq_base;
>> +       int                     update;
>> +       int                     trigger_type;
>> +       int                     irq_mask;
>> +};
>> +static struct crystalcove_gpio gpio_info;
>> +
>> +static void __crystalcove_irq_mask(int gpio, int mask)
>> +{
>> +       u8 mirqs0 = gpio < 8 ? MGPIO0IRQS0 : MGPIO1IRQS0;
>> +       int offset = gpio < 8 ? gpio : gpio - 8;
>> +
>> +       if (mask)
>> +               intel_soc_pmic_setb(mirqs0, 1 << offset);
>> +       else
>> +               intel_soc_pmic_clearb(mirqs0, 1 << offset);
>> +}
>> +
>> +static void __crystalcove_irq_type(int gpio, int type)
>> +{
>> +       u8 ctli = gpio < 8 ? GPIO0P0CTLI + gpio : GPIO1P0CTLI + (gpio - 8);
> 
> Not a big comment, but wouldn't it be safer to factorize this logic
> (which is repeated in many places) into some macro? e.g. something
> like:
> 
> #define GPIOP0CTL(gpio, dir) ((gpio < 8 ? GPIO0P0CTL ## dir :
> GPIO1P0CTL ## dir) + (gpio & ~0x8))
> 
> ...
> 
> u8 ctli = GPIOP0CTL(gpio, I);
> 
> Feel free to come with a better macro (or to ignore that comment
> altogether if you think it makes the code less readable), but I think
> it would be less error-prone if you did not have to copy-paste that
> code all over the place.
> 

Thank you. I'll fix them in v2 as well.

Best Regards
Lejun