> > +static void ftgmac100_set_internal_delay(struct platform_device
> > +*pdev) {
> > + struct device_node *np = pdev->dev.of_node;
> > + struct net_device *netdev;
> > + struct ftgmac100 *priv;
> > + struct regmap *scu;
> > + u32 rgmii_tx_delay, rgmii_rx_delay;
> > + u32 dly_reg, tx_dly_mask, rx_dly_mask;
> > + int tx, rx;
>
> Please use the reverse christmas tree notation, sorting declarations by
> descending line length
Got it. I will modify these in next version.
> > + netdev = platform_get_drvdata(pdev);
> > + priv = netdev_priv(netdev);
> > +
> > + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
> > + rx = of_property_read_u32(np, "rx-internal-delay-ps",
> > +&rgmii_rx_delay);
> > +
> > + if (of_device_is_compatible(np, "aspeed,ast2600-mac")) {
> > + /* According to mac base address to get mac index */
> > + switch (priv->res->start) {
> > + case 0x1e660000:
> > + dly_reg = AST2600_MAC12_CLK_DLY;
> > + tx_dly_mask = AST2600_MAC1_TX_DLY;
> > + rx_dly_mask = AST2600_MAC1_RX_DLY;
> > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY,
> rgmii_tx_delay);
> > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY,
> rgmii_rx_delay);
> > + break;
> > + case 0x1e680000:
> > + dly_reg = AST2600_MAC12_CLK_DLY;
> > + tx_dly_mask = AST2600_MAC2_TX_DLY;
> > + rx_dly_mask = AST2600_MAC2_RX_DLY;
> > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY,
> rgmii_tx_delay);
> > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY,
> rgmii_rx_delay);
> > + break;
> > + case 0x1e670000:
> > + dly_reg = AST2600_MAC34_CLK_DLY;
> > + tx_dly_mask = AST2600_MAC3_TX_DLY;
> > + rx_dly_mask = AST2600_MAC3_RX_DLY;
> > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY,
> rgmii_tx_delay);
> > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY,
> rgmii_rx_delay);
> > + break;
> > + case 0x1e690000:
> > + dly_reg = AST2600_MAC34_CLK_DLY;
> > + tx_dly_mask = AST2600_MAC4_TX_DLY;
> > + rx_dly_mask = AST2600_MAC4_RX_DLY;
> > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY,
> rgmii_tx_delay);
> > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY,
> rgmii_rx_delay);
> > + break;
> > + default:
> > + dev_warn(&pdev->dev, "Invalid mac base address");
> > + return;
>
> There has to be a better way that directly looking up the base address.
> Maybe you need an extra DT property ?
I use the base address to identify the MAC index because it is the only fixed value
that is paired with the MAC.
If I create a property to identify the MAC index and someone use the wrong value,
the driver will configure the wrong register field.
Therefore, I use the base address as MAC index, and it is very clear which MAC is
configured.
>
> > + }
> > + } else {
> > + return;
> > + }
> > +
> > + scu = syscon_regmap_lookup_by_phandle(np, "scu");
> > + if (IS_ERR(scu)) {
> > + dev_warn(&pdev->dev, "failed to map scu base");
> > + return;
> > + }
> > +
> > + if (!tx) {
> > + /* Use tx-internal-delay-ps as index to configure tx delay
> > + * into scu register.
> > + */
>
> So this goes completely against the naming of the property. It has the -ps suffix,
> so you would expect to have picoseconds values passed, and not an arbiraty
> index.
>
> Take a look at other drivers, you should accept picseconds values from these
> properties, then compute the relevant index in the driver. That index should be
> something internal to your driver.
>
> An example here :
>
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchi
> p/sparx5/lan969x/lan969x_rgmii.c#L51
>
Agreed. Thank you for your information.
I will adjust this part to use ps unit to translate to the relevant index in next version.
Jacky
