| 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: <20141029101047.GL1304@lahna.fi.intel.com>
Date: Wed, 29 Oct 2014 12:10:47 +0200
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Alexandre Courbot <gnurou@...il.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Ning Li <ning.li@...el.com>, Alan Cox <alan@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] pinctrl: Add Intel Cherryview/Braswell pin
controller support
On Wed, Oct 29, 2014 at 10:35:01AM +0100, Linus Walleij wrote:
> On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
> <mika.westerberg@...ux.intel.com> wrote:
>
> > This driver supports the pin/GPIO controllers found in newer Intel SoCs
> > like Cherryview and Braswell. The driver provides full GPIO support and
> > minimal set of pin controlling funtionality.
> >
> > The driver is based on the original Cherryview GPIO driver authored by Ning
> > Li and Alan Cox.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
>
> *VERY* nice work Mika! Just minor nitpicks...
>
> (...)
> > +static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
> > + unsigned long *config)
> > +{
> > + struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + enum pin_config_param param = pinconf_to_config_param(*config);
> > + unsigned long flags;
> > + u32 ctrl0, ctrl1;
> > + u16 arg = 0;
> > + u32 term;
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > + ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
> > + spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > + term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
> > +
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + if (term)
> > + return -EINVAL;
> > + break;
> > +
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + if (!(ctrl0 & CHV_PADCTRL0_TERM_UP))
> > + return -EINVAL;
> > +
> > + switch (term) {
> > + case CHV_PADCTRL0_TERM_20K:
> > + arg = 20;
>
> These are in Ohms IIRC so should be 20000
>
> > + break;
> > + case CHV_PADCTRL0_TERM_5K:
> > + arg = 5;
>
> 5000
>
> > + break;
> > + case CHV_PADCTRL0_TERM_1K:
> > + arg = 1;
>
> 1000
>
> > + break;
> > + }
> > +
> > + break;
> > +
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + if (!term || (ctrl0 & CHV_PADCTRL0_TERM_UP))
> > + return -EINVAL;
> > +
> > + switch (term) {
> > + case CHV_PADCTRL0_TERM_20K:
> > + arg = 20;
>
> 20000
>
> > + break;
> > + case CHV_PADCTRL0_TERM_5K:
> > + arg = 5;
>
> 5000
Right, I'll change them.
> (...)
> > +static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
> > + enum pin_config_param param, u16 arg)
> > +{
> > + void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
> > + unsigned long flags;
> > + u32 ctrl0, pull;
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + ctrl0 = readl(reg);
> > +
> > + pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
> > + switch (arg) {
>
> This looks seriously convoluted: you can't inspect an argument before
> checking what parameter you're dealing with. This should be
> under a case PIN_CONFIG_BIAS_PULL_UP in the switch (param)
> below I think?
OK.
>
> > + case 1:
>
> case 1000
>
> > + /* For 1k there is only pull up */
> > + if (param == PIN_CONFIG_BIAS_PULL_UP)
> > + pull = CHV_PADCTRL0_TERM_1K << CHV_PADCTRL0_TERM_SHIFT;
>
> Well you do check it here but...0
>
> > + break;
> > + case 5:
>
> case 5000
>
> > + pull = CHV_PADCTRL0_TERM_5K << CHV_PADCTRL0_TERM_SHIFT;
>
> This will be applied to whatever config with arg == 5000 comes here!
>
> (...)
> > +static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> > + unsigned offset)
> > +{
> > + return pctrl->community->pins[offset].number;
> > +}
>
> I'm a bit worried about the massive pin<->offsets<->gpio# translations
> happening in this and patch 1/4 etc. It's a bit unsettling. Are you
> sure we are translating in the simplest way?
We are translating from ACPI GPIO number, which is relative to the GPIO
controller in question to pin controller space (which is not 1:1 in this
driver).
I'll double check this, just in case.
> > +static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > + struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip);
> > + int pin = chv_gpio_offset_to_pin(pctrl, offset);
> > + unsigned long flags;
> > + u32 ctrl0, cfg;
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > + ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > + spin_unlock_irqrestore(&pctrl->lock, flags);
>
> If you need a lock before and after reading every register in this
> range, consider regmap-mmio, because that is part of what
> it does. Just a hint...
Yeah, I don't think we need a lock for reading simple registers so I'll
remove such usage from the driver.
> (...)
> > +static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> > +{
> > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > + struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc);
> > + int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
> > + u32 value, intr_line;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&pctrl->lock, flags);
> > +
> > + intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> > + intr_line &= CHV_PADCTRL0_INTSEL_MASK;
> > + intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
> > +
> > + value = readl(pctrl->regs + CHV_INTMASK);
> > + if (mask)
> > + value &= ~(1 << intr_line);
>
> I usually do this kind of stuff with
>
> #include <linux/bitops.h>
>
> value &= ~BIT(intr_line);
>
> > + else
> > + value |= (1 << intr_line);
>
> value |= BIT(intr_line);
>
> (probably a few more occasions in the driver)
OK, will fix.
--
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