Message-ID: <3959f3be-19bd-c9c4-36ee-e93959a7f8e1@gmail.com>
Date:   Tue, 2 Mar 2021 17:59:03 +0100
From:   Álvaro Fernández Rojas <noltari@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jonas Gorski <jonas.gorski@...il.com>,
        Necip Fazil Yildiran <fazilyildiran@...il.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/12] pinctrl: add a pincontrol driver for BCM6328

Hi Linus,

El 02/03/2021 a las 16:20, Linus Walleij escribió:
> Hi Álvaro,
> 
> thanks for your patch!
> 
> On Thu, Feb 25, 2021 at 5:42 PM Álvaro Fernández Rojas
> <noltari@...il.com> wrote:
> 
>> Add a pincontrol driver for BCM6328. BCM628 supports muxing 32 pins as
>> GPIOs, as LEDs for the integrated LED controller, or various other
>> functions. Its pincontrol mux registers also control other aspects, like
>> switching the second USB port between host and device mode.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
>> Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> 
> Thanks for working on this. This SoC definitely need to come upstream.

I will try my best :).

> 
> I think this driver can be simplified a bit and reuse some core infrastructure
> to make it more maintainable. It might be a bit of challenge but definitely
> worth it!
> 
>> +config PINCTRL_BCM6328
>> +       bool "Broadcom BCM6328 GPIO driver"
>> +       depends on OF_GPIO && (BMIPS_GENERIC || COMPILE_TEST)
>> +       select PINMUX
>> +       select PINCONF
>> +       select GENERIC_PINCONF
>> +       select MFD_SYSCON
>> +       default BMIPS_GENERIC
>> +       help
>> +          Say Y here to enable the Broadcom BCM6328 GPIO driver.
> 
> I suggest
> 
> select GPIO_REGMAP
> select GPIOLIB_IRQCHIP
> select IRQ_DOMAIN_HIERARCHY
> 
> see below.
> 
> (...)
>> +#include <linux/bitops.h>
> 
> Just <linux/bits.h> maybe, if you only use BIT() and GENMASK().
> 
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
> 
> Do not include these, just:
> #include <linux/gpio/driver.h>
> 
>> +#define BCM6328_DIROUT_REG     0x04
>> +#define BCM6328_DATA_REG       0x0c
>> +#define BCM6328_MODE_REG       0x18
> 
> This looks very much like it could use GPIO_REGMAP.
> Can you look at:
> drivers/gpio/gpio-regmap.c
> drivers/gpio/gpio-sl28cpld.c
> 
> And see if you can do what that driver is doing and reuse
> this core infrastructure?

I've just checked drivers/gpio/gpio-regmap.c and it seems that "struct 
gpio_regmap" should be declared in include/linux/gpio/regmap.h.
Right now devm_gpio_regmap_register() is returning a pointer to a 
structure which none except gpio-regmap knows. Does that make any sense?
I need to access gpio_regmap->gpio_chip in order to gather 
gpio_chip.base and pass it to pinctrl_add_gpio_range().

> 
>> +static inline unsigned int bcm6328_bank_pin(unsigned int pin)
>> +{
>> +       return pin % PINS_PER_BANK;
>> +}
> 
> I am generally reluctant about registering several banks/instances
> of the GPIO if it is possible to just use more devices in the
> device tree, like one for each instance.
> 
>> +static inline unsigned int bcm6328_reg_off(unsigned int reg, unsigned int pin)
>> +{
>> +       return reg - (pin / PINS_PER_BANK) * BANK_SIZE;
>> +}
> 
> Because it leads to this kind of weirdness to split out the devices
> from the main device in practice.
> 
>> +static int bcm6328_gpio_direction_input(struct gpio_chip *chip,
>> +                                       unsigned int pin)
>> +{
> (...)
>> +       /*
>> +        * Check with the pinctrl driver whether this pin is usable as
>> +        * an input GPIO
>> +        */
>> +       ret = pinctrl_gpio_direction_input(chip->base + pin);
>> +       if (ret)
>> +               return ret;
> 
> This is very nice.
> 
>> +static int bcm6328_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
>> +{
>> +       char irq_name[7];
>> +
>> +       sprintf(irq_name, "gpio%d", gpio);
>> +
>> +       return of_irq_get_byname(chip->of_node, irq_name);
>> +}
> 
> This is a clear indication that we are dealing with a hierarchical irqchip.
> 
> My assumption is that you have one IRQ per GPIO line, so each
> GPIO has a dedicated IRQ on the interrupt controller. Correct?
> 
> This means:
> 
> - Do not add all the interrupts into the device tree by name.
> 
> - In Kconfig select GPIOLIB_IRQCHIP, select IRQ_DOMAIN_HIERARCHY
> 
> - Populate a simple struct gpio_irq_chip, if no local registers need
>    updating on interrupts, just pass interrupts through
>          .irq_mask       = irq_chip_mask_parent,
>          .irq_unmask     = irq_chip_unmask_parent,
>    etc.
> 
> - Implement bcm6328_gpio_child_to_parent_hwirq() for this chip
>    with hardcoded mappings between the hardware GPIO and interrupt
>    lines, using the parent interrupt controller hierarchically. This mapping
>    is determined from the compatible-string, and part of the property
>    of how the GPIO block is integrated with the SoC. If need be to
>    tell different chips apart, more precise compatible strings are needed.
> 
> - Examples:
>    drivers/gpio/gpio-ixp4xx.c
>    drivers/gpio/gpio-sifive.c
> 
> If you do this you will notice the core is more helpful to cut down on the
> code.
> 
> Yours,
> Linus Walleij
> 

Best regards,
Álvaro.

Powered by blists - more mailing lists