On 12/10/2012 11:48 AM, Linus Walleij wrote:
On Fri, Dec 7, 2012 at 3:01 PM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
