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

>On Tue, Dec 12, 2023 at 09:55:59AM +0000, TY_Chang[張子逸] wrote:
>> >On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
>
>...
>
>> >> 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.
>
>If it appears that it makes code uglier / complicated, please add the note
>somewhere to answer the above question.
>

I've traced the gpio-regmap.c file. It appears that for the driver to register
gpio_irq_chip, it must create the irq_domain and add it into gpio_regmap_config.
Additionally, the driver needs to register the irq handler by itself.
However, this process can be managed by the gpiolib if the driver fills in the struct
gpio_irq_chip inside struct gpio_chip before invoking gpiochip_add_data.

Moreover, apart from managing the registers for gpio direction and value, there
are several other registers that require access(interrupt enable, debounce...).
The GPIO IRQ status registers are located at different base addresses and are
not contiguous. It may need to create an additional regmap and assign the access
table to this regmap.

With the above consideration, I tend to keep using the existing method.

>...
>
>> >> +     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.
>
>My understanding that these checks are equivalent to the
>
>        if (offset >= ngpio)
>
>one, which is performed by GPIO library, i.o.w. you will never get an offset
>outside the range of supported GPIO lines.
>
>If my understanding is wrong, these checks need a comment why.
>

I agree with you. I will remove these checks.

>...
>
>> >> +     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.
>
>Thinking about this more, perhaps you can register two IRQ chips with different
>functions, so this won't be part of the very critical interrupt handler (as we all
>want to reduce overhead in it as much as possible).
>Anyway, think about this and try different options, choose the one you think the
>best.
>

In the previous patch (v1), I had registered two IRQ chips with different handlers. 
However, these two handlers appeared quite similar and the gpio_irq_chip only allows
the registration of a single handler. Therefore, I ended up registering one handler
for both IRQs and included conditional checks within the handler to differentiate
between the two.

>--
>With Best Regards,
>Andy Shevchenko

Thanks,
Tzuyi Chang