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 <samuel@xxxxxxxxxxxx> >> --- >> >> 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. Regards, Samuel