RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

["TY_Chang[張子逸]" <tychang@realtek.com>]

Hi Andy,

Thank you for the review.

>On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
>> From: Tzuyi Chang <tychang@realtek.com>
>>
>> This driver enables configuration of GPIO direction, GPIO values, GPIO
>> debounce settings and handles GPIO interrupts.
>
>Why gpio-regmap can't be used?
>

I will try to use gpio-remap in the next version.

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

I will revise it in the next version.

>> +};
>
>...
>
>> +struct rtd_gpio {
>> +     struct platform_device          *pdev;
>
>Why
>
>        struct device *dev;
>
>is not suffice?
>

I will remove the pdev.

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

I will move the gpio_chip to the first member.

>> +     unsigned int                    irqs[2];
>> +     spinlock_t                      lock;
>> +};
>> +
>> +
>
>One blank line is enough.
>

I will remove it.

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

I will revise it.

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

It is only to check if the offset value is missing in the rtd_gpio_info. I'm uncertain
about the necessity of these checks. If they are not necessary, I will remove
the num_* members in the rtd_gpio_info structure along with these checks. 

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

I will add a callback in the struct rtd_gpio_info to get these values.

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

No, we do not enable CONFIG_PREEMT_RT. However, a custom driver might change
the GPIO status in the ISR. I will utilize raw_spinlock instead and only lock
the write operations.

>> +     val = readl_relaxed(data->base + reg_offset);
>
>> +     val &= BIT(offset % 32);
>
>Why this is is under lock?
>

I'll move it outside of the 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.
>

I will try to use these macros.

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

I will revise it.

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

I will move these codes outside of the lock.

>> +     val = readl_relaxed(data->base + dat_reg_offset);
>
>> +     val >>= offset % 32;
>> +     val &= 0x1;
>
>Why were these operations done under the lock?
>

I will move these codes outside of 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?
>

I will revised it to unsigned int.

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

I will revise it.

>> +     for (i = 0; i < data->info->num_gpios; i = i + 31) {
>
>31 ?!  In any case i += 31 is simply shorter.
>

I will revise it.

>> +             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()
>

I will revise it.

>> +                     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()?
>

I will revise it.

>> +                     }
>> +             }
>> +     }
>> +
>> +     chained_irq_exit(chip, desc);
>> +}
>
>...
>
>> +     u32 mask = BIT(d->hwirq % 32);
>
>Use proper type and getter for hwirq. It's mentioned in the Documentation.
>

I will use irqd_to_hwirq(d) to get hwirq.

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

I think I missed to call the gpiochip_disable_irq/gpiochip_enable_irq in the
.irq_disable/.irq_enable callback function to inform the gpiolib.
I will revise it.

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

I will revise it.

>> +};
>> +MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
>
>Move all these closer to its user (struct platform_device below).
>

I will revise it.

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

I will revise it to data->gpio_chip.parent = dev;.

>...
>
>> +static int rtd_gpio_init(void)
>> +{
>> +     return platform_driver_register(&rtd_gpio_platform_driver);
>> +}
>
>> +
>
>Unneeded blank line.
>

I will remove it.

>> +subsys_initcall(rtd_gpio_init);
>
>Why? Anything that is not on standard initcall must be justified.
>

I will revise it to module_init.

>--
>With Best Regards,
>Andy Shevchenko


Thanks,
Tzuyi Chang