Messages in this thread |  | | Date | Sun, 21 Jan 2024 17:19:14 +0100 | | From | Andrew Lunn <> | | Subject | Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver |
| |
On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: > Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
You need to say something in the commit message. One obvious thing is to justify not using the at803x driver, since
> +#define IPQ5018_PHY_ID 0x004dd0c0
is in the Atheros OUI range.
> +static int ipq5018_probe(struct phy_device *phydev) > +{ > + struct ipq5018_phy *priv; > + struct device *dev = &phydev->mdio.dev; > + char name[64]; > + int ret;
I guess you are new to mainline network. Please read:
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
Section 1.6.4.
> + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return dev_err_probe(dev, -ENOMEM, > + "failed to allocate priv\n");
Please read the documentation of dev_err_probe() and this fix the obvious problem with this.
> + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); > + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > + TX_RX_CLK_RATE); > + if (IS_ERR_OR_NULL(priv->clk_rx)) > + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), > + "failed to register rx clock\n"); > + > + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); > + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > + TX_RX_CLK_RATE); > + if (IS_ERR_OR_NULL(priv->clk_tx)) > + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), > + "failed to register tx clock\n"); > + > + priv->clk_data = devm_kzalloc(dev, > + struct_size(priv->clk_data, hws, 2), > + GFP_KERNEL); > + if (!priv->clk_data) > + return dev_err_probe(dev, -ENOMEM, > + "failed to allocate clk_data\n"); > + > + priv->clk_data->num = 2; > + priv->clk_data->hws[0] = priv->clk_rx; > + priv->clk_data->hws[1] = priv->clk_tx; > + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, > + priv->clk_data); > + if (ret) > + return dev_err_probe(dev, ret, > + "fail to register clock provider\n");
This needs an explanation. Why register two fixed clocks, which you never use? Why not put these two clocks in DT?
> + > + return 0; > +} > + > +static int ipq5018_soft_reset(struct phy_device *phydev) > +{ > + int ret; > + > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > + IPQ5018_PHY_FIFO_RESET, 0); > + if (ret < 0) > + return ret; > + > + msleep(50); > + > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); > + if (ret < 0) > + return ret; > + > + return 0; > +}
This needs an explanation. It is also somewhat like qca808x_link_change_notify(). Is it really sufficient to only do this reset FIFO during a soft reset, or is it needed when ever the link changes?
You also appear to be missing device tree bindings.
Andrew
--- pw-bot: cr
|  |