Re: [PATCH 3/6] drm/sun4i: sun8i-hdmi-phy: Used device-managed clocks/resets

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@sholland.org

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