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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx/
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
