Re: [PATCH RFT net-next 02/10] net: stmmac: Add support for Allwinner A523 GMAC200

[Yanteng Si <si.yanteng@linux.dev>]

在 7/2/25 10:09 AM, Chen-Yu Tsai 写道:
>>> +/* EMAC PHY Interface Type */
>>> +#define SYSCON_EPIT                  BIT(2) /* 1: RGMII, 0: MII */
>>> +#define SYSCON_ETCS_MASK             GENMASK(1, 0)
>>> +#define SYSCON_ETCS_MII              0x0
>>> +#define SYSCON_ETCS_EXT_GMII 0x1
>>> +#define SYSCON_ETCS_INT_GMII 0x2
>>> +
>>> +#define MASK_TO_VAL(mask)   ((mask) >> (__builtin_ffsll(mask) - 1))
>>> +
>>> +static int sun55i_gmac200_set_syscon(struct device *dev,
>>> +                                  struct plat_stmmacenet_data *plat)
>>> +{
>>> +     struct device_node *node = dev->of_node;
>>> +     struct regmap *regmap;
>>> +     u32 val, reg = 0;
>>> +
>>> +     regmap = syscon_regmap_lookup_by_phandle(node, "syscon");
>>> +     if (IS_ERR(regmap))
>>> +             return dev_err_probe(dev, PTR_ERR(regmap), "Unable to map syscon\n");
>>> +
>> -----------
>>> +     if (!of_property_read_u32(node, "allwinner,tx-delay-ps", &val)) {
>>> +             if (val % 100) {
>>> +                     dev_err(dev, "tx-delay must be a multiple of 100\n");
>>> +                     return -EINVAL;
>>> +             }
>>> +             val /= 100;
>>> +             dev_dbg(dev, "set tx-delay to %x\n", val);
>>> +             if (val > MASK_TO_VAL(SYSCON_ETXDC_MASK))
>>> +                     return dev_err_probe(dev, -EINVAL,
>>> +                                          "Invalid TX clock delay: %d\n",
>>> +                                          val);
>>> +
>>> +             reg |= FIELD_PREP(SYSCON_ETXDC_MASK, val);
>>> +     }
>>> +
>>> +     if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
>>> +             if (val % 100) {
>>> +                     dev_err(dev, "rx-delay must be a multiple of 100\n");
>>> +                     return -EINVAL;
>>> +             }
>>> +             val /= 100;
>>> +             dev_dbg(dev, "set rx-delay to %x\n", val);
>>> +             if (val > MASK_TO_VAL(SYSCON_ERXDC_MASK))
>>> +                     return dev_err_probe(dev, -EINVAL,
>>> +                                          "Invalid RX clock delay: %d\n",
>>> +                                          val);
>>> +
>>> +             reg |= FIELD_PREP(SYSCON_ERXDC_MASK, val);
>>> +     }
>> ------------
>> These two parts of the code are highly similar.
>> Can you construct a separate function?
> As in, have a function that sets up either TX or RX delay based on
> a parameter? That also means constructing the property name on the
> fly or using ternary ops. And chopping up the log messages.
> 
> I don't think this makes it easier to read. And chopping up the log
> message makes it harder to grep.
I've looked through other driver codes, and it seems they
are all written this way, so let's keep it as it is for now.

Thanks,
Yanteng