lkml.org 
[lkml]   [2022]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v13, 2/2] net: Add dm9051 driver
> +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

\
 
 \ /
  Last update: 2022-01-25 15:18    [W:0.059 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site