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

[TY_Chang[張子逸] <tychang@xxxxxxxxxxx>]

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