Messages in this thread | | | From | "Andrey Panin" <> | Date | Mon, 7 Sep 2009 11:25:07 +0400 | Subject | Re: [PATCH 2/2] leds: Add WM831x status LED driver |
| |
On 248, 09 05, 2009 at 02:09:21PM +0100, Mark Brown wrote: > The WM831x devices feature two software controlled status LEDs with > hardware assisted blinking. > > The device can also autonomously control the LEDs based on a selection > of sources. This can be configured at boot time using either platform > data or the chip OTP. A sysfs file in the style of that for triggers > allowing the control source to be configured at run time. Triggers > can't be used here since they can't depend on the implementation details > of a specific LED type. > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> +static int wm831x_status_probe(struct platform_device *pdev) > +{ > + struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent); > + struct wm831x_pdata *chip_pdata; > + struct wm831x_status_pdata pdata; > + struct wm831x_status *drvdata; > + struct resource *res; > + int id = pdev->id % ARRAY_SIZE(chip_pdata->status); > + int ret; > + > + res = platform_get_resource(pdev, IORESOURCE_IO, 0); > + if (res == NULL) { > + dev_err(&pdev->dev, "No I/O resource\n"); > + ret = -EINVAL; > + goto err;
Why not return -EINVAL right here ?
> + } > + > + drvdata = kzalloc(sizeof(struct wm831x_status), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + dev_set_drvdata(&pdev->dev, drvdata); > + > + drvdata->wm831x = wm831x; > + drvdata->reg = res->start; > + > + if (wm831x->dev->platform_data) > + chip_pdata = wm831x->dev->platform_data; > + else > + chip_pdata = NULL; > + > + memset(&pdata, 0, sizeof(pdata)); > + if (chip_pdata && chip_pdata->status[id]) > + memcpy(&pdata, chip_pdata->status[id], sizeof(pdata)); > + else > + pdata.name = dev_name(&pdev->dev); > + > + mutex_init(&drvdata->mutex); > + INIT_WORK(&drvdata->work, wm831x_status_work); > + spin_lock_init(&drvdata->value_lock); > + > + /* We cache the configuration register and read startup values > + * from it. */ > + drvdata->reg_val = wm831x_reg_read(wm831x, drvdata->reg); > + > + if (drvdata->reg_val & WM831X_LED_MODE_MASK) > + drvdata->brightness = LED_FULL; > + else > + drvdata->brightness = LED_OFF; > + > + /* Set a default source if configured, otherwise leave the > + * current hardware setting. > + */ > + if (pdata.default_src == WM831X_STATUS_PRESERVE) { > + drvdata->src = drvdata->reg_val; > + drvdata->src &= WM831X_LED_SRC_MASK; > + drvdata->src >>= WM831X_LED_SRC_SHIFT; > + } else { > + drvdata->src = pdata.default_src - 1; > + } > + > + drvdata->cdev.name = pdata.name; > + drvdata->cdev.default_trigger = pdata.default_trigger; > + drvdata->cdev.brightness_set = wm831x_status_set; > + drvdata->cdev.blink_set = wm831x_status_blink_set; > + > + ret = led_classdev_register(wm831x->dev, &drvdata->cdev); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to register LED: %d\n", ret); > + goto err_led; > + } > + > + ret = device_create_file(drvdata->cdev.dev, &dev_attr_src); > + if (ret != 0) > + dev_err(&pdev->dev, > + "No source control for LED: %d\n", ret); > + > + return 0; > + > +err_led: > + led_classdev_unregister(&drvdata->cdev);
This line looks useless and possibly unsafe, led device was not registered. Also this part of code can be removed if you move next line up.
> + kfree(drvdata); > +err: > + return ret; > +}
| |