On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule [off-list ref] wrote:

On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:

quoted

On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule [off-list ref]
wrote:

...

quoted

quoted

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

...

quoted

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.

Exactly! So, please use better variable names on stack.

...

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.

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.

...

quoted

quoted

+};

quoted

+

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,

...

quoted

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

Good and move it to the callback.

->init_hw() in GPIO IRQ chip data structure.

...

quoted

quoted

+};

quoted

+

Extra blank line.

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

Same as above.

quoted

quoted

+builtin_platform_driver(realtek_gpio_driver);

...

quoted

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