> +static int thead_dwmac_set_txclk_dir(struct plat_stmmacenet_data *plat) > +{ > + struct thead_dwmac *dwmac = plat->bsp_priv; > + u32 txclk_dir; > + > + switch (plat->mac_interface) { > + case PHY_INTERFACE_MODE_MII: > + txclk_dir = TXCLK_DIR_INPUT; > + break; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_ID: > + case PHY_INTERFACE_MODE_RGMII_TXID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + txclk_dir = TXCLK_DIR_OUTPUT; > + break; > + default: > + dev_err(dwmac->dev, "unsupported phy interface %d\n", > + plat->mac_interface); > + return -EINVAL; > + }; > + > + regmap_write(dwmac->apb_regmap, GMAC_TXCLK_OEN, txclk_dir); > + > + return 0; regmap_write() can fail. So it would be better to have return regmap_write(dwmac->apb_regmap, GMAC_TXCLK_OEN, txclk_dir); Please consider this for all functions which a similar. > +static void thead_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode) > +{ > + struct thead_dwmac *dwmac = priv; > + struct plat_stmmacenet_data *plat = dwmac->plat; > + unsigned long rate; > + u32 div; Reverse christmas tree please. You will need to move the plat assignment into the body of the code. > +static int thead_dwmac_probe(struct platform_device *pdev) > +{ > + struct plat_stmmacenet_data *plat; > + struct stmmac_resources stmmac_res; > + struct thead_dwmac *dwmac; > + struct device_node *np = pdev->dev.of_node; > + u32 delay_ps; > + int ret; > + void __iomem *apb; np and apb should be earlier to keep with reverse christmas tree. Please check and fix all functions. > + > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "failed to get resources\n"); > + > + plat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac); > + if (IS_ERR(plat)) > + return dev_err_probe(&pdev->dev, PTR_ERR(plat), > + "dt configuration failed\n"); > + > + dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL); > + if (!dwmac) > + return -ENOMEM; > + > + if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay_ps)) > + dwmac->rx_delay = delay_ps; > +#define GMAC_RXCLK_DELAY_MASK GENMASK(4, 0) So this is a 5 bit value. You should perform a range check. So, do you need to do some sort of conversion from picoseconds to register value? > +MODULE_AUTHOR("T-HEAD"); Authors are people. > +MODULE_AUTHOR("Jisheng Zhang <jszhang@xxxxxxxxxx>"); > +MODULE_DESCRIPTION("T-HEAD dwmac platform driver"); > +MODULE_LICENSE("GPL"); Andrew --- pw-bot: cr