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 [off-list ref]
wrote:

quoted

+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?

quoted

+       depends on MACH_REALTEK_RTL
+       default MACH_REALTEK_RTL
+       select GPIO_GENERIC
+       select GPIOLIB_IRQCHIP

quoted

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

...

quoted

+/*
+ * 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.

quoted

+ * 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.

quoted

+/*

Seems like kernel doc format with missed ** header and properly formed
summary and description.

I'll reformat.

quoted

+ * 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.

quoted

+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);
+}

quoted

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

quoted

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

quoted

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

quoted

+}

...

quoted

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

quoted

+       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);

quoted

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

quoted

+               }
+       }

...

quoted

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

quoted

+       },
+       {
+               .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/ (local)

Note that the fall-back compatible doesn't have this flag set.

.

quoted

+       },
+       {}
+};

quoted

+

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.

quoted

+MODULE_DEVICE_TABLE(of, realtek_gpio_of_match);

...

quoted

+               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));
        }

quoted

+};

quoted

+

Extra blank line.

I see the same use of one blank line in other drivers.

quoted

+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