Re: [PATCH] leds: Add WM8350 LED driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Nov 26, 2008 at 06:39:04PM +0100, Sven Wegener wrote:

You should cut irrelevant text from your mail; it makes it much easier
to find what you're actually saying - if you've got hundreds of lines of
unbroken quoted text it's easy to page straight past small comments.

> On Wed, 26 Nov 2008, Mark Brown wrote:

> > +	isink = regulator_get(&pdev->dev, "led_isink");
> > +	if (IS_ERR(isink) || isink == NULL) {
> > +		printk(KERN_ERR "%s: cant get ISINK\n", __func__);
> > +		return PTR_ERR(isink);

> Uhm, if isink is ever NULL, which according to your check above you
> expect it to happen, this will return 0, meaning success. I'm not sure
> if this is what you want and looking at regulator_get() it should never
> be NULL.

No, it should never be NULL - this is was inherited from the out of tree
history.

> > +	for (i = 0; i < ARRAY_SIZE(isink_cur) - 1; i++)

> This loop will skip the last array member.

This is intentional, it ensures that we always end up pointing to a
value within the array.

> > +int wm8350_register_led(struct wm8350 *wm8350, int lednum, int dcdc, int isink,
> > +			struct wm8350_led_platform_data *pdata)
> > +{
> > +	struct wm8350_led *led = &wm8350->pmic.led[lednum];
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	if (lednum > ARRAY_SIZE(wm8350->pmic.led) || lednum < 0) {
> > +		dev_err(wm8350->dev, "Invalid LED index %d\n", lednum);
> > +		return -ENODEV;
> > +	}

> You already used lednum as an array index above, I'm not sure if this
> can have any side effects. I think it shouldn't, as you only use it to
> get a pointer.

It's out of spec to index beyond the end of the array but shouldn't
produce any actual runtime problems unless the either the implementation
or the platform code is being silly.  I'll shuffle the code around,
though.

Fixed everything else.