On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule [off-list ref] 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.

-- 
With Best Regards,
Andy Shevchenko