Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP

On Tue, Jun 9, 2020 at 3:20 PM Xin Ji xji@analogixsemi.com wrote:

The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.

Signed-off-by: Xin Ji xji@analogixsemi.com

drivers/gpu/drm/bridge/analogix/Kconfig | 9 + drivers/gpu/drm/bridge/analogix/Makefile | 1 + drivers/gpu/drm/bridge/analogix/anx7625.c | 1999 +++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++++++ 4 files changed, 2406 insertions(+) create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h

[snip] +static int anx7625_parse_dt(struct device *dev,

•                       struct anx7625_platform_data *pdata)
  

+{

•   struct device_node *np = dev->of_node;
  

•   struct device_node *panel_node, *out_ep;
  

•   pdata->node.mipi_dsi_host_node = of_graph_get_remote_node(np, 0, 0);
  

•   if (!pdata->node.mipi_dsi_host_node) {
  

•           DRM_DEV_ERROR(dev, "fail to get internal panel.\n");
  

•           return -EPROBE_DEFER;
  

This does not look correct. I don't think of_graph_get_remote_node will ever return NULL if the device tree is configured properly, and it's useless to retry later (EPROBE_DEFER). You should just fail (e.g. return EINVAL).

•   }
  

•   of_node_put(pdata->node.mipi_dsi_host_node);
  

You are using pdata->node.mipi_dsi_host_node in other places in the code, so I don't think it's ok to call of_node_put?

•   DRM_DEV_DEBUG_DRIVER(dev, "found dsi host node.\n");
  

•   pdata->node.panel_node = of_graph_get_port_by_id(np, 1);
  

•   if (!pdata->node.panel_node) {
  

•           DRM_DEV_ERROR(dev, "fail to get panel node.\n");
  

•           return -EPROBE_DEFER;
  

-EINVAL.

•   }
  

•   of_node_put(pdata->node.panel_node);
  

•   out_ep = of_get_child_by_name(pdata->node.panel_node,
  

•                                 "endpoint");
  

•   if (!out_ep) {
  

•           DRM_DEV_DEBUG_DRIVER(dev, "cannot get endpoint.\n");
  

DRM_DEV_ERROR seems more appropriate

•           return -EPROBE_DEFER;
  

-EINVAL

•   }
  

•   panel_node = of_graph_get_remote_port_parent(out_ep);
  

•   of_node_put(out_ep);
  

•   pdata->panel = of_drm_find_panel(panel_node);
  

•   DRM_DEV_DEBUG_DRIVER(dev, "get panel node.\n");
  

•   of_node_put(panel_node);
  

•   if (IS_ERR_OR_NULL(pdata->panel))
  

•           return -EPROBE_DEFER;
  

of_drm_find_panel cannot return NULL, so, do this instead:

if (IS_ERR(pdata->panel)) return PTR_ERR(pdata->panel);

(which actually _may_ return EPROBE_DEFER)

•   return 0;
  

+} [snip] +static int anx7625_i2c_probe(struct i2c_client *client,

•                        const struct i2c_device_id *id)
  

+{

•   struct anx7625_data *platform;
  

•   struct anx7625_platform_data *pdata;
  

•   int ret = 0;
  

•   struct device *dev = &client->dev;
  

•   if (!i2c_check_functionality(client->adapter,
  

•                                I2C_FUNC_SMBUS_I2C_BLOCK)) {
  

•           DRM_DEV_ERROR(dev, "anx7625's i2c bus doesn't support\n");
  

•           return -ENODEV;
  

•   }
  

•   platform = kzalloc(sizeof(*platform), GFP_KERNEL);
  

•   if (!platform) {
  

•           DRM_DEV_ERROR(dev, "fail to allocate driver data\n");
  

•           return -ENOMEM;
  

•   }
  

•   pdata = &platform->pdata;
  

•   ret = anx7625_parse_dt(dev, pdata);
  

•   if (ret) {
  

•           DRM_DEV_ERROR(dev, "fail to parse devicetree.\n");
  

Please do not print this error (or at least not if err == -EPROBE_DEFER).

•           goto free_platform;
  

•   }