| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Vdi06dLxJNCo4f1CA=cS1MuPwG0nEAnVqt8BRrz9bnOtw@mail.gmail.com>
Date: Fri, 26 Mar 2021 20:19:13 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Sander Vanheule <sander@...nheule.net>
Cc: devicetree <devicetree@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Bert Vermeulen <bert@...t.com>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Linus Walleij <linus.walleij@...aro.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Marc Zyngier <maz@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support
On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <sander@...nheule.net> 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
Powered by blists - more mailing lists