Re: [PATCH 08/12] net: stmmac: Add glue layer for StarFive JH7100 SoC

Andrew Lunn
2023-02-11T16:11:55Z

> +
> +#define JH7100_SYSMAIN_REGISTER28 0x70
> +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */
> +#define JH7100_SYSMAIN_REGISTER49 0xc8

Seems like the comment should be one line earlier?

There is value in basing the names on the datasheet, but you could
append something meaningful on the end:

#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8

???

> + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", &gtxclk_dlychain)) {
> + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain);
> + if (ret)
> + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n");
> + }

You should probably document that if starfive,gtxclk-dlychain is not
found in the DT blob, the value for the delay chain is undefined. It
would actually be better to define it, set it to 0 for example. That
way, you know you don't have any dependency on the bootloader for
example.

Andrew