Re: [PATCH v8 1/4] drm/bridge: add support for sn65dsi86 bridge driver

On 2018-06-12 05:05, Stephen Boyd wrote:

Quoting Sandeep Panda (2018-06-04 22:40:15)

quoted

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
new file mode 100644
index 0000000..add6e0f
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c

@@ -0,0 +1,666 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+

[...]

quoted

+
+static const struct regmap_config ti_sn_bridge_regmap_config = {
+       .reg_bits = 8,
+       .val_bits = 8,
+       .volatile_table = &ti_sn_bridge_volatile_table,
+       .cache_type = REGCACHE_NONE,
+};
+
+static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
+                                  unsigned int reg, u16 val)
+{
+       regmap_write(pdata->regmap, reg, val & 0xFF);
+       regmap_write(pdata->regmap, reg + 1, val >> 8);
+}
+
+static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
+{
+       struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
+       int ret = 0;

Please don't assign variables and then reassign them again immediately
after. It hides use before real initialization bugs.

Ok.

quoted

+
+       ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, pdata->supplies);

[...]

quoted

+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+        const struct i2c_device_id *id)
+{
+       struct ti_sn_bridge *pdata;
+       struct device_node *ddc_node;
+       struct mipi_dsi_host *host;
+       struct mipi_dsi_device *dsi;
+       int ret = 0;
+       const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+                                                  .channel = 0,
+                                                  .node = NULL,
+                                                };
+
+       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+               DRM_ERROR("device doesn't support I2C\n");
+               return -ENODEV;
+       }
+
+       ret = ti_sn_bridge_parse_dsi_host(pdata);
+       if (ret)
+               return ret;
+
+       host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+       if (!host) {
+               DRM_ERROR("failed to find dsi host\n");

Not sure we want to print an error and then return -EPROBE_DEFER.
Usually EPROBE_DEFER is silent.

quoted

+               ret = -EPROBE_DEFER;
+               goto err_dsi_host;
+       }
+

[...]

quoted

+
+       /* TODO: setting to 4 lanes always for now */
+       dsi->lanes = 4;
+       dsi->format = MIPI_DSI_FMT_RGB888;
+       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+                         MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+       ret = mipi_dsi_attach(dsi);
+       if (ret < 0) {
+               DRM_ERROR("failed to attach dsi to host\n");
+               goto err_dsi_attach;
+       }
+       pdata->dsi = dsi;
+
+       pdata->refclk = devm_clk_get(pdata->dev, "refclk");

We need to check for error

	if (IS_ERR(pdata->refclk))

And then if it's EPROBE_DEFER I suppose we would bail out, otherwise
assume it's not present?

Yes found this issue while testing the driver on actual sn65dsi86 HW, i will fix this in next patchset.
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno