| Message ID | 20210826123844.8464-1-yifeng.zhao@rock-chips.com |
|---|---|
| Headers | show |
| Series | [v1,1/3] dt-bindings: phy: rockchip: Add Naneng combo PHY bindings | expand |
On Thu, Aug 26, 2021 at 8:42 AM Yifeng Zhao <yifeng.zhao@rock-chips.com> wrote: > > Add Naneng combo PHY support for RK3568 > > This phy can be used as pcie-phy, usb3-phy, sata-phy or sgmii-phy. Good Afternoon, Some feedback on this driver, for when you resend it to the mailing list. I'm sending this against the cover letter since the mailing list doesn't have the actual driver. The driver doesn't work out of the box, you renamed the clocks but missed one use point. There's a lot of "magic numbers" that need to be defined. Configuration could use some cleanup. A parallel problem, if the PCIe phy fails to probe (because of the aforementioned clock issue) the PCIe controller hard locks the board during probe. I look forward to version two. Very Respectfully, Peter Geis > > > > Yifeng Zhao (3): > dt-bindings: phy: rockchip: Add Naneng combo PHY bindings > phy/rockchip: add naneng combo phy for RK3568 > arm64: dts: rockchip: add naneng combo phy nodes for rk3568 > > .../phy/phy-rockchip-naneng-combphy.yaml | 100 +++ > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 68 ++ > drivers/phy/rockchip/Kconfig | 8 + > drivers/phy/rockchip/Makefile | 1 + > .../rockchip/phy-rockchip-naneng-combphy.c | 646 ++++++++++++++++++ > 5 files changed, 823 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml > create mode 100644 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c > > -- > 2.17.1 > > > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Donnerstag, 26. August 2021 14:38:43 CEST Yifeng Zhao wrote: > This patch implements a combo phy driver for Rockchip SoCs > with NaNeng IP block. This phy can be used as pcie-phy, usb3-phy, > sata-phy or sgmii-phy. > > Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com> > --- > [...] > +static int rockchip_combphy_pcie_init(struct rockchip_combphy_priv *priv) > +{ > + int ret = 0; > + > + if (priv->cfg->combphy_cfg) { > + ret = priv->cfg->combphy_cfg(priv); > + if (ret) { > + dev_err(priv->dev, "failed to init phy for pcie\n"); > + return ret; > + } > + } > + > + return ret; > +} > + > +static int rockchip_combphy_usb3_init(struct rockchip_combphy_priv *priv) > +{ > + int ret = 0; > + > + if (priv->cfg->combphy_cfg) { > + ret = priv->cfg->combphy_cfg(priv); > + if (ret) { > + dev_err(priv->dev, "failed to init phy for usb3\n"); > + return ret; > + } > + } > + > + return ret; > +} > + > +static int rockchip_combphy_sata_init(struct rockchip_combphy_priv *priv) > +{ > + int ret = 0; > + > + if (priv->cfg->combphy_cfg) { > + ret = priv->cfg->combphy_cfg(priv); > + if (ret) { > + dev_err(priv->dev, "failed to init phy for sata\n"); > + return ret; > + } > + } > + > + return ret; > +} > + > +static int rockchip_combphy_sgmii_init(struct rockchip_combphy_priv *priv) > +{ > + int ret = 0; > + > + if (priv->cfg->combphy_cfg) { > + ret = priv->cfg->combphy_cfg(priv); > + if (ret) { > + dev_err(priv->dev, "failed to init phy for sgmii\n"); > + return ret; > + } > + } > + > + return ret; > +} > + > +static int rockchip_combphy_set_mode(struct rockchip_combphy_priv *priv) > +{ > + switch (priv->mode) { > + case PHY_TYPE_PCIE: > + rockchip_combphy_pcie_init(priv); > + break; > + case PHY_TYPE_USB3: > + rockchip_combphy_usb3_init(priv); > + break; > + case PHY_TYPE_SATA: > + rockchip_combphy_sata_init(priv); > + break; > + case PHY_TYPE_SGMII: > + case PHY_TYPE_QSGMII: > + return rockchip_combphy_sgmii_init(priv); > + default: > + dev_err(priv->dev, "incompatible PHY type\n"); > + return -EINVAL; > + } > + > + return 0; > +} All of the _init functions appear to be the same except for the error string. I think it would be better to just have the init done in _set_mode, and then use the switch case statement to show the right error message on if (ret). > [...] > + > +static int rockchip_combphy_probe(struct platform_device *pdev) > +{ > + struct phy_provider *phy_provider; > + struct device *dev = &pdev->dev; > + struct rockchip_combphy_priv *priv; > + const struct rockchip_combphy_cfg *phy_cfg; > + struct resource *res; > + int ret; > + > + phy_cfg = of_device_get_match_data(dev); > + if (!phy_cfg) { > + dev_err(dev, "No OF match data provided\n"); > + return -EINVAL; > + } > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->mmio = devm_ioremap_resource(dev, res); I think devm_platform_get_and_ioremap_resource is preferred here, using it also means you can get rid of res. > + if (IS_ERR(priv->mmio)) { > + ret = PTR_ERR(priv->mmio); > + return ret; > + } > + > [...] Regards, Nicolas Frattaroli