| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Feb 2013 15:59:52 +0100
From: Andi Shyti <andi@...zian.org>
To: Ruslan Bilovol <ruslan.bilovol@...com>
Cc: tomi.valkeinen@...com, FlorianSchandinat@....de,
linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-omap@...r.kernel.org
Subject: Re: [PATCH 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards
Hi,
> 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
> +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?
> +
> + 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?
> + 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
> + /* reset LVDS-PHY */
> + tc358765_write_register(dssdev, LVPHY0, (1 << 22));
> + mdelay(2);
You should give me a really good reason for using mdelay.
> + if (r) {
> + dev_err(&dssdev->dev, "failed to configure DSI pins\n");
> + goto err_disp_enable;
> + };
^^^
???
> +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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists