Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.
From: Andy Shevchenko
Date: Thu Dec 07 2023 - 08:45:27 EST
On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
> From: Tzuyi Chang <tychang@xxxxxxxxxxx>
>
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.
Why gpio-regmap can't be used?
...
> +struct rtd_gpio_info {
> + u8 *dir_offset;
> + u8 num_dir;
> + u8 *dato_offset;
> + u8 num_dato;
> + u8 *dati_offset;
> + u8 num_dati;
> + u8 *ie_offset;
> + u8 num_ie;
> + u8 *dp_offset;
> + u8 num_dp;
> + u8 *gpa_offset;
> + u8 num_gpa;
> + u8 *gpda_offset;
> + u8 num_gpda;
> + u8 *deb_offset;
> + u8 num_deb;
A lot of wasted space. Can you group pointers followed by u8 members?
Note, use `pahole` tool to check the struct layout in C.
> +};
...
> +struct rtd_gpio {
> + struct platform_device *pdev;
Why
struct device *dev;
is not suffice?
> + const struct rtd_gpio_info *info;
> + void __iomem *base;
> + void __iomem *irq_base;
> + struct gpio_chip gpio_chip;
Make this to be the first member, it might reduce some code
(due to pointer arithmetics).
> + unsigned int irqs[2];
> + spinlock_t lock;
> +};
> +
> +
One blank line is enough.
...
> +static const struct rtd_gpio_info rtd_iso_gpio_info = {
> + .name = "rtd_iso_gpio",
> + .type = RTD_ISO_GPIO,
> + .gpio_base = 0,
> + .num_gpios = 82,
> + .dir_offset = (u8 []){ 0x0, 0x18, 0x2c },
> + .num_dir = 3,
> + .dato_offset = (u8 []){ 0x4, 0x1c, 0x30 },
> + .num_dato = 3,
> + .dati_offset = (u8 []){ 0x8, 0x20, 0x34 },
> + .num_dati = 3,
> + .ie_offset = (u8 []){ 0xc, 0x24, 0x38 },
> + .num_ie = 3,
> + .dp_offset = (u8 []){ 0x10, 0x28, 0x3c },
> + .num_dp = 3,
> + .gpa_offset = (u8 []){ 0x8, 0xe0, 0x90 },
> + .num_gpa = 3,
> + .gpda_offset = (u8 []){ 0xc, 0xe4, 0x94 },
> + .num_gpda = 3,
> + .deb_offset = (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
> + 0x60, 0x64, 0x68, 0x6c },
> + .num_deb = 11,
Use ARRAY_SIZE() from array_size.h for all num_* assignments.
> +};
...
> +static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
Ditto.
> +};
...
> +static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
Ditto.
> +};
> +
> +static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
Ditto.
> +};
> +
> +static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
Ditto.
> +};
...
> +static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> + int index = offset / 32;
> + if (index > data->info->num_dir)
> + return -EINVAL;
When this conditional can be true?
Same Q to the similar checks over the code.
> + return data->info->dir_offset[index];
> +}
...
> + if (data->info->type == RTD1295_ISO_GPIO) {
> + reg_offset = rtd_gpio_deb_offset(data, 0);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = 0;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + } else if (data->info->type == RTD1295_MISC_GPIO) {
> + reg_offset = rtd_gpio_deb_offset(data, 0);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = (offset >> 4) * 4;
> + deb_val += 1;
> + write_en = BIT(shift + 3);
> + } else {
> + reg_offset = rtd_gpio_deb_offset(data, offset);
> + if (reg_offset < 0)
> + return reg_offset;
> + shift = (offset % 8) * 4;
> + write_en = BIT(shift + 3);
> + }
You should probably have kind of chip_info constant structure that goes via
driver_data and will have a callback, so, here you will call one and get all
three at once:
- register offset;
- shift
- updated debounce value
...
> +static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rtd_gpio *data = gpiochip_get_data(chip);
> + unsigned long flags;
> + int reg_offset;
> + u32 val;
> +
> + reg_offset = rtd_gpio_dir_offset(data, offset);
> + if (reg_offset < 0)
> + return reg_offset;
> + spin_lock_irqsave(&data->lock, flags);
So, is your IRQ chip going to work with CONFIG_PREEMT_RT?
> + val = readl_relaxed(data->base + reg_offset);
> + val &= BIT(offset % 32);
Why this is is under lock?
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}
...
> +static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned int offset, bool out)
> +{
> + unsigned long flags;
> + spin_lock_irqsave(&data->lock, flags);
> + spin_unlock_irqrestore(&data->lock, flags);
Consider to utilise guard() / scoped_guard() from cleanup.h.
> +}
...
> +static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> + chip->set(chip, offset, value);
Why? Can't you call the function by its name directly?
> +
> + return rtd_gpio_set_direction(chip, offset, true);
> +}
...
> +static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct rtd_gpio *data = gpiochip_get_data(chip);
> + int dir_reg_offset, dat_reg_offset;
> + unsigned long flags;
> + u32 val;
> +
> + dir_reg_offset = rtd_gpio_dir_offset(data, offset);
> + if (dir_reg_offset < 0)
> + return dir_reg_offset;
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + val = readl_relaxed(data->base + dir_reg_offset);
> + val &= BIT(offset % 32);
> + dat_reg_offset = val ?
> + rtd_gpio_dato_offset(data, offset) : rtd_gpio_dati_offset(data, offset);
Can't you have the direction be cached and already know which offset to use
even before the lock?
> + val = readl_relaxed(data->base + dat_reg_offset);
> + val >>= offset % 32;
> + val &= 0x1;
Why were these operations done under the lock?
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + return val;
> +}
...
> +static void rtd_gpio_irq_handle(struct irq_desc *desc)
> +{
> + int (*get_reg_offset)(struct rtd_gpio *gpio, unsigned int offset);
> + struct rtd_gpio *data = irq_desc_get_handler_data(desc);
> + struct irq_domain *domain = data->gpio_chip.irq.domain;
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned int irq = irq_desc_get_irq(desc);
> + int reg_offset;
> + u32 status;
> + int hwirq;
Why signed?
> + int i;
> + int j;
> +
> + chained_irq_enter(chip, desc);
> + if (irq == data->irqs[0])
> + get_reg_offset = &rtd_gpio_gpa_offset;
> + else if (irq == data->irqs[1])
> + get_reg_offset = &rtd_gpio_gpda_offset;
Can't it be done before entering into chained IRQ handler?
> + for (i = 0; i < data->info->num_gpios; i = i + 31) {
31 ?! In any case i += 31 is simply shorter.
> + reg_offset = get_reg_offset(data, i);
> + if (reg_offset < 0)
> + return;
> +
> + status = readl_relaxed(data->irq_base + reg_offset) >> 1;
> + writel_relaxed(status << 1, data->irq_base + reg_offset);
> + while (status) {
> + j = __ffs(status);
> + status &= ~BIT(j);
NIH for_each_set_bit()
> + hwirq = i + j;
> + if (rtd_gpio_check_ie(data, hwirq)) {
> + int girq = irq_find_mapping(domain, hwirq);
> + u32 irq_type = irq_get_trigger_type(girq);
> +
> + if ((irq == data->irqs[1]) && ((irq_type & IRQ_TYPE_SENSE_MASK) !=
> + IRQ_TYPE_EDGE_BOTH))
> + break;
> + generic_handle_irq(girq);
Why you can't use generic_handle_domain_irq()?
> + }
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
...
> + u32 mask = BIT(d->hwirq % 32);
Use proper type and getter for hwirq. It's mentioned in the Documentation.
...
> +static const struct irq_chip rtd_gpio_irq_chip = {
> + .name = "rtd-gpio",
> + .irq_enable = rtd_gpio_enable_irq,
> + .irq_disable = rtd_gpio_disable_irq,
> + .irq_set_type = rtd_gpio_irq_set_type,
> + .flags = IRQCHIP_IMMUTABLE,
Is it? You seems missed something to fulfill the immutability requirements.
Please consult with the Documentation, it's all written there.
> +};
...
> +static const struct of_device_id rtd_gpio_of_matches[] = {
> + { .compatible = "realtek,rtd1295-misc-gpio", .data = &rtd1295_misc_gpio_info },
> + { .compatible = "realtek,rtd1295-iso-gpio", .data = &rtd1295_iso_gpio_info },
> + { .compatible = "realtek,rtd1395-iso-gpio", .data = &rtd1395_iso_gpio_info },
> + { .compatible = "realtek,rtd1619-iso-gpio", .data = &rtd1619_iso_gpio_info },
> + { .compatible = "realtek,rtd1319-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1619b-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1319d-iso-gpio", .data = &rtd_iso_gpio_info },
> + { .compatible = "realtek,rtd1315e-iso-gpio", .data = &rtd_iso_gpio_info },
> + { },
No comma in the terminator entry.
> +};
> +MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
Move all these closer to its user (struct platform_device below).
...
> + data->gpio_chip.label = dev_name(&pdev->dev);
dev
...
> + data->gpio_chip.fwnode = dev_fwnode(&pdev->dev);
dev
But why setting parent device is not suffice?
...
> +static int rtd_gpio_init(void)
> +{
> + return platform_driver_register(&rtd_gpio_platform_driver);
> +}
> +
Unneeded blank line.
> +subsys_initcall(rtd_gpio_init);
Why? Anything that is not on standard initcall must be justified.
--
With Best Regards,
Andy Shevchenko