Messages in this thread | | | Subject | Re: [PATCH 02/12] pinctrl: add a pincontrol driver for BCM6328 | From | Álvaro Fernández Rojas <> | Date | Tue, 2 Mar 2021 17:59:03 +0100 |
| |
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@gmail.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@gmail.com> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.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.
| |