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

[TY_Chang[張子逸]]

Hi Andy,

>On Tue, Dec 26, 2023 at 07:34:37AM +0000, TY_Chang[張子逸] wrote:
>> >On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:
>
>...
>
>> >> +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.
>
>Yes, please add in all places, while it's a dup, it helps understanding the point
>without looking around for a while.
>
>...
>
>> >> +     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.
>
>As per above, add a comment.
>
>> >> +             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.
>
>The above is just a code hack to help bitops to optimise. 32 is power-of-2 which
>might be treated better by the compiler and hence produce better code.
>
>Yet, it's an interrupt handler where we want to have the ops as shorter as
>possible, so even micro-optimizations are good to have here (I don't insist to
>follow the same idea elsewhere).
>

I understand. Thank you for explaining. I will revise it.

>> >> +                     }
>> >> +             }
>> >> +     }
>

Thanks,
Tzuyi Chang