Hi Samuel, Sorry for the (very) late answer
On Tue, Apr 12, 2022 at 06:34:40PM -0500, Samuel Holland wrote: > On 4/12/22 8:23 AM, Maxime Ripard wrote: > > Hi, > > > > On Mon, Apr 11, 2022 at 11:35:08PM -0500, Samuel Holland wrote: > >> Now that the HDMI PHY is using a platform driver, it can use device- > >> managed resources. Use these, as well as the dev_err_probe helper, to > >> simplify the probe function and get rid of the remove function. > >> > >> Signed-off-by: Samuel Holland <[email protected]> > >> --- > >> > >> drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 100 ++++++++----------------- > >> 1 file changed, 30 insertions(+), 70 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > >> b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > >> index 1effa30bfe62..1351e633d485 100644 > >> --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > >> +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > >> @@ -673,10 +673,8 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, > >> struct device_node *node) > >> static int sun8i_hdmi_phy_probe(struct platform_device *pdev) > >> { > >> struct device *dev = &pdev->dev; > >> - struct device_node *node = dev->of_node; > >> struct sun8i_hdmi_phy *phy; > >> void __iomem *regs; > >> - int ret; > >> > >> phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); > >> if (!phy) > >> @@ -686,88 +684,50 @@ static int sun8i_hdmi_phy_probe(struct > >> platform_device *pdev) > >> phy->dev = dev; > >> > >> regs = devm_platform_ioremap_resource(pdev, 0); > >> - if (IS_ERR(regs)) { > >> - dev_err(dev, "Couldn't map the HDMI PHY registers\n"); > >> - return PTR_ERR(regs); > >> - } > >> + if (IS_ERR(regs)) > >> + return dev_err_probe(dev, PTR_ERR(regs), > >> + "Couldn't map the HDMI PHY registers\n"); > >> > >> phy->regs = devm_regmap_init_mmio(dev, regs, > >> &sun8i_hdmi_phy_regmap_config); > >> - if (IS_ERR(phy->regs)) { > >> - dev_err(dev, "Couldn't create the HDMI PHY regmap\n"); > >> - return PTR_ERR(phy->regs); > >> - } > >> + if (IS_ERR(phy->regs)) > >> + return dev_err_probe(dev, PTR_ERR(phy->regs), > >> + "Couldn't create the HDMI PHY regmap\n"); > >> > >> - phy->clk_bus = of_clk_get_by_name(node, "bus"); > >> - if (IS_ERR(phy->clk_bus)) { > >> - dev_err(dev, "Could not get bus clock\n"); > >> - return PTR_ERR(phy->clk_bus); > >> - } > >> - > >> - phy->clk_mod = of_clk_get_by_name(node, "mod"); > >> - if (IS_ERR(phy->clk_mod)) { > >> - dev_err(dev, "Could not get mod clock\n"); > >> - ret = PTR_ERR(phy->clk_mod); > >> - goto err_put_clk_bus; > >> - } > >> + phy->clk_bus = devm_clk_get(dev, "bus"); > >> + if (IS_ERR(phy->clk_bus)) > >> + return dev_err_probe(dev, PTR_ERR(phy->clk_bus), > >> + "Could not get bus clock\n"); > >> > >> - if (phy->variant->has_phy_clk) { > >> - phy->clk_pll0 = of_clk_get_by_name(node, "pll-0"); > >> - if (IS_ERR(phy->clk_pll0)) { > >> - dev_err(dev, "Could not get pll-0 clock\n"); > >> - ret = PTR_ERR(phy->clk_pll0); > >> - goto err_put_clk_mod; > >> - } > >> - > >> - if (phy->variant->has_second_pll) { > >> - phy->clk_pll1 = of_clk_get_by_name(node, "pll-1"); > >> - if (IS_ERR(phy->clk_pll1)) { > >> - dev_err(dev, "Could not get pll-1 clock\n"); > >> - ret = PTR_ERR(phy->clk_pll1); > >> - goto err_put_clk_pll0; > >> - } > >> - } > >> - } > >> + phy->clk_mod = devm_clk_get(dev, "mod"); > >> + if (IS_ERR(phy->clk_mod)) > >> + return dev_err_probe(dev, PTR_ERR(phy->clk_mod), > >> + "Could not get mod clock\n"); > >> > >> - phy->rst_phy = of_reset_control_get_shared(node, "phy"); > >> - if (IS_ERR(phy->rst_phy)) { > >> - dev_err(dev, "Could not get phy reset control\n"); > >> - ret = PTR_ERR(phy->rst_phy); > >> - goto err_put_clk_pll1; > >> - } > >> + if (phy->variant->has_phy_clk) > >> + phy->clk_pll0 = devm_clk_get(dev, "pll-0"); > >> + if (IS_ERR(phy->clk_pll0)) > >> + return dev_err_probe(dev, PTR_ERR(phy->clk_pll0), > >> + "Could not get pll-0 clock\n"); > >> + > >> + if (phy->variant->has_second_pll) > >> + phy->clk_pll1 = devm_clk_get(dev, "pll-1"); > >> + if (IS_ERR(phy->clk_pll1)) > >> + return dev_err_probe(dev, PTR_ERR(phy->clk_pll1), > >> + "Could not get pll-1 clock\n"); > >> + > >> + phy->rst_phy = devm_reset_control_get_shared(dev, "phy"); > >> + if (IS_ERR(phy->rst_phy)) > >> + return dev_err_probe(dev, PTR_ERR(phy->rst_phy), > >> + "Could not get phy reset control\n"); > > > > I find the old construct clearer with the imbricated blocks. > > I'm not sure what you mean here. Are you suggesting braces around the > dev_err_probe statements? Or do you want me to put the error handling for > variant-specific resources inside the variant checks? Please clarify. I meant that we went, for example, from: if (phy->variant->has_phy_clk) { phy->clk_pll0 = devm_clk_get(dev, "pll-0"); if (IS_ERR(phy->clk_pll0)) { ... } } to if (phy->variant->has_phy_clk) phy->clk_pll0 = devm_clk_get(dev, "pll-0"); if (IS_ERR(phy->clk_pll0)) { ... } Which relies on the fact that phy->clk_pll0 is initialized to !IS_ERR(...), which we never explicitly enforced. I find the first and original code clearer for that aspect (since we only use that value if it was set), and less fragile. Maxime
