RE: [PATCH] backlight: add new lp855x backlight driver

[Kim, Milo]

-----Original Message-----
From: Andrew Morton [mailto:akpm@xxxxxxxxxxxxxxxxxxxx] 
Sent: Saturday, January 21, 2012 9:24 AM
To: Kim, Milo
Cc: linux-kernel@xxxxxxxxxxxxxxx; Kim, Milo; Richard Purdie
Subject: Re: [PATCH] backlight: add new lp855x backlight driver

On Thu, 5 Jan 2012 23:00:24 -0800
"Kim, Milo" <Milo.Kim@xxxxxx> wrote:


>> +       len = snprintf(buf, sizeof(buf), "%s\n", help);

> `len' should have type size_t.

>> +       if (len > sizeof(buf))
>> +               len = sizeof(buf);

> Here you could use max().  But a better approach is to form the output
> using scnprintf().

This code can be replaced with scnprintf().
But in the updated driver, debugfs nodes will be removed.
The 'chip_id' and 'bl_ctl_mode' will be moved to the lp855x device attributes.
And register access is not necessary in lp855x driver.

>> +static char *lp855x_parse_register_cmd(const char *cmd, u8 *byte)
>> +{
>> +       char tmp[10];
>> +       char *blank;
>> +       unsigned long arg;
>> +
>> +       blank = strchr(cmd, ' ');
>> +       memset(tmp, 0x0, sizeof(tmp));
>> +       memcpy(tmp, cmd, blank - cmd);
>> +
>> +       if (strict_strtol(tmp, 16, &arg) < 0)
>> +               return NULL;

> Gee, what's all this code doing?  Please add a nice comment explaining
> what the input format is, and what this function is trying to do with
> it.

> I worry about what it does when strchr() returns NULL!

Will be removed in the updated driver.

>> +static ssize_t lp855x_ctrl_register(struct file *file,
>> +                                   const char __user *userbuf, size_t count,
>> +                                   loff_t *ppos)
>> +{
>> +       char mode, buf[20];
>> +       char *pos, *pos2;
>> +       u8 i, arg1, arg2, val;
>> +       struct lp855x *lp = file->private_data;
>> +
>> +       if (copy_from_user(buf, userbuf, min(count, sizeof(buf))))
>> +               return -EFAULT;

> Looks risky.  If count>sizeof(buf), this will quietly truncate the
> user's input.  It would be much better to reject the input in this
> case.

Will be removed in the updated driver.

>> +static ssize_t lp855x_get_chipid(struct file *file, char __user *userbuf,
>> +                                size_t count, loff_t *ppos)
>> +{
>> +       struct lp855x *lp = file->private_data;
>> +       char buf[10];
>> +       unsigned int len;
>> +
>> +       len = snprintf(buf, sizeof(buf), "%s\n", lp->chipid);
>> +
>> +       if (len > sizeof(buf))
>> +               len = sizeof(buf);

> See above.

This function will be moved to the lp855x device attribute.
And snprintf() and sizeof(buf) will be replaced with scnprintf().

>> +       return simple_read_from_buffer(userbuf, count, ppos, buf, len);
>> +}
>> +
>> +static ssize_t lp855x_get_bl_mode(struct file *file, char __user *userbuf,
>> +                                 size_t count, loff_t *ppos)
>> +{
>> +       char buf[20];
>> +       unsigned int len;
>> +       char *strmode = NULL;
>> +       struct lp855x *lp = file->private_data;
>> +       enum lp855x_brightness_ctrl_mode mode = lp->pdata->mode;
>> +
>> +       if (mode == PWM_BASED)
>> +               strmode = "pwm based";
>> +       else if (mode == REGISTER_BASED)
>> +               strmode = "register based";
>> +
>> +       len = snprintf(buf, sizeof(buf), "%s\n", strmode);
>> +
>> +       if (len > sizeof(buf))
>> +               len = sizeof(buf);

> More...

This function also will be moved to the device attribute.
And snprintf() and sizeof(buf) will be replaced with scnprintf().

>> +static void lp855x_create_debugfs(struct lp855x *lp)
>> +{
>> +       struct debug_dentry *dd = &lp->dd;
>> +
>> +       dd->dir = debugfs_create_dir("lp855x", NULL);
>> +
>> +       dd->reg = debugfs_create_file("registers", S_IWUSR | S_IRUGO,
>> +                                     dd->dir, lp, &dbg_registers_fops);
>> +
>> +       dd->chip = debugfs_create_file("chip_id", S_IRUGO,
>> +                                      dd->dir, lp, &dbg_chip_fops);
>> +
>> +       dd->blmode = debugfs_create_file("bl_ctl_mode", S_IRUGO,
>> +                                        dd->dir, lp, &dbg_blmode_fops);

> Error checking?

Return code should be checked.
But the 'register' node will be removed in new lp855x driver patch.
The 'chip_id' and 'bl_ctl_mode' will be moved to lp855x device attribute.

>> +static void lp855x_init_device(struct lp855x *lp)
>> +{
>> +       u8 val, addr;
>> +       int i, ret;
>> +       struct lp855x_platform_data *pd = lp->pdata;
>> +
>> +       val = pd->initial_brightness;
>> +       ret = lp855x_write_byte(lp, BRIGHTNESS_CTRL, val);
>> +
>> +       val = pd->device_control;
>> +       ret |= lp855x_write_byte(lp, DEVICE_CTRL, val);
>> +
>> +       if (pd->load_new_rom_data && pd->size_program) {
>> +               for (i = 0; i < pd->size_program; i++) {
>> +                       addr = pd->rom_data[i].addr;
>> +                       val = pd->rom_data[i].val;
>> +                       if (!lp855x_is_valid_rom_area(lp, addr))
>> +                               continue;
>> +
>> +                       ret |= lp855x_write_byte(lp, addr, val);
>> +               }
>> +       }
>> +
>> +       if (ret)
>> +               dev_err(lp->dev, "i2c write err\n");
>> +}

> This isn't very good.  lp855x_write_byte() can return various -Efoo
> values: -EINVAL, -ENOMEM, etc.  But this function can end up
> bitwise-ORing those errnos together, thus producing a completely new
> (and wrong) errno.

> That's not a big problem in this case, because that errno is simply
> dropped on the floor.  However it would be more useful if the errno
> were reported to the operator in that dev_err() call.

I totally agree with your opinion.
These return code should not be manipulated and returned on xxx_probe().
It will be fixed in new driver patch.

>> +static int lp855x_backlight_register(struct lp855x *lp)
>> +{
>> +       struct backlight_device *bl;
>> +       struct backlight_properties props;
>> +       const char *name = lp->pdata->name;
>> +
>> +       if (!name)
>> +               return -ENODEV;
>> +
>> +       props.brightness = lp->pdata->initial_brightness;
>> +       props.max_brightness =
>> +               (lp->pdata->max_brightness < lp->pdata->initial_brightness) ?
>> +               255 : lp->pdata->max_brightness;
>> +
>> +       bl = backlight_device_register(name, lp->dev, lp,
>> +                                      &lp855x_bl_ops, &props);
>> +       if (IS_ERR(bl))
>> +               return -EIO;

> If `lb' contains an errno, we should return that errno to the caller
> rather than unconditionally overwriting it with -EIO?

That code will be changed below.

If (IS_ERR(bl))
	return PTR_ERR(bl);

I appreciate your help and detailed advice.
Updated lp855x driver will be patched as [PATCH v2].

Thanks & BR
Milo -

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/