Hi Linus,
On Wed, May 6, 2015 at 12:14 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Govindraj Raja <govindraj.raja@xxxxxxxxxx>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@xxxxxxxxxx>
>> Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxxxxx>
>> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>> ---
>> 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
