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@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 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.
