| Message ID | 31e5a5aeb833c43c07daafcf939864497ff1c349.1616760183.git.sander@svanheule.net |
|---|---|
| State | New |
| Headers | show |
| Series | Add Realtek Otto GPIO support | expand |
On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net> wrote: > > Realtek MIPS SoCs (platform name Otto) have GPIO controllers with up to > 64 GPIOs, divided over two banks. Each bank has a set of registers for > 32 GPIOs, with support for edge-triggered interrupts. > > Each GPIO bank consists of four 8-bit GPIO ports (ABCD and EFGH). Most > registers pack one bit per GPIO, except for the IMR register, which > packs two bits per GPIO (AB-CD). > > Although the byte order is currently assumed to have port A..D at offset > 0x0..0x3, this has been observed to be reversed on other, Lexra-based, > SoCs (e.g. RTL8196E/97D/97F). > > Interrupt support is disabled for the fallback devicetree-compatible > 'realtek,otto-gpio'. This allows for quick support of GPIO banks in > which the byte order would be unknown. In this case, the port ordering > in the IMR registers may not match the reversed order in the other > registers (DCBA, and BA-DC or DC-BA). Thanks for the update! My comments below. ... > +config GPIO_REALTEK_OTTO > + bool "Realtek Otto GPIO support" Why not module? > + depends on MACH_REALTEK_RTL > + default MACH_REALTEK_RTL > + select GPIO_GENERIC > + select GPIOLIB_IRQCHIP > + help > + The GPIO controller on the Otto MIPS platform supports up to two > + banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs > + are grouped in four 8-bit wide ports. When allowing module build, here you may add what will be the name of it. ... > +/* > + * Total register block size is 0x1C for four ports. > + * On the RTL8380/RLT8390 platforms port A, B, and C are implemented. D? > + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H. > + * > + * Port information is stored with the first port at offset 0, followed by the > + * second, etc. Most registers store one bit per GPIO and should be read out in > + * reversed endian order. The two interrupt mask registers store two bits per > + * GPIO, and should be manipulated with swahw32, if required. > + */ ... > +/* Seems like kernel doc format with missed ** header and properly formed summary and description. > + * Realtek GPIO driver data > + * Because the interrupt mask register (IMR) combines the function of > + * IRQ type selection and masking, two extra values are stored. > + * intr_mask is used to mask/unmask the interrupts for certain GPIO, > + * and intr_type is used to store the selected interrupt types. The > + * logical AND of these values is written to IMR on changes. > + * > + * @gc Associated gpio_chip instance > + * @base Base address of the register block > + * @lock Lock for accessing the IRQ registers and values > + * @intr_mask Mask for GPIO interrupts > + * @intr_type GPIO interrupt type selection > + */ > +struct realtek_gpio_ctrl { > + struct gpio_chip gc; > + void __iomem *base; > + raw_spinlock_t lock; > + u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK]; > + u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK]; > +}; > + > +enum realtek_gpio_flags { > + GPIO_INTERRUPTS = BIT(0), > +}; ... > +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > + > + return container_of(gc, struct realtek_gpio_ctrl, gc); > +} > +static unsigned int line_to_port(unsigned int line) > +{ > + return line / 8; > +} > + > +static unsigned int line_to_port_pin(unsigned int line) > +{ > + return line % 8; > +} These are useless. Just use them inline. ... > +static u8 read_u8_reg(void __iomem *reg, unsigned int port) > +{ > + return ioread8(reg + port); > +} > + > +static void write_u8_reg(void __iomem *reg, unsigned int port, u8 value) > +{ > + iowrite8(value, reg + port); > +} > + > +static void write_u16_reg(void __iomem *reg, unsigned int port, u16 value) > +{ > + iowrite16(value, reg + 2 * port); > +} What's the point? You better provide a controller structure as a parameter. Look into other drivers. There are plenty of examples how to provide IO accessors in smarter way. ... > +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl, > + unsigned int port, u16 irq_type, u16 irq_mask) > +{ > + write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port, > + irq_type & irq_mask); Can be one line. > +} ... > + write_u8_reg(ctrl->base + REALTEK_GPIO_REG_ISR, line_to_port(line), > + BIT(line_to_port_pin(line))); line % 8 and line / 8 is much shorter. ANd then it becomes only one line. ... > +static int realtek_gpio_irq_set_type(struct irq_data *data, > + unsigned int flow_type) One line? ... > +static void realtek_gpio_irq_handler(struct irq_desc *desc) > +{ > + struct gpio_chip *gc = irq_desc_get_handler_data(desc); > + struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + struct irq_chip *irq_chip = irq_desc_get_chip(desc); > + void __iomem *reg_isr = ctrl->base + REALTEK_GPIO_REG_ISR; > + unsigned int lines_done; > + unsigned int port_pin_count; > + unsigned int port; > + unsigned int irq; > + int offset; > + unsigned long status; Rearrange them by swapping lines. > + chained_irq_enter(irq_chip, desc); > + > + for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) { > + port = line_to_port(lines_done); > + status = read_u8_reg(reg_isr, port); > + port_pin_count = min(gc->ngpio - lines_done, 8U); > + for_each_set_bit(offset, &status, port_pin_count) { > + irq = irq_find_mapping(gc->irq.domain, offset); > + generic_handle_irq(irq); > + write_u8_reg(reg_isr, port, BIT(offset)); Shouldn't it be in the ->irq_ack() callback? > + } > + } ... > +static const struct of_device_id realtek_gpio_of_match[] = { > + { .compatible = "realtek,otto-gpio" }, > + { > + .compatible = "realtek,rtl8380-gpio", > + .data = (void *)GPIO_INTERRUPTS Not sure why this flag is needed right now. Drop it completely for good. > + }, > + { > + .compatible = "realtek,rtl8390-gpio", > + .data = (void *)GPIO_INTERRUPTS Ditto. > + }, > + {} > +}; > + Extra blank line. > +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match); ... > + iowrite32(GENMASK(31, 0), ctrl->base + REALTEK_GPIO_REG_ISR); This one perhaps needs a comment like "cleaning all IRQ states". Note, we have a proper callback for this, i.e. hw_init. Consider to use it. ... > +}; > + Extra blank line. > +builtin_platform_driver(realtek_gpio_driver); ... So, looking into the code, I think you may easily get rid of 30-50 LOCs. So, expecting <= 300 LOCs in v5.
Hi Andy, Replies inline below. On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote: > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net> > wrote: > > > +config GPIO_REALTEK_OTTO > > + bool "Realtek Otto GPIO support" > > Why not module? This driver is only useful on a few specific MIPS SoCs, where this GPIO peripheral is a part of that SoC. What would be the point of providing this driver as a module? > > > + depends on MACH_REALTEK_RTL > > + default MACH_REALTEK_RTL > > + select GPIO_GENERIC > > + select GPIOLIB_IRQCHIP > > > + help > > + The GPIO controller on the Otto MIPS platform supports up > > to two > > + banks of 32 GPIOs, with edge triggered interrupts. The 32 > > GPIOs > > + are grouped in four 8-bit wide ports. > > When allowing module build, here you may add what will be the name of > it. > > ... > > > +/* > > + * Total register block size is 0x1C for four ports. > > + * On the RTL8380/RLT8390 platforms port A, B, and C are > > implemented. > > D? No port D on 8380/8390. Only 24 GPIO lines are present on these platforms. I'll rephrase this comment. > > > + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, > > and H. > > + * > > + * Port information is stored with the first port at offset 0, > > followed by the > > + * second, etc. Most registers store one bit per GPIO and should be > > read out in > > + * reversed endian order. The two interrupt mask registers store two > > bits per > > + * GPIO, and should be manipulated with swahw32, if required. > > + */ This reference to swahw32 and the include of linux/swab.h will be dropped. > > > +/* > > Seems like kernel doc format with missed ** header and properly formed > summary and description. I'll reformat. > > > + * Realtek GPIO driver data > > + * Because the interrupt mask register (IMR) combines the function > > of > > + * IRQ type selection and masking, two extra values are stored. > > + * intr_mask is used to mask/unmask the interrupts for certain > > GPIO, > > + * and intr_type is used to store the selected interrupt types. > > The > > + * logical AND of these values is written to IMR on changes. > > + * > > + * @gc Associated gpio_chip instance > > + * @base Base address of the register block > > + * @lock Lock for accessing the IRQ registers and values > > + * @intr_mask Mask for GPIO interrupts > > + * @intr_type GPIO interrupt type selection > > + */ > > +struct realtek_gpio_ctrl { > > + struct gpio_chip gc; > > + void __iomem *base; > > + raw_spinlock_t lock; > > + u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK]; > > + u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK]; > > +}; > > + > > +enum realtek_gpio_flags { > > + GPIO_INTERRUPTS = BIT(0), > > +}; > > ... See below. I'll add a comment. > > > +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data > > *data) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > > + > > + return container_of(gc, struct realtek_gpio_ctrl, gc); > > +} > > > +static unsigned int line_to_port(unsigned int line) > > +{ > > + return line / 8; > > +} > > + > > +static unsigned int line_to_port_pin(unsigned int line) > > +{ > > + return line % 8; > > +} > > These are useless. Just use them inline. I added these as the alternative of the /16 and %16 I had for the IMR offsets in v2. The function names tell the reader _why_ I'm doing the division and modulo operations, but I guess a properly named variable would do the same. > > > +static u8 read_u8_reg(void __iomem *reg, unsigned int port) > > +{ > > + return ioread8(reg + port); > > +} > > + > > +static void write_u8_reg(void __iomem *reg, unsigned int port, u8 > > value) > > +{ > > + iowrite8(value, reg + port); > > +} > > + > > +static void write_u16_reg(void __iomem *reg, unsigned int port, u16 > > value) > > +{ > > + iowrite16(value, reg + 2 * port); > > +} > > What's the point? You better provide a controller structure as a > parameter. Look into other drivers. There are plenty of examples how > to provide IO accessors in smarter way. Since these are currently only really used for IMR and ISR, I'll fold them into their accessor functions for v5. > > > +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl, > > + unsigned int port, u16 irq_type, u16 irq_mask) > > +{ > > + write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port, > > + irq_type & irq_mask); > > Can be one line. > > > +} > > ... > > > +static int realtek_gpio_irq_set_type(struct irq_data *data, > > + unsigned int flow_type) > > One line? I thought checkpatch.pl would complain, but it doesn't. Folded onto one line. > > + chained_irq_enter(irq_chip, desc); > > + > > + for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) > > { > > + port = line_to_port(lines_done); > > + status = read_u8_reg(reg_isr, port); > > + port_pin_count = min(gc->ngpio - lines_done, 8U); > > + for_each_set_bit(offset, &status, port_pin_count) { > > + irq = irq_find_mapping(gc->irq.domain, > > offset); > > + generic_handle_irq(irq); > > > + write_u8_reg(reg_isr, port, BIT(offset)); > > Shouldn't it be in the ->irq_ack() callback? I think I added this line to deal with handle_bad_irq during development. Like you say, handle_edge_irq() has it's specific ACK logic, so this is probably even a bug. Will be removed. > > > + } > > + } > > ... > > > +static const struct of_device_id realtek_gpio_of_match[] = { > > + { .compatible = "realtek,otto-gpio" }, > > + { > > + .compatible = "realtek,rtl8380-gpio", > > + .data = (void *)GPIO_INTERRUPTS > > Not sure why this flag is needed right now. Drop it completely for > good. > > + }, > > + { > > + .compatible = "realtek,rtl8390-gpio", > > + .data = (void *)GPIO_INTERRUPTS > > Ditto Linus Walleij asked this question too after v1: https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/ Note that the fall-back compatible doesn't have this flag set. > . > > > + }, > > + {} > > +}; > > > + > > Extra blank line. Add or drop? I see other drivers using no empty line between the of_match table and the MODULE_DEVICE_TABLE macro. > > > +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match); > > > ... > > > + iowrite32(GENMASK(31, 0), ctrl->base + > > REALTEK_GPIO_REG_ISR); > > This one perhaps needs a comment like "cleaning all IRQ states". > Note, we have a proper callback for this, i.e. hw_init. Consider to use > it. Which "hw_init" are you referring too? I can't really find much, aside from drivers implementing it themselves to differentiate between driver and hardware set-up. Since this is normally only called once, I can turn it into the more readable: for (port = 0; (port * 8) < ngpios; port++) { realtek_gpio_write_imr(ctrl, port, 0, 0); realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0)); } > > > +}; > > > + > > Extra blank line. I see the same use of one blank line in other drivers. > > +builtin_platform_driver(realtek_gpio_driver); > > ... > > So, looking into the code, I think you may easily get rid of 30-50 > LOCs. > So, expecting <= 300 LOCs in v5. After trimming the file, sloccount puts me at 224, but the total line count is still 310. :-) Best, Sander
On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <sander@svanheule.net> wrote: > On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote: > > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@svanheule.net> > > wrote: ... > > > + bool "Realtek Otto GPIO support" > > > > Why not module? > > This driver is only useful on a few specific MIPS SoCs, where this GPIO > peripheral is a part of that SoC. What would be the point of providing > this driver as a module? If it's not critical for boot this makes the kernel smaller and loads modules only on demand. Also, (the main part) it allows to build multi-target kernels which are in general smaller. That said, you must provide quite a good justification why it's *not* a module. Otherwise, fix the Kconfig and code accordingly. ... > > > +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data > > > *data) > > > +{ > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); > > > + > > > + return container_of(gc, struct realtek_gpio_ctrl, gc); > > > +} > > > > > +static unsigned int line_to_port(unsigned int line) > > > +{ > > > + return line / 8; > > > +} > > > + > > > +static unsigned int line_to_port_pin(unsigned int line) > > > +{ > > > + return line % 8; > > > +} > > > > These are useless. Just use them inline. > > I added these as the alternative of the /16 and %16 I had for the IMR > offsets in v2. The function names tell the reader _why_ I'm doing the > division and modulo operations, but I guess a properly named variable > would do the same. Exactly! So, please use better variable names on stack. ... > > > +static const struct of_device_id realtek_gpio_of_match[] = { > > > + { .compatible = "realtek,otto-gpio" }, > > > + { > > > + .compatible = "realtek,rtl8380-gpio", > > > + .data = (void *)GPIO_INTERRUPTS > > > > Not sure why this flag is needed right now. Drop it completely for > > good. > > > + }, > > > + { > > > + .compatible = "realtek,rtl8390-gpio", > > > + .data = (void *)GPIO_INTERRUPTS > > > > Ditto > > Linus Walleij asked this question too after v1: > https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/ > > Note that the fall-back compatible doesn't have this flag set. AFAICS all, except one have this flag, I suggest you to do other way around, i.e. check compatible string in the code. Or do something more clever. What happens if you have this flag enabled for the fallback node? If two people ask the same, it might be a smoking gun. ... > > > +}; > > > > > + > > > > Extra blank line. > > Add or drop? What do you think? :-) > I see other drivers using no empty line between the > of_match table and the MODULE_DEVICE_TABLE macro. Yep, this is not a competition on amount of LOCs, actually, less LOCs is better, if it keeps the same level of readability and maintainability, ... > > > + iowrite32(GENMASK(31, 0), ctrl->base + > > > REALTEK_GPIO_REG_ISR); > > > > This one perhaps needs a comment like "cleaning all IRQ states". > > Note, we have a proper callback for this, i.e. hw_init. Consider to use > > it. > > Which "hw_init" are you referring too? I can't really find much, aside > from drivers implementing it themselves to differentiate between driver > and hardware set-up. > > Since this is normally only called once, I can turn it into the more > readable: > for (port = 0; (port * 8) < ngpios; port++) { > realtek_gpio_write_imr(ctrl, port, 0, 0); > realtek_gpio_clear_isr(ctrl, port, GENMASK(7, 0)); > } Good and move it to the callback. ->init_hw() in GPIO IRQ chip data structure. ... > > > +}; > > > > > + > > > > Extra blank line. > > I see the same use of one blank line in other drivers. Same as above. > > > +builtin_platform_driver(realtek_gpio_driver); ... > > So, looking into the code, I think you may easily get rid of 30-50 > > LOCs. > > So, expecting <= 300 LOCs in v5. > > After trimming the file, sloccount puts me at 224, but the total line > count is still 310. :-) I was referring to the LOCs, i.o.w. real code with comments :-) -- With Best Regards, Andy Shevchenko
Hi Andy, Thank you for clarifying your remarks. I'll support for building as a module, and have implemented the gpio_irq_chip->init_hw() callback. On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote: > On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule < > sander@svanheule.net> wrote: > > On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote: > > > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule < > > > sander@svanheule.net> > > > wrote: > > > > +static const struct of_device_id realtek_gpio_of_match[] = { > > > > + { .compatible = "realtek,otto-gpio" }, > > > > + { > > > > + .compatible = "realtek,rtl8380-gpio", > > > > + .data = (void *)GPIO_INTERRUPTS > > > > > > Not sure why this flag is needed right now. Drop it completely for > > > good. > > > > + }, > > > > + { > > > > + .compatible = "realtek,rtl8390-gpio", > > > > + .data = (void *)GPIO_INTERRUPTS > > > > > > Ditto > > > > Linus Walleij asked this question too after v1: > > https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/ > > > > Note that the fall-back compatible doesn't have this flag set. > > AFAICS all, except one have this flag, I suggest you to do other way > around, i.e. check compatible string in the code. Or do something more > clever. What happens if you have this flag enabled for the fallback > node? > > If two people ask the same, it might be a smoking gun. > Testing for the fallback wouldn't work, since of_device_is_compatible() would always match. Setting the (inverse) flag only on the fallback would indeed reduce the clutter. If the port order is reversed w.r.t. to the current implementation, enabling a GPIO+IRQ would enable the same pin on a different port. I don't think the result would be catastrophical, but it would result in unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts would actually come from C0, and vice versa. Intended port | A | B | C | D -----------------+---+---+---+--- Actual GPIO port | D | C | B | A Actual IRQ port | B | A | D | C If only the actual GPIO ports change, at least you can still use a modified GPIO line number and polling. The user could just leave out the optional irq-controller from the devicetree, but I would rather have it enforced in some way. Best, Sander
On Mon, Mar 29, 2021 at 8:28 PM Sander Vanheule <sander@svanheule.net> wrote: > On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote: > > On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule < > > sander@svanheule.net> wrote: ... > > AFAICS all, except one have this flag, I suggest you to do other way > > around, i.e. check compatible string in the code. Or do something more > > clever. What happens if you have this flag enabled for the fallback > > node? > > > > If two people ask the same, it might be a smoking gun. > > > > Testing for the fallback wouldn't work, since of_device_is_compatible() > would always match. Setting the (inverse) flag only on the fallback > would indeed reduce the clutter. > > If the port order is reversed w.r.t. to the current implementation, > enabling a GPIO+IRQ would enable the same pin on a different port. I > don't think the result would be catastrophical, but it would result in > unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts > would actually come from C0, and vice versa. > > Intended port | A | B | C | D > -----------------+---+---+---+--- > Actual GPIO port | D | C | B | A > Actual IRQ port | B | A | D | C > > If only the actual GPIO ports change, at least you can still use a > modified GPIO line number and polling. The user could just leave out > the optional irq-controller from the devicetree, but I would rather > have it enforced in some way. OK! Thanks for clarification.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec4c2e8..d3be17812f94 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -502,6 +502,17 @@ config GPIO_RDA help Say Y here to support RDA Micro GPIO controller. +config GPIO_REALTEK_OTTO + bool "Realtek Otto GPIO support" + depends on MACH_REALTEK_RTL + default MACH_REALTEK_RTL + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + help + The GPIO controller on the Otto MIPS platform supports up to two + banks of 32 GPIOs, with edge triggered interrupts. The 32 GPIOs + are grouped in four 8-bit wide ports. + config GPIO_REG bool help diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c58a90a3c3b1..8ace5934e3c3 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -124,6 +124,7 @@ obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o obj-$(CONFIG_GPIO_RDA) += gpio-rda.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o +obj-$(CONFIG_GPIO_REALTEK_OTTO) += gpio-realtek-otto.o obj-$(CONFIG_GPIO_REG) += gpio-reg.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c new file mode 100644 index 000000000000..07641a1686eb --- /dev/null +++ b/drivers/gpio/gpio-realtek-otto.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/gpio/driver.h> +#include <linux/irq.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/swab.h> + +/* + * Total register block size is 0x1C for four ports. + * On the RTL8380/RLT8390 platforms port A, B, and C are implemented. + * RTL8389 and RTL8328 implement a second bank with ports E, F, G, and H. + * + * Port information is stored with the first port at offset 0, followed by the + * second, etc. Most registers store one bit per GPIO and should be read out in + * reversed endian order. The two interrupt mask registers store two bits per + * GPIO, and should be manipulated with swahw32, if required. + */ + +/* + * Pin select: (0) "normal", (1) "dedicate peripheral" + * Not used on RTL8380/RTL8390, peripheral selection is managed by control bits + * in the peripheral registers. + */ +#define REALTEK_GPIO_REG_CNR 0x00 +/* Clear bit (0) for input, set bit (1) for output */ +#define REALTEK_GPIO_REG_DIR 0x08 +#define REALTEK_GPIO_REG_DATA 0x0C +/* Read bit for IRQ status, write 1 to clear IRQ */ +#define REALTEK_GPIO_REG_ISR 0x10 +/* Two bits per GPIO in IMR registers */ +#define REALTEK_GPIO_REG_IMR 0x14 +#define REALTEK_GPIO_REG_IMR_AB 0x14 +#define REALTEK_GPIO_REG_IMR_CD 0x18 +#define REALTEK_GPIO_IMR_LINE_MASK GENMASK(1, 0) +#define REALTEK_GPIO_IRQ_EDGE_FALLING 1 +#define REALTEK_GPIO_IRQ_EDGE_RISING 2 +#define REALTEK_GPIO_IRQ_EDGE_BOTH 3 + +#define REALTEK_GPIO_MAX 32 +#define REALTEK_GPIO_PORTS_PER_BANK 4 + +/* + * Realtek GPIO driver data + * Because the interrupt mask register (IMR) combines the function of + * IRQ type selection and masking, two extra values are stored. + * intr_mask is used to mask/unmask the interrupts for certain GPIO, + * and intr_type is used to store the selected interrupt types. The + * logical AND of these values is written to IMR on changes. + * + * @gc Associated gpio_chip instance + * @base Base address of the register block + * @lock Lock for accessing the IRQ registers and values + * @intr_mask Mask for GPIO interrupts + * @intr_type GPIO interrupt type selection + */ +struct realtek_gpio_ctrl { + struct gpio_chip gc; + void __iomem *base; + raw_spinlock_t lock; + u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK]; + u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK]; +}; + +enum realtek_gpio_flags { + GPIO_INTERRUPTS = BIT(0), +}; + +static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + + return container_of(gc, struct realtek_gpio_ctrl, gc); +} + +static unsigned int line_to_port(unsigned int line) +{ + return line / 8; +} + +static unsigned int line_to_port_pin(unsigned int line) +{ + return line % 8; +} + +static u8 read_u8_reg(void __iomem *reg, unsigned int port) +{ + return ioread8(reg + port); +} + +static void write_u8_reg(void __iomem *reg, unsigned int port, u8 value) +{ + iowrite8(value, reg + port); +} + +static void write_u16_reg(void __iomem *reg, unsigned int port, u16 value) +{ + iowrite16(value, reg + 2 * port); +} + +/* + * Since the IMRs contain two bits per GPIO, only 16 GPIO lines fit in a 32-bit + * register. Use realtek_gpio_imr_bits() to put the GPIO line's new value in + * the right place. + */ +static u16 realtek_gpio_imr_bits(unsigned int pin, u16 value) +{ + return (value & REALTEK_GPIO_IMR_LINE_MASK) << 2 * pin; +} + +static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl, + unsigned int port, u16 irq_type, u16 irq_mask) +{ + write_u16_reg(ctrl->base + REALTEK_GPIO_REG_IMR, port, + irq_type & irq_mask); +} + +static void realtek_gpio_irq_ack(struct irq_data *data) +{ + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data); + irq_hw_number_t line = irqd_to_hwirq(data); + + write_u8_reg(ctrl->base + REALTEK_GPIO_REG_ISR, line_to_port(line), + BIT(line_to_port_pin(line))); +} + +static void realtek_gpio_irq_unmask(struct irq_data *data) +{ + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data); + unsigned int line = irqd_to_hwirq(data); + unsigned int port = line_to_port(line); + unsigned int pin = line_to_port_pin(line); + unsigned long flags; + u16 m; + + raw_spin_lock_irqsave(&ctrl->lock, flags); + m = ctrl->intr_mask[port]; + m |= realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK); + ctrl->intr_mask[port] = m; + realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m); + raw_spin_unlock_irqrestore(&ctrl->lock, flags); +} + +static void realtek_gpio_irq_mask(struct irq_data *data) +{ + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data); + unsigned int line = irqd_to_hwirq(data); + unsigned int port = line_to_port(line); + unsigned int pin = line_to_port_pin(line); + unsigned long flags; + u16 m; + + raw_spin_lock_irqsave(&ctrl->lock, flags); + m = ctrl->intr_mask[port]; + m &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK); + ctrl->intr_mask[port] = m; + realtek_gpio_write_imr(ctrl, port, ctrl->intr_type[port], m); + raw_spin_unlock_irqrestore(&ctrl->lock, flags); +} + +static int realtek_gpio_irq_set_type(struct irq_data *data, + unsigned int flow_type) +{ + struct realtek_gpio_ctrl *ctrl = irq_data_to_ctrl(data); + unsigned int line = irqd_to_hwirq(data); + unsigned int port = line_to_port(line); + unsigned int pin = line_to_port_pin(line); + unsigned long flags; + u16 type, t; + + switch (flow_type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_FALLING: + type = REALTEK_GPIO_IRQ_EDGE_FALLING; + break; + case IRQ_TYPE_EDGE_RISING: + type = REALTEK_GPIO_IRQ_EDGE_RISING; + break; + case IRQ_TYPE_EDGE_BOTH: + type = REALTEK_GPIO_IRQ_EDGE_BOTH; + break; + default: + return -EINVAL; + } + + irq_set_handler_locked(data, handle_edge_irq); + + raw_spin_lock_irqsave(&ctrl->lock, flags); + t = ctrl->intr_type[port]; + t &= ~realtek_gpio_imr_bits(pin, REALTEK_GPIO_IMR_LINE_MASK); + t |= realtek_gpio_imr_bits(pin, type); + ctrl->intr_type[port] = t; + realtek_gpio_write_imr(ctrl, port, t, ctrl->intr_mask[port]); + raw_spin_unlock_irqrestore(&ctrl->lock, flags); + + return 0; +} + +static void realtek_gpio_irq_handler(struct irq_desc *desc) +{ + struct gpio_chip *gc = irq_desc_get_handler_data(desc); + struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc); + struct irq_chip *irq_chip = irq_desc_get_chip(desc); + void __iomem *reg_isr = ctrl->base + REALTEK_GPIO_REG_ISR; + unsigned int lines_done; + unsigned int port_pin_count; + unsigned int port; + unsigned int irq; + int offset; + unsigned long status; + + chained_irq_enter(irq_chip, desc); + + for (lines_done = 0; lines_done < gc->ngpio; lines_done += 8) { + port = line_to_port(lines_done); + status = read_u8_reg(reg_isr, port); + port_pin_count = min(gc->ngpio - lines_done, 8U); + for_each_set_bit(offset, &status, port_pin_count) { + irq = irq_find_mapping(gc->irq.domain, offset); + generic_handle_irq(irq); + write_u8_reg(reg_isr, port, BIT(offset)); + } + } + + chained_irq_exit(irq_chip, desc); +} + +static struct irq_chip realtek_gpio_irq_chip = { + .name = "realtek-otto-gpio", + .irq_ack = realtek_gpio_irq_ack, + .irq_mask = realtek_gpio_irq_mask, + .irq_unmask = realtek_gpio_irq_unmask, + .irq_set_type = realtek_gpio_irq_set_type, +}; + +static const struct of_device_id realtek_gpio_of_match[] = { + { .compatible = "realtek,otto-gpio" }, + { + .compatible = "realtek,rtl8380-gpio", + .data = (void *)GPIO_INTERRUPTS + }, + { + .compatible = "realtek,rtl8390-gpio", + .data = (void *)GPIO_INTERRUPTS + }, + {} +}; + +MODULE_DEVICE_TABLE(of, realtek_gpio_of_match); + +static int realtek_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + unsigned int dev_flags; + struct gpio_irq_chip *girq; + struct realtek_gpio_ctrl *ctrl; + u32 ngpios; + int err, irq; + + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); + if (!ctrl) + return -ENOMEM; + + dev_flags = (unsigned int) device_get_match_data(dev); + + if (device_property_read_u32(dev, "ngpios", &ngpios)) + ngpios = REALTEK_GPIO_MAX; + + if (ngpios > REALTEK_GPIO_MAX) { + dev_err(&pdev->dev, "invalid ngpios (max. %d)\n", + REALTEK_GPIO_MAX); + return -EINVAL; + } + + ctrl->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(ctrl->base)) + return PTR_ERR(ctrl->base); + + raw_spin_lock_init(&ctrl->lock); + + err = bgpio_init(&ctrl->gc, dev, 4, + ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL, + ctrl->base + REALTEK_GPIO_REG_DIR, NULL, + BGPIOF_BIG_ENDIAN_BYTE_ORDER); + if (err) { + dev_err(dev, "unable to init generic GPIO"); + return err; + } + + ctrl->gc.ngpio = ngpios; + ctrl->gc.owner = THIS_MODULE; + + irq = platform_get_irq_optional(pdev, 0); + if ((dev_flags & GPIO_INTERRUPTS) && irq > 0) { + girq = &ctrl->gc.irq; + girq->chip = &realtek_gpio_irq_chip; + girq->parent_handler = realtek_gpio_irq_handler; + girq->num_parents = 1; + girq->parents = devm_kcalloc(dev, girq->num_parents, + sizeof(*girq->parents), GFP_KERNEL); + if (!girq->parents) + return -ENOMEM; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_bad_irq; + girq->parents[0] = irq; + + /* Disable and clear all interrupts */ + iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_AB); + iowrite32(0, ctrl->base + REALTEK_GPIO_REG_IMR_CD); + iowrite32(GENMASK(31, 0), ctrl->base + REALTEK_GPIO_REG_ISR); + } + + return gpiochip_add_data(&ctrl->gc, ctrl); +} + +static struct platform_driver realtek_gpio_driver = { + .driver = { + .name = "realtek-otto-gpio", + .of_match_table = realtek_gpio_of_match, + }, + .probe = realtek_gpio_probe, +}; + +builtin_platform_driver(realtek_gpio_driver); + +MODULE_DESCRIPTION("Realtek Otto GPIO support"); +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>"); +MODULE_LICENSE("GPL v2");