lkml.org 
[lkml]   [2011]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv2 1/2] leds: add driver for LM3530 ALS
On Thu, 20 Jan 2011 15:40:49 +0530
Shreshtha Kumar SAHU <shreshthakumar.sahu@stericsson.com> wrote:

> From: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
>
> simple backlight driver for National Semiconductor LM3530.
> Presently only manual mode is supported, PWM and ALS support
> to be added.
>
>
> ...
>
> +static int lm3530_get_mode_from_str(const char *str, int count)
> +{
> + int i;
> +
> + if (str[0] == '\n')
> + return -1;

Why is this here? I think the function would work OK if it was removed?

> + for (i = 0; i < LM3530_BL_MODE_MAX; i++)

Could use ARRAY_SIZE(mode_map) here and do away with
LM3530_BL_MODE_MAX.

> + if (!strncmp(mode_map[i].mode, str, count))

Why strncmp? The code will treat input of the form "alsfoo" as "als",
which is sloppy.

> + return mode_map[i].mode_val;
> +
> + return -1;
> +}
> +
>
> ...
>
> +static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
> + *attr, const char *buf, size_t size)
> +{
> + int err;
> + struct i2c_client *client = container_of(
> + dev->parent, struct i2c_client, dev);
> + struct lm3530_data *drvdata = i2c_get_clientdata(client);
> + int mode;
> +
> + mode = lm3530_get_mode_from_str(buf, size-1);

And why the "size-1"? Assuming there's a \n? But userspace should be
able to do write(fd, 3, "als")?

Perhaps all this can be tidied up with

mode = lm3530_get_mode_from_str(strim(buf));

Alternatively, see sysfs_streq() - it was added to address these sorts
of things.

> + if (mode < 0) {
> + dev_err(dev, "Invalid mode\n");
> + return -EINVAL;
> + }
> +
> + if (mode == LM3530_BL_MODE_MANUAL)
> + drvdata->mode = LM3530_BL_MODE_MANUAL;
> + else if (mode == LM3530_BL_MODE_ALS)
> + drvdata->mode = LM3530_BL_MODE_ALS;
> + else if (mode == LM3530_BL_MODE_PWM) {
> + dev_err(dev, "PWM mode not supported\n");
> + return -EINVAL;
> + }
> +
> + err = lm3530_init_registers(drvdata);
> + if (err) {
> + dev_err(dev, "Setting %s Mode failed :%d\n", buf, err);
> + return err;
> + }
> +
> + return sizeof(drvdata->mode);
> +}
> +
>
> ...
>



\
 
 \ /
  Last update: 2011-01-21 01:07    [W:0.037 / U:11.204 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site