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] [day] [month] [year] [list]

Message-ID: <532f905a4df6444c85c7932bca60aeca@realtek.com>
Date: Thu, 28 Dec 2023 02:27:06 +0000
From: TY_Chang[張子逸] <tychang@...ltek.com>
To: Andy Shevchenko <andy@...nel.org>
CC: Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski
	<brgl@...ev.pl>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "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 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