Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 12/10/2012 11:48 AM, Linus Walleij wrote:
> On Fri, Dec 7, 2012 at 3:01 PM, Mathias Nyman
> <mathias.nyman@linux.intel.com>  wrote:
>
>> Add gpio support for Intel Lynxpoint chipset.
>> Lynxpoint supports 94 gpio pins which can generate interrupts.
>> Driver will fail requests for pins that are marked as owned by ACPI, or
>> set in an alternate mode (non-gpio).
>>
>> Signed-off-by: Mathias Nyman<mathias.nyman@linux.intel.com>
>
> Nice shape on this patch, makes it easy to focus the review on the
> important stuff, thanks Magnus!
>
>> +config GPIO_LYNXPOINT
>> +       bool "Intel Lynxpoint GPIO support"
>
> So you don't want to be able to build it as module?
> (OK just odd on Intel systems.)

Reason for having it as bool was the subsys_initcall() got degraded
to something like module_init() if built as module. This was not good 
because the IO port ranges used for gpios were specified in ACPI tables 
both in the gpio device, and as a part of a motherboard device. Pnpacpi 
code would then reserve all the IO port ranges in the motherboard device 
before this gpio driver had a chance to take reserve them.

I have to re-check if this is still the case.

>> +struct lp_gpio {
>> +       struct gpio_chip        chip;
>> +       struct irq_domain       *domain;
>> +       struct platform_device  *pdev;
>> +       spinlock_t              lock;
>> +       unsigned                hwirq;
>
> This struct member is only used in probe() so make it a local variable there
> and cut it from the struct.
>
>> +       unsigned long           reg_base;
>> +       unsigned long           reg_len;
>
> Same comment for reg_len.

Will fix

>
>> +};
>> +
>> +static unsigned long gpio_reg(struct gpio_chip *chip, unsigned gpio, int reg)
>
> Rename lp_gpio_reg() so as to match the family.
>
> Rename the argument "gpio" to "offset" since it's a local number,
> not in the global GPIO numberspace. (Please change this everywhere
> a local index is used.)

Sure.

>
>> +{
>> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +       int offset;
>> +
>> +       if (reg == LP_CONFIG1 || reg == LP_CONFIG2)
>> +               offset = gpio * 8;
>> +       else
>> +               offset = (gpio / 32) * 4;
>
> Put in some comment above this explaining the layout of the offsets
> for these two cases so we understand what's happening here.
>
>> +       return lg->reg_base + reg + offset;
>> +}
>> +
>> +static int lp_gpio_request(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +       struct lp_gpio *lg = container_of(chip, struct lp_gpio, chip);
>> +       unsigned long reg = gpio_reg(chip, gpio, LP_CONFIG1);
>> +       unsigned long conf2 = gpio_reg(chip, gpio, LP_CONFIG2);
>> +       unsigned long acpi_use = gpio_reg(chip, gpio, LP_ACPI_OWNED);
>> +
>> +       /* Fail if BIOS reserved pin for ACPI use */
>> +       if (!(inl(acpi_use)&  BIT(gpio % 32))) {
>> +               dev_err(&lg->pdev->dev, "gpio %d reserved for ACPI\n", gpio);
>> +               return -EBUSY;
>> +       }
>
> This %32 magic only seems to consider the latter part of the if()
> statement in the gpio_reg() function. It's like you assume only the
> (gpio / 32) * 4 path to be taken. It seems that for the two models
> handled there this should be %8 or something. Or I'm getting it
> wrong, which is an indication that something is pretty unclear here...
>

I should document the register layout better. It's a bit messy because 
in some cases a register represents a functionality where each bit 
stands for a gpio (bitmapped) (each register is 32 bit, so to cover all 
94 gpios three 32bit registers are needed).

In other cases a register represent a gpio, and each bit a 
functionality. This is the case with LP_CONFIGx registers. So we got
94 LP_CONFIG1 registers and 94 LP_CONFIG2 registers.

In the case of acpi_use & BIT(gpio % 32) I know that acpi_used is 
pointing to one of the three LP_ACPI_OWNED bitmapped registers, and the 
(gpio / 32 * 4) path is taken, and % 32 will work.

I'll add better description of the register layout as a comment

>> +static void lp_irq_enable(struct irq_data *d)
>> +{
>> +       struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
>> +       u32 gpio = irqd_to_hwirq(d);
>
> That variable is confusingly named. It's not a global gpio number,
> it's a local offset, so please rename it "offset".

sure, (is "pin" ok?  "offset" is already used in may places)

>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = {
>> +       { "INT33C7", 0 },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, lynxpoint_gpio_acpi_match);
>> +#endif
>
> Aha so how many users are there of this driver which are not
> using ACPI?
>
> If zero, why not just depend on ACPI in Kconfig?
>

Only ACPI support for now.
Will remove ifdefs and add dependency.

>> +static int __devexit lp_gpio_remove(struct platform_device *pdev)
>
> All __devinit/__devexit macros are going away in v3.8 so delete them
> everywhere.

Ah, Ok.

>
> Yours,
> Linus Walleij

Thanks for taking the time to review this, I'll make the suggested changes.

I also got some offline comments about adding runtime pm/system suspend 
code to this driver.

-Mathias