Re: [PATCH v7 10/15] drm/rockchip: analogix_dp: Add support to get panel from the DP AUX bus

Hi Doug,

On 2025/2/25 9:42, Doug Anderson wrote:

Hi,

On Mon, Feb 24, 2025 at 12:14 AM Damon Ding [off-list ref] wrote:

quoted

@@ -392,11 +393,27 @@ static const struct component_ops rockchip_dp_component_ops = {
         .unbind = rockchip_dp_unbind,
  };

+static int rockchip_dp_link_panel(struct drm_dp_aux *aux)
+{
+       struct analogix_dp_plat_data *plat_data = analogix_dp_aux_to_plat_data(aux);
+       struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data);
+       int ret;
+
+       ret = drm_of_find_panel_or_bridge(dp->dev->of_node, 1, 0, &plat_data->panel, NULL);
+       if (ret && ret != -ENODEV)
+               return ret;

Can you explain why you treat -ENODEV as a non-error case here? Maybe
this is for the non-eDP case (AKA the DP case) where there's no
further panels or bridges? Maybe a comment would be helpful to remind
us?

I think the commit 86caee745e45 ("drm/rockchip: analogix_dp: allow to 
work without panel") can help the Analogix DP driver work when the 
bridge is driver-free or when the user uses the eDP IP as a DP.

And I will add some comments in the next version.

quoted

+       ret = component_add(dp->dev, &rockchip_dp_component_ops);
+       if (ret)
+               return ret;
+
+       return ret;

nit: the above could just be:

return component_add(dp->dev, &rockchip_dp_component_ops);

Yeah, it is a good idea.

quoted

@@ -448,9 +460,16 @@ static int rockchip_dp_probe(struct platform_device *pdev)
         if (IS_ERR(dp->adp))
                 return PTR_ERR(dp->adp);

-       ret = component_add(dev, &rockchip_dp_component_ops);
-       if (ret)
-               return ret;
+       ret = devm_of_dp_aux_populate_bus(analogix_dp_get_aux(dp->adp), rockchip_dp_link_panel);
+       if (ret) {
+               if (ret != -ENODEV)
+                       return dev_err_probe(dp->dev, ret,
+                                            "failed to populate aux bus : %d\n", ret);

IIRC this -ENODEV case is for old legacy panels that aren't listed
under the aux bus in the device tree. Maybe a comment would be helpful
to remind us?

I will add a comment here if devm_of_dp_aux_populate_bus() returns -ENODEV.

nit: don't need the %d in your error message. dev_err_probe() already
prints the error code.

I will remove it in the next version.

quoted

+               ret = rockchip_dp_link_panel(analogix_dp_get_aux(dp->adp));
+               if (ret)
+                       return ret;
+       }

         return 0;

You can get rid of a few of your return cases by just returning "ret" here.

Yeah, it is better.

-Doug

Best regards
Damon