| 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: <50F5A1B7.20504@wwwdotorg.org>
Date: Tue, 15 Jan 2013 11:36:39 -0700
From: Stephen Warren <swarren@...dotorg.org>
To: Linus Walleij <linus.walleij@...ricsson.com>
CC: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Stephen Warren <swarren@...dia.com>,
Anmar Oueja <anmar.oueja@...aro.org>,
Lee Jones <lee.jones@...aro.org>,
Patrice Chotard <patrice.chotard@...com>,
Samuel Ortiz <sameo@...ux.intel.com>,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 3/4] pinctrl: add abx500 pinctrl driver core
On 01/15/2013 02:43 AM, Linus Walleij wrote:
> From: Patrice Chotard <patrice.chotard@...com>
>
> This adds the AB8500 core driver, which will be utilized by
> the follow-on drivers for different ABx500 variants.
> Sselect the driver from the DBX500_SOC, as this chip is
> powering and clocking that SoC.
> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
> +static int abx500_gpio_get(struct gpio_chip *chip, unsigned offset)
Shouldn't this call abx500_gpio_get_bit(), just like abx500_gpio_set()
calls abx500_gpio_set_bit()?
> +static int abx500_gpio_direction_output(struct gpio_chip *chip,
> + unsigned offset,
> + int val)
...
> + /* disable pull down */
...
> + /* if supported, disable both pull down and pull up */
Why the need to override those options?
> +static u8 abx500_get_mode(struct pinctrl_dev *pctldev, struct gpio_chip *chip,
> + unsigned gpio)
> + if (af.gpiosel_bit == UNUSED)
> + return ABX500_DEFAULT;
That's odd; abx500_set_mode() seems to allow setting the mode to
something other than default even if (af.gpiosel_bit == UNUSED). Are
set_mode/get_mode actually correct inverses of each-other?
> +static int abx500_gpio_irq_init(struct abx500_pinctrl *pct)
...
> +#ifdef CONFIG_ARM
> + set_irq_flags(irq, IRQF_VALID);
> +#else
> + irq_set_noprobe(irq);
> +#endif
I assume that ifdef is always set one particular way?
> +static void abx500_gpio_irq_remove(struct abx500_pinctrl *pct)
...
> +#ifdef CONFIG_ARM
> + set_irq_flags(irq, 0);
> +#endif
Same there.
> +static void abx500_pmx_disable(struct pinctrl_dev *pctldev,
> + unsigned function, unsigned group)
> +{
> + struct abx500_pinctrl *pct = pinctrl_dev_get_drvdata(pctldev);
> + const struct abx500_pingroup *g;
> +
> + g = &pct->soc->groups[group];
> + if (g->altsetting < 0)
> + return;
> +
> + /* Poke out the mux, set the pin to some default state? */
> + dev_dbg(pct->dev, "disable group %s, %u pins\n", g->name, g->npins);
> +}
That looks basically unimplemented, and the comment seems like a FIXME?
> +int abx500_gpio_request_enable(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range,
> + unsigned offset)
...
> + /*
> + * by default, for ABx5xx family, GPIO mode is selected by
> + * writing 1 in GPIOSELx registers
> + */
> + ret = abx500_mask_and_set_register_interruptible(pct->dev,
> + AB8500_MISC, reg, 1 << pos, 1 << pos);
It sounds like this should be implemented using abx500_set_mode()?
> +int abx500_pin_config_set(struct pinctrl_dev *pctldev,
> + unsigned pin,
> + unsigned long config)
> + switch (param) {
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + /*
> + * if argument = 1 set the pull down
> + * else clear the pull down
> + */
> + ret = abx500_gpio_direction_input(chip, offset);
That looks odd; why force the pin to be a GPIO just to enable a pull down?
> + /* check if pin supports pull updown feature */
> + if (pullud && pin >= pullud->first_pin && pin <= pullud->last_pin)
> + ret = abx500_config_pull_updown(pct,
> + offset,
> + argument ? ABX500_GPIO_PULL_DOWN : ABX500_GPIO_PULL_NONE);
> + else
> + ret = abx500_gpio_set_bits(chip, AB8500_GPIO_PUD1_REG,
> + offset, argument ? 0 : 1);
Hmm. Wouldn't it be better to remove the if statement, and just store
ABX500_GPIO_PULL_DOWN or 0, and ABX500_GPIO_PULL_NONE or 1, in the soc_data?
> +static int __devinit abx500_gpio_probe(struct platform_device *pdev)
> + /* Poke in other ASIC variants here */
> + switch (platid->driver_data) {
> + case PINCTRL_AB8500:
> + abx500_pinctrl_ab8500_init(&pct->soc);
> + break;
> + case PINCTRL_AB8540:
> + abx500_pinctrl_ab8540_init(&pct->soc);
> + break;
> + case PINCTRL_AB9540:
> + abx500_pinctrl_ab9540_init(&pct->soc);
> + break;
> + case PINCTRL_AB8505:
> + abx500_pinctrl_ab8505_init(&pct->soc);
> + break;
> + default:
> + dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
> + (int) platid->driver_data);
> + return -EINVAL;
> + }
Most of those functions don't exist yet. I see there are dummy inlines
below for them if the /config/ options aren't turned on, but what's to
stop somebody turning on the config option before the real
implementation exists?
In the past, Arnd requested that each variant had a separate top-level
driver object that called into a utility probe() function, rather than
having a probe() function that knew about all the SoC variants, and
dispatched out to a variant-specific function.
> +static int __devexit abx500_gpio_remove(struct platform_device *pdev)
> + platform_set_drvdata(pdev, NULL);
There's no point doing that; nothing should touch the drvdata while the
device doesn't exist (or isn't probed rather).
> + mutex_destroy(&pct->lock);
> + kfree(pct);
That was allocated using devm_kzalloc(). There's no point freeing it
here, and if there were, devm_kfree() should be used, or a double-free
will occur.
--
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