Re: [PATCH V4 2/2] pinctrl: Add Pistachio SoC pin control driver

[Andrew Bresticker]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/