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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 26 Dec 2023 07:34:57 +0000
From: TY_Chang[張子逸] <tychang@...ltek.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Linus Walleij
	<linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        "Andy
 Shevchenko" <andy@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley
	<conor+dt@...nel.org>
CC: "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 Krzysztof,

Thank you for the review.

>...
>
>> +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);
>
>Why are you using raw spinlock? This question applies to entire driver.
>

I think consumer driver may operate GPIOs within the ISR, so it needs a lock.

>> +
>> +     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;
>> +}
>
>...
>
>}
>> +
>> +module_init(rtd_gpio_init);
>> +
>> +static void __exit rtd_gpio_exit(void) {
>> +     platform_driver_unregister(&rtd_gpio_platform_driver);
>> +}
>> +module_exit(rtd_gpio_exit);
>
>Why not using module_platform_driver?
>

I will revise it.

Thanks,
Tzuyi Chang