Hi Álvaro,
thanks for your patch!
On Thu, Feb 25, 2021 at 5:42 PM Álvaro Fernández Rojas
<noltari@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx>
Thanks for working on this. This SoC definitely need to come upstream.
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?
> +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
