Re: [PATCH v13, 2/2] net: Add dm9051 driver

[Andrew Lunn <andrew@lunn.ch>]

> +static int dm9051_mdiobus_read(struct mii_bus *mdiobus, int phy_id, int reg)
> +{
> +	struct board_info *db = mdiobus->priv;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	if (phy_id == DM9051_PHY_ID) {

phy_id is a poor choice of name. It normally means the value you find
in register 2 and 3 of the PHY which identifies the manufacture, make
and possibly revision.

If you look at the read function prototype in struct mii_bus:

https://elixir.bootlin.com/linux/v5.17-rc1/source/include/linux/phy.h#L357

the normal name is addr.

Ideally your driver needs to look similar to other drivers. Ideally
you use the same variable names for the same things. That makes it
easier for somebody else to read your driver and debug it. It makes it
easier to review, etc. It is worth spending time reading a few other
drivers and looking for common patterns, and making use of those
patterns in your driver.

> +static int dm9051_map_phyup(struct board_info *db)
> +{
> +	int ret;
> +
> +	/* ~BMCR_PDOWN to power-up the internal phy
> +	 */
> +	ret = mdiobus_modify(db->mdiobus, DM9051_PHY_ID, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret < 0)
> +		return ret;

You are still touching PHY registers from the MAC driver. Why is your
PHY driver not going this as part of the _config() function?

    Andrew