Message-ID: <63983de33ce2415abb8b5b745db58911@realtek.com>
Date: Tue, 26 Dec 2023 07:34:37 +0000
From: TY_Chang[張子逸] <tychang@...ltek.com>
To: Andy Shevchenko <andy@...nel.org>
CC: Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski
	<brgl@...ev.pl>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

Hi Andy,

Thank you for the review.

>On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:
>> This driver enables configuration of GPIO direction, GPIO values, GPIO
>> debounce settings and handles GPIO interrupts.
>
>...
>
>> +     help
>> +       Say yes here to support GPIO on Realtek DHC(Digital Home Center)
>> +       SoCs.
>
>checkpatch.pl complains if it's less than 3 lines.
>

I will add more description.

>...
>
>Please, follow IWYU principle.
>
>> +#include <linux/bitops.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
>> +#include <linux/module.h> #include <linux/platform_device.h> #include
>> +<linux/property.h>
>
>> +#include <linux/spinlock.h>
>
>+ types.h
>

I will revise it.

>...
>
>> +struct rtd_gpio_info {
>> +     const char              *name;
>> +     unsigned int            gpio_base;
>> +     unsigned int            num_gpios;
>> +     u8                      *dir_offset;
>> +     u8                      *dato_offset;
>> +     u8                      *dati_offset;
>> +     u8                      *ie_offset;
>> +     u8                      *dp_offset;
>> +     u8                      *gpa_offset;
>> +     u8                      *gpda_offset;
>> +     u8                      *deb_offset;
>> +     u8                      *deb_val;
>> +     u8                      (*get_deb_setval)(const struct
>rtd_gpio_info *info,
>> +                                                unsigned int
>offset, u8 *reg_offset,
>> +                                                u8 *shift, u8
>> +deb_index);
>
>Basically you should group input parameters and output for better
>understanding.
>

I will define a structure to hold the necessary information for the output.

>        u8              (*get_deb_setval)(const struct rtd_gpio_info
>*info,
>                                          unsigned int offset, u8
>deb_index,
>                                          u8 *reg_offset, u8 *shift);
>
>Also indent the lines properly (besides the TABs).
>
>> +};
>
>Make it one TAB less in the middle.
>

I will revise it.

>...
>
>> +static u8 rtd_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned
>int offset,
>> +                             u8 *reg_offset, u8 *shift, u8 deb_val)
>
>Why is it called val here and index in the other cases?
>Can you come up with better naming that it can be consistent in all four places?
>

I missed to rename it to 'deb_index'. Sorry about that.

>> +{
>> +     *reg_offset = info->deb_offset[offset / 8];
>> +     *shift = (offset % 8) * 4;
>> +     return info->deb_val[deb_val];
>> +}
>> +
>> +static u8 rtd1295_misc_gpio_get_deb_setval(const struct rtd_gpio_info *info,
>unsigned int offset,
>> +                                      u8 *reg_offset, u8 *shift, u8
>> +deb_index) {
>> +     *reg_offset = info->deb_offset[0];
>> +     *shift = (offset % 8) * 4;
>> +     return info->deb_val[deb_index]; }
>
>> +static u8 rtd1295_iso_gpio_get_deb_setval(const struct rtd_gpio_info *info,
>unsigned int offset,
>> +                                     u8 *reg_offset, u8 *shift, u8
>> +deb_index) {
>> +     *reg_offset = info->deb_offset[0];
>> +     *shift = 0;
>> +     return info->deb_val[deb_index]; }
>
>...
>
>> +static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int
>> +offset) {
>> +     return data->info->gpa_offset[offset / 31]; }
>> +
>> +static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int
>> +offset) {
>> +     return data->info->gpda_offset[offset / 31]; }
>
>The / 31 so-o-o counter intuitive, please add a comment in each case to explain
>why [it's not 32 or other power-of-2].
>

In our hardware design, the bit 0 of the gpda and gpa status registers does not correspond to a GPIO.
If bit 0 is set to 1, the other bit can be set to 1 by writing 1.
If bit 0 is set to 0, the other bit can be clear to 0 by writing 1.

Therefore, each status register only contains the status of 31 GPIOs. I will add the comment for this.

>...
>
>> +     raw_spin_lock_irqsave(&data->lock, flags);
>> +     writel_relaxed(val, data->base + reg_offset);
>> +     raw_spin_unlock_irqrestore(&data->lock, flags);
>
>Convert to use cleanup.c, in particular here it becomes
>
>        guard(raw_spinlock_irqsave)(&data->lock);
>
>        writel_relaxed(val, data->base + reg_offset);
>

I will revise it.

>...
>
>> +     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);
>> +
>> +     val = readl_relaxed(data->base + dat_reg_offset);
>
>> +     val >>= offset % 32;
>> +     val &= 0x1;
>
>Replace 3 LoCs by 1:
>
>        return !!(val & BIT(ofsset % 32));
>
>
>Missed locking. How do you guarantee that you will get consistent results
>between the reads?
>

I will revise it.

>...
>
>> +     val &= BIT(offset % 32);
>> +
>> +     return val ? GPIO_LINE_DIRECTION_OUT :
>GPIO_LINE_DIRECTION_IN;
>
>        if (val & BIT(...))
>                return _OUT;
>        return _IN;
>
>...
>
>> +     for (i = 0; i < data->info->num_gpios; i += 31) {
>
>Same, add explanation why 31.
>
>Note, I actually prefer to see use of valid_mask instead of this weirdness.
>Then you will need to comment only once and use 32 (almost?) everywhere.
>

The reason remains consistent with the previous explanation. Each status register
exclusively holds the status of 31 GPIOs.

>> +             reg_offset = get_reg_offset(data, i);
>> +
>> +             status = readl_relaxed(data->irq_base + reg_offset) >> 1;
>> +             writel_relaxed(status << 1, data->irq_base +
>> + reg_offset);
>> +
>> +             for_each_set_bit(j, &status, 31) {
>> +                     hwirq = i + j;
>
>Nice, but you can do better
>
>                /* Bit 0 is special... bla-bla-bla... */
>                status = readl_relaxed(data->irq_base + reg_offset);
>                status &= ~BIT(0);
>                writel_relaxed(status, data->irq_base + reg_offset);
>
>                for_each_set_bit(j, &status, 32) {
>                        hwirq = i + j - 1;
>

Given that each status register accommodates the status of only 31 GPIOs, I think utilizing 
the upper format and including explanatory comments would be appropriate. It can indicate the
status registers only contains 31 GPIOs. Please correct me if my understanding is incorrect.

>> +                     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))
>
>Do you need mask? Isn't irq_type already properly masked here?
>
>> +                                     break;
>> +                             generic_handle_domain_irq(domain,
>hwirq);
>> +                     }
>> +             }
>> +     }
>

It has already been masked in the irq_get_trigger_type. I will revise it.

>...
>
>> +     u32 clr_mask = BIT(hwirq % 31) << 1;
>> +     u32 ie_mask = BIT(hwirq % 32);
>
>This blows the mind. Needs a comment.
>

The clr_mask is used to clear the gpa/gpda registers, each of which accommodates only 31 GPIOs.

>...
>
>> +static int rtd_gpio_irq_set_type(struct irq_data *d, unsigned int
>> +type) {
>> +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +     struct rtd_gpio *data = gpiochip_get_data(gc);
>> +     irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> +     u32 mask = BIT(hwirq % 32);
>> +     unsigned long flags;
>> +     int dp_reg_offset;
>> +     bool polarity;
>> +     u32 val;
>> +
>> +     dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
>> +
>> +     switch (type & IRQ_TYPE_SENSE_MASK) {
>> +     case IRQ_TYPE_EDGE_RISING:
>> +             polarity = 1;
>> +             break;
>> +
>> +     case IRQ_TYPE_EDGE_FALLING:
>> +             polarity = 0;
>> +             break;
>> +
>> +     case IRQ_TYPE_EDGE_BOTH:
>> +             polarity = 1;
>> +             break;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     raw_spin_lock_irqsave(&data->lock, flags);
>> +
>> +     val = readl_relaxed(data->base + dp_reg_offset);
>> +     if (polarity)
>> +             val |= mask;
>> +     else
>> +             val &= ~mask;
>> +     writel_relaxed(val, data->base + dp_reg_offset);
>> +
>> +     raw_spin_unlock_irqrestore(&data->lock, flags);
>> +
>> +     return 0;
>> +}
>
>...
>
>> +     irq_chip->handler = handle_simple_irq;
>
>Please, apply bad handler here and lock it in the set_type callback above.
>
>You may read eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel
>Galileo Gen 2") to understand the difference.
>

I will revise it.

>...
>
>> +static int rtd_gpio_init(void)
>> +{
>> +     return platform_driver_register(&rtd_gpio_platform_driver);
>> +}
>
>> +
>
>Redundant blank line, but see below.
>
>> +module_init(rtd_gpio_init);
>> +
>> +static void __exit rtd_gpio_exit(void) {
>> +     platform_driver_unregister(&rtd_gpio_platform_driver);
>> +}
>> +module_exit(rtd_gpio_exit);
>
>There is no special initcall, you may use module_platform_driver() macro
>instead.
>

I will revise it.

Thanks,
Tzuyi Chang

Powered by blists - more mailing lists