On Tue, Jul 07, 2020 at 03:01:25PM +0800, Nicolas Boichat wrote:
Hi Nicolas, thanks for the replay.
> On Tue, Jun 9, 2020 at 3:20 PM Xin Ji <xji@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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).
OK
>
> > + }
> > +
> > + 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?
I'll move the related code to here.
>
> > + 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.
OK
>
> > + }
> > +
> > + 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
OK, also I'll remove drm_panel based on Sam comment.
>
> > + return -EPROBE_DEFER;
>
> -EINVAL
OK
>
> > + }
> > +
> > + 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)
I'll remove drm_panel, use panel_bridge.
>
> > +
> > + 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).
OK
>
> > + goto free_platform;
> > + }
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
