Re: [PATCHv2 1/2] leds: add driver for LM3530 ALS

[Andrew Morton <akpm@linux-foundation.org>]

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);
> +}
> +
>
> ...
>