Re: [PATCH v8 11/11] tests/qtest: Adding PCS Module test to GMAC Qtest

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

On Thu, 14 Dec 2023 at 21:15, Nabih Estefan <nabihestefan@google.com> wrote:
>
> From: Nabih Estefan Diaz <nabihestefan@google.com>
>
>  - Add PCS Register check to npcm_gmac-test
>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> ---



> @@ -129,7 +198,12 @@ static void test_init(gconstpointer test_data)
>  #define CHECK_REG32(regno, value) \
>      do { \
>          g_assert_cmphex(gmac_read(qts, mod, (regno)), ==, (value)); \
> -    } while (0)
> +    } while (0) ;
> +
> +#define CHECK_REG_PCS(regno, value) \
> +    do { \
> +        g_assert_cmphex(pcs_read(qts, mod, (regno)), ==, (value)); \
> +    } while (0) ;

The standard way to write this kind of do {...} while (0)
macro does not have a ';' at the end of the definition.
That's the whole point of the idiom -- it means the macro
expands to a single C statement, so it does the right thing
if you write
  if CHECK_REG_PCS(x,y)
      goto foo;
without braces. You got this right with CHECK_REG32(), but
then this patch (a) incorrectly adds the extra ';' to it
and (b) has a ';' after the new macro.


>      CHECK_REG32(NPCM_DMA_BUS_MODE, 0x00020100);
>      CHECK_REG32(NPCM_DMA_XMT_POLL_DEMAND, 0);
> @@ -180,6 +254,64 @@ static void test_init(gconstpointer test_data)
>      CHECK_REG32(NPCM_GMAC_PTP_TAR, 0);
>      CHECK_REG32(NPCM_GMAC_PTP_TTSR, 0);
>
> +    /* TODO Add registers PCS */
> +    if (mod->base_addr == 0xf0802000) {
> +        CHECK_REG_PCS(NPCM_PCS_SR_CTL_ID1, 0x699e)
> +        CHECK_REG_PCS(NPCM_PCS_SR_CTL_ID2, 0)

These should all have a trailing ';' like the CHECK_REG32()
invocations. This makes them look like function calls
syntax-wise, which is nice for things like editor code
indentation.

thanks
-- PMM