Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support

[Sander Vanheule <sander@svanheule.net>]

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