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

[Andy Shevchenko <andy@xxxxxxxxxx>]

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

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

-- 
With Best Regards,
Andy Shevchenko