| 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: <CAL1qeaF9ykWLpmXBe7EhVcUGpUSaqMqj-ckTef=6Ng=t+pBOGQ@mail.gmail.com>
Date: Wed, 6 May 2015 11:45:03 -0700
From: Andrew Bresticker <abrestic@...omium.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Ralf Baechle <ralf@...ux-mips.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Linux MIPS <linux-mips@...ux-mips.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ezequiel Garcia <ezequiel.garcia@...tec.com>,
James Hartley <james.hartley@...tec.com>,
James Hogan <james.hogan@...tec.com>,
Damien Horsley <Damien.Horsley@...tec.com>,
Govindraj Raja <govindraj.raja@...tec.com>,
Kevin Cernekee <cernekee@...omium.org>
Subject: Re: [PATCH V4 2/2] pinctrl: Add Pistachio SoC pin control driver
Hi Linus,
On Wed, May 6, 2015 at 12:14 AM, Linus Walleij <linus.walleij@...aro.org> wrote:
> Hi Andrew and sorry for a slow review process, I've been
> overloaded :(
>
> On Wed, Apr 29, 2015 at 3:13 AM, Andrew Bresticker
> <abrestic@...omium.org> wrote:
>
>> Add a driver for the pin controller present on the IMG Pistachio SoC.
>> This driver provides pinmux and pinconfig operations as well as GPIO
>> and IRQ chips for the GPIO banks.
>>
>> Signed-off-by: Damien Horsley <Damien.Horsley@...tec.com>
>> Signed-off-by: Govindraj Raja <govindraj.raja@...tec.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@...tec.com>
>> Signed-off-by: Kevin Cernekee <cernekee@...omium.org>
>> Signed-off-by: Andrew Bresticker <abrestic@...omium.org>
>> ---
>> Changes from v3:
>> - Addressed review comments from Ezequiel.
>
> Overall this is a very very nice looking driver so expect it to
> be merged after addressing or answering my last few
> concerns.
>
>> +config PINCTRL_PISTACHIO
>> + def_bool y if MACH_PISTACHIO
>> + select PINMUX
>> + select GENERIC_PINCONF
>> + select GPIOLIB_IRQCHIP
>
> I think you also need
> depends on / select GPIOLIB
> select OF_GPIO
>
> x86_64 allmodconfig is usually the best way to test
> if your GPIOs and pin control fragments are correctly Kconfig:ed.
Right, will fix.
>> +#define GPIO_BANK(_bank, _pin_base, _npins) \
>> + { \
>> + .gpio_chip = { \
>> + .label = "GPIO" #_bank, \
>> + .request = pistachio_gpio_request, \
>> + .free = pistachio_gpio_free, \
>> + .get_direction = pistachio_gpio_get_direction, \
>> + .direction_input = pistachio_gpio_direction_input, \
>> + .direction_output = pistachio_gpio_direction_output, \
>> + .get = pistachio_gpio_get, \
>> + .set = pistachio_gpio_set, \
>> + .base = _pin_base, \
>> + .ngpio = _npins, \
>> + }, \
>> + .irq_chip = { \
>> + .name = "GPIO" #_bank, \
>> + .irq_startup = pistachio_gpio_irq_startup, \
>> + .irq_ack = pistachio_gpio_irq_ack, \
>> + .irq_mask = pistachio_gpio_irq_mask, \
>> + .irq_unmask = pistachio_gpio_irq_unmask, \
>> + .irq_set_type = pistachio_gpio_irq_set_type, \
>> + }, \
>> + .gpio_range = { \
>> + .name = "GPIO" #_bank, \
>> + .id = _bank, \
>> + .base = _pin_base, \
>> + .pin_base = _pin_base, \
>> + .npins = _npins, \
>> + }, \
>> + }
>
> This -gpio_range is the only thing that bothers me a little, combined with
> this:
>
>> + bank->gpio_range.gc = &bank->gpio_chip;
>> + pinctrl_add_gpio_range(pctl->pctldev, &bank->gpio_range);
>
> Because it adds the ranges from the pinctrl side instead of
> doing it from the gpiochip side using
> gpiochip_add_pin_range() or gpiochip_add_pingroup_range().
>
> Have you tried using those (from <linux/gpio/driver.h> instead?
>
> They have the upside that .base is taken from the gpio_chip
> meaning it is unnecessary to define .base for the gpiochip
> too and you can just go for what gpiolib dynamically assigns
> for you.
Yup, switching to gpiochip_add_pin_range() works just fine here.
>> +static int __init pistachio_pinctrl_register(void)
>> +{
>> + return platform_driver_register(&pistachio_pinctrl_driver);
>> +}
>> +arch_initcall(pistachio_pinctrl_register);
>
> Is it necessary to have it registered so early?
No, nothing depends on it being initialized this early. When I was
asked to remove the module stuff, I just picked arch_initcall since
that's what most other pinctrl drivers not using
module_platform_driver() use. It also has the benefit of reducing the
number of probe deferrals.
Thanks,
Andrew
--
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