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
