| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdYG641KYR05jtu3OS7BDGcFpJ-pzu+gTM+RHJnMqytfEQ@mail.gmail.com>
Date: Mon, 10 Dec 2012 10:48:39 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: grant.likely@...retlab.ca, mika.westerberg@...ux.intel.com,
rafael.j.wysocki@...el.com, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org
Subject: Re: [PATCH 1/1] gpio: add Lynxpoint chipset gpio driver.
On Fri, Dec 7, 2012 at 3:01 PM, Mathias Nyman
<mathias.nyman@...ux.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@...ux.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.)
> + select IRQ_DOMAIN
> + help
> + driver for GPIO functionality on Intel Lynxpoint PCH chipset
> + Requires platform or ACPI code to set up a platform device.
(...)
> +++ b/drivers/gpio/gpio-lynxpoint.c
(...)
> +/* LynxPoint chipset has support for 94 gpio pins */
> +
> +#define LP_NUM_GPIO 94
> +
> +/* Register offsets */
> +#define LP_ACPI_OWNED 0x00 /* Bitmap, set by bios, 0: pin reserved for ACPI */
> +#define LP_GC 0x7C /* set APIC IRQ to IRQ14 or IRQ15 for all pins */
> +#define LP_INT_STAT 0x80
> +#define LP_INT_ENABLE 0x90
> +
> +/* Each pin has two 32 bit config registers, starting at 0x100 */
> +#define LP_CONFIG1 0x100
> +#define LP_CONFIG2 0x104
> +
> +/* LP_CONFIG1 reg bits */
> +#define OUT_LVL_BIT BIT(31)
> +#define IN_LVL_BIT BIT(30)
> +#define TRIG_SEL_BIT BIT(4) /* 0: Edge, 1: Level */
> +#define INT_INV_BIT BIT(3) /* Invert interrupt triggering */
> +#define DIR_BIT BIT(2) /* 0: Output, 1: Input */
> +#define USE_SEL_BIT BIT(0) /* 0: Native, 1: GPIO */
> +
> +/* LP_CONFIG2 reg bits */
> +#define GPINDIS_BIT BIT(2) /* disable input sensing */
> +#define GPIWP_BIT (BIT(0) | BIT(1)) /* weak pull options */
Kudos for using BIT() macros, very readable, thanks!
> +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.
> +};
> +
> +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.)
> +{
> + 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...
> + /* Fail if pin is in alternate function mode (not GPIO mode) */
> + if (!(inl(reg) & USE_SEL_BIT))
> + return -ENODEV;
> +
> + /* enable input sensing */
> + outl(inl(conf2) & ~GPINDIS_BIT, conf2);
> +
> + return 0;
> +}
(...)
> +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".
> + unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> + outl(inl(reg) | BIT(gpio % 32), reg);
> + spin_unlock_irqrestore(&lg->lock, flags);
> +}
> +
> +static void lp_irq_disable(struct irq_data *d)
> +{
> + struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
> + u32 gpio = irqd_to_hwirq(d);
Dito.
> + unsigned long reg = gpio_reg(&lg->chip, gpio, LP_INT_ENABLE);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&lg->lock, flags);
> + outl(inl(reg) & ~BIT(gpio % 32), reg);
> + spin_unlock_irqrestore(&lg->lock, flags);
> +}
(...)
> +#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?
> +static int __devexit lp_gpio_remove(struct platform_device *pdev)
All __devinit/__devexit macros are going away in v3.8 so delete them
everywhere.
> +{
> + struct lp_gpio *lg = platform_get_drvdata(pdev);
> + int err;
> + err = gpiochip_remove(&lg->chip);
> + if (err)
> + dev_warn(&pdev->dev, "failed to remove gpio_chip.\n");
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +static struct platform_driver lp_gpio_driver = {
> + .probe = lp_gpio_probe,
> + .remove = __devexit_p(lp_gpio_remove),
Delete that __devexit_p() too I suppose.
> + .driver = {
> + .name = "lp_gpio",
> + .owner = THIS_MODULE,
> + .acpi_match_table = ACPI_PTR(lynxpoint_gpio_acpi_match),
> + },
> +};
> +
> +static int __init lp_gpio_init(void)
> +{
> + return platform_driver_register(&lp_gpio_driver);
> +}
> +
> +subsys_initcall(lp_gpio_init);
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists