Re: [PATCH 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards

Hi,

Thanks for looking into

On Thu, Feb 7, 2013 at 4:59 PM, Andi Shyti [off-list ref] wrote:

Hi,

quoted

TC358765 is DSI-to-LVDS transmitter from Toshiba, used in
OMAP44XX Blaze Tablet and Blaze Tablet2 boards.

I had a really fast look and I have few comments

quoted

+static int tc358765_read_register(struct omap_dss_device *dssdev,
+                                     u16 reg, u32 *val)
+{
+     int ret = 0;
+     pm_runtime_get_sync(&dssdev->dev);
+     /* I2C is preferred way of reading, but fall back to DSI
+      * if I2C didn't got initialized
+     */
+     if (tc358765_i2c)
+             ret = tc358765_i2c_read(reg, val);
+     else
+             ret = tc358765_dsi_read(dssdev, reg, val);
+     pm_runtime_put_sync(&dssdev->dev);
+
+     return ret;
+}
+
+static int tc358765_write_register(struct omap_dss_device *dssdev, u16 reg,
+             u32 value)
+{
+     struct tc358765_data *d2d = dev_get_drvdata(&dssdev->dev);
+     u8 buf[6];
+     int r;
+
+     buf[0] = (reg >> 0) & 0xff;
+     buf[1] = (reg >> 8) & 0xff;
+     buf[2] = (value >> 0) & 0xff;
+     buf[3] = (value >> 8) & 0xff;
+     buf[4] = (value >> 16) & 0xff;
+     buf[5] = (value >> 24) & 0xff;
+
+     r = dsi_vc_generic_write_nosync(dssdev, d2d->channel1, buf, 6);
+     if (r)
+             dev_err(&dssdev->dev, "reg write reg(%x) val(%x) failed: %d\n",
+                            reg, value, r);
+     return r;
+}
+
+/****************************
+********* DEBUG *************
+****************************/
+#ifdef CONFIG_TC358765_DEBUG
+static int tc358765_write_register_i2c(u16 reg, u32 val)
+{
+     int ret = -ENODEV;
+     unsigned char buf[6];
+     struct i2c_msg msg;
+
+     if (!tc358765_i2c) {
+             dev_err(tc358765_debug.dev, "%s: I2C not initilized\n",
+                                                     __func__);
+             return ret;
+     }
+
+     buf[0] = (reg >> 8) & 0xff;
+     buf[1] = (reg >> 0) & 0xff;
+     buf[2] = (val >> 0) & 0xff;
+     buf[3] = (val >> 8) & 0xff;
+     buf[4] = (val >> 16) & 0xff;
+     buf[5] = (val >> 24) & 0xff;
+     msg.addr = tc358765_i2c->client->addr;
+     msg.len = sizeof(buf);
+     msg.flags = 0;
+     msg.buf = buf;
+
+     mutex_lock(&tc358765_i2c->xfer_lock);
+     ret = i2c_transfer(tc358765_i2c->client->adapter, &msg, 1);
+     mutex_unlock(&tc358765_i2c->xfer_lock);
+
+     if (ret != 1)
+             return ret;
+     return 0;
+}

What about using smbus?

OMAP2+ SoCs have full I2C controller so panel drivers traditionally use I2C.
Since implementation is OMAP-specific, I don't think we need to use smbus here.

Also please look at other panels implementation:
drivers/video/omap2/displays/panel-tfp410.c
drivers/video/omap2/displays/panel-picodlp.c

quoted

+
+     if (copy_from_user(&buf, ubuf, size))
+             return -EFAULT;
+
+     buf[size-1] = '\0';
+     if (sscanf(buf, "%s %x", name, &value) != 2) {
+             dev_err(dev, "%s: unable to parse input\n", __func__);
+             return -1;
+     }
+
+     if (!tc358765_i2c) {
+             dev_warn(dev,
+                     "failed to write register: I2C not initialized\n");
+             return -ENODEV;
+     }
+
+     reg_count = sizeof(tc358765_regs) / sizeof(tc358765_regs[0]);
+     for (i = 0; i < reg_count; i++) {
+             if (!strcmp(name, tc358765_regs[i].name)) {
+                     if (!(tc358765_regs[i].perm & A_WO)) {
+                             dev_err(dev, "%s is write-protected\n", name);
+                             return -EACCES;
+                     }
+
+                     error = tc358765_write_register_i2c(
+                             tc358765_regs[i].reg, value);
+                     if (error) {
+                             dev_err(dev, "%s: failed to write %s\n",
+                                     __func__, name);
+                             return -1;

Could avoid returning -1 instead of returning a correct errno?

Agree. Will be fixed in next patchset

quoted

+     gpio_set_value(dssdev->reset_gpio, 1);
+     udelay(200);
+     /* reset the panel */
+     gpio_set_value(dssdev->reset_gpio, 0);
+     /* assert reset */
+     udelay(200);
+     gpio_set_value(dssdev->reset_gpio, 1);
+     /* wait after releasing reset */
+     msleep(200);

I invite you to have a look at

Documentation/timers/timers-howto.txt

Will be fixed in next patchset

quoted

+     /* reset LVDS-PHY */
+     tc358765_write_register(dssdev, LVPHY0, (1 << 22));
+     mdelay(2);

You should give me a really good reason for using mdelay.

Will be fixed in next patchset

quoted

+     if (r) {
+             dev_err(&dssdev->dev, "failed to configure DSI pins\n");
+             goto err_disp_enable;
+     };

        ^^^

???

Just a typo. Will be fixed in next patchset

quoted

+static int tc358765_i2c_probe(struct i2c_client *client,
+                                const struct i2c_device_id *id)
+{
+     tc358765_i2c = kzalloc(sizeof(*tc358765_i2c), GFP_KERNEL);

what about devm_kzalloc?

Will be fixed in next patchset

-
Ruslan