Re: [PATCH v2 5/8] phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY

On Mon, 26 Feb 2024 at 14:24, Marek Vasut marex@denx.de wrote:

On 2/26/24 9:04 AM, Sumit Garg wrote:

...

Add initial support for i.MX8M{M/P} PCIe PHY. On i.MX8M{M/P} SoCs PCIe PHY initialization moved to this standalone PHY driver.

Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

Commit ID , see previous comments.

[...]

...

+static int imx8_pcie_phy_probe(struct udevice *dev) +{

• struct imx8_pcie_phy *imx8_phy = dev_get_priv(dev);
  

• ofnode gpr;
  

• int ret = 0;
  

• imx8_phy->drvdata = (void *)dev_get_driver_data(dev);
  

• imx8_phy->base = dev_read_addr(dev);
  

• if (!imx8_phy->base)
  

•         return -EINVAL;
  

• /* get PHY refclk pad mode */
  

• dev_read_u32(dev, "fsl,refclk-pad-mode", &imx8_phy->refclk_pad_mode);
  

• if (dev_read_u32(dev, "fsl,tx-deemph-gen1", &imx8_phy->tx_deemph_gen1))
  

•         imx8_phy->tx_deemph_gen1 = 0;
  

Use dev_read_u32_default() and you won't need the if (...)

Ack.

...

• if (dev_read_u32(dev, "fsl,tx-deemph-gen2", &imx8_phy->tx_deemph_gen2))
  

•         imx8_phy->tx_deemph_gen2 = 0;
  

• if (dev_read_bool(dev, "fsl,clkreq-unsupported"))
  

•         imx8_phy->clkreq_unused = true;
  

• else
  

•         imx8_phy->clkreq_unused = false;
  

• /* Grab GPR config register range */
  

• gpr = ofnode_by_compatible(ofnode_null(), imx8_phy->drvdata->gpr);
  

• if (ofnode_equal(gpr, ofnode_null())) {
  

•         dev_err(dev, "unable to find GPR node\n");
  

•         return -ENODEV;
  

• }
  

• imx8_phy->iomuxc_gpr = syscon_node_to_regmap(gpr);
  

• if (IS_ERR(imx8_phy->iomuxc_gpr)) {
  

•         dev_err(dev, "unable to find iomuxc registers\n");
  

•         return PTR_ERR(imx8_phy->iomuxc_gpr);
  

• }
  

• ret = clk_get_by_name(dev, "ref", &imx8_phy->hsio_clk);
  

• if (ret) {
  

•         dev_err(dev, "Failed to get PCIEPHY ref clock\n");
  

•         return ret;
  

• }
  

• ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
  

• if (ret) {
  

•         dev_err(dev, "Failed to get PCIEPHY reset control\n");
  

•         return ret;
  

• }
  

Some clk_put() and co. fail path is missing here.

Ditto for clk_put() not being available. However, I can add the fail path for the other reset.

Also .remove is missing .

Sure, I will add that to release resets.

-Sumit