Re: [PATCH 1/8] hw/arm/exynos4210: Replace magic 32 by proper 'GIC_INTERNAL' definition

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The 32 IRQ lines skipped are the GIC internal ones.
> Use the GIC_INTERNAL definition for clarity.
> No logical change.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/exynos4210.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index dd0edc81d5c..99b05a175d6 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -393,8 +393,9 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>              }
>          }
>          if (irq_id) {
> +            irq_id -= GIC_INTERNAL;
>              qdev_connect_gpio_out(splitter, splitin,
> -                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
> +                                  qdev_get_gpio_in(extgicdev, irq_id));
>          }
>      }
>      for (; n < EXYNOS4210_MAX_INT_COMBINER_IN_IRQ; n++) {
> @@ -413,6 +414,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>          }
>
>          if (irq_id) {
> +            irq_id -= GIC_INTERNAL;
>              assert(splitcount < EXYNOS4210_NUM_SPLITTERS);
>              splitter = DEVICE(&s->splitter[splitcount]);
>              qdev_prop_set_uint16(splitter, "num-lines", 2);
> @@ -421,7 +423,7 @@ static void exynos4210_init_board_irqs(Exynos4210State *s)
>              s->irq_table[n] = qdev_get_gpio_in(splitter, 0);
>              qdev_connect_gpio_out(splitter, 0, qdev_get_gpio_in(intcdev, n));
>              qdev_connect_gpio_out(splitter, 1,
> -                                  qdev_get_gpio_in(extgicdev, irq_id - 32));
> +                                  qdev_get_gpio_in(extgicdev, irq_id));
>          } else {
>              s->irq_table[n] = qdev_get_gpio_in(intcdev, n);

A small nit, but I think I would prefer these as
irq_id - GIC_INTERNAL, rather than changing the value of the variable.
Otherwise the semantics of the value in the variable change
mid-way. That doesn't have a practical effect since the call
to qdev_get_gpio_in() is in both places that last use of the
variable, but I think it's a bit confusing.

thanks
-- PMM