lkml.org 
[lkml]   [2012]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] add LED driver for PCA9663 I2C chip
On Wed, 18 Jan 2012 20:51:29 +0100
Peter Meerwald <pmeerw@pmeerw.net> wrote:

> From: Peter Meerwald <p.meerwald@bct-electronic.com>
>
> v2: address Lars-Peter's comments
> * remove unnecessary spinlock
> * remove printk()
> * change return code to EINVAL for invalid platform data
> * use module_i2c_driver()

The patch is missing its changelog and has no signed-off-by:.
Documentation/SubmittingPatches discusses these things.

>
> ...
>
> +static void pca9633_led_work(struct work_struct *work)
> +{
> + struct pca9633_led *pca9633 = container_of(work,
> + struct pca9633_led, work);
> +

Unneeded newline here.

> + u8 ledout = i2c_smbus_read_byte_data(pca9633->client, PCA9633_LEDOUT);
> + int shift = 2 * pca9633->led_num;
> + u8 mask = 0x3 << shift;
> +
> + switch (pca9633->brightness) {
> + case LED_FULL:
> + i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> + (ledout & ~mask) | (PCA9633_LED_ON << shift));
> + break;
> + case LED_OFF:
> + i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> + ledout & ~mask);
> + break;
> + default:
> + i2c_smbus_write_byte_data(pca9633->client,
> + PCA9633_PWM_BASE + pca9633->led_num,
> + pca9633->brightness);
> + i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> + (ledout & ~mask) | (PCA9633_LED_PWM << shift));
> + break;
> + }
> +}
> +
>
> ...
>
> +static int __devinit pca9633_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct pca9633_led *pca9633;
> + struct i2c_adapter *adapter;
> + struct led_platform_data *pdata;
> + int i, err;
> +
> + adapter = to_i2c_adapter(client->dev.parent);
> + pdata = client->dev.platform_data;
> +
> + if (pdata) {
> + if (pdata->num_leds <= 0 || pdata->num_leds > 4) {
> + dev_err(&client->dev, "board info must claim at most 4 LEDs");
> + return -EINVAL;
> + }
> + }
> +
> + pca9633 = kzalloc(sizeof(*pca9633) * 4, GFP_KERNEL);

Could use kcalloc() here, although that's rather a pointless change.

> + if (!pca9633)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, pca9633);
> +
> + for (i = 0; i < 4; i++) {
> + pca9633[i].client = client;
> + pca9633[i].led_num = i;
> +
> + /* Platform data can specify LED names and default triggers */
> + if (pdata && i < pdata->num_leds) {
> + if (pdata->leds[i].name)
> + snprintf(pca9633[i].name,
> + sizeof(pca9633[i].name), "pca9633:%s",
> + pdata->leds[i].name);
> + if (pdata->leds[i].default_trigger)
> + pca9633[i].led_cdev.default_trigger =
> + pdata->leds[i].default_trigger;
> + } else {
> + snprintf(pca9633[i].name, sizeof(pca9633[i].name),
> + "pca9633:%d", i);
> + }
> +
> + pca9633[i].led_cdev.name = pca9633[i].name;
> + pca9633[i].led_cdev.brightness_set = pca9633_led_set;
> +
> + INIT_WORK(&pca9633[i].work, pca9633_led_work);
> +
> + err = led_classdev_register(&client->dev, &pca9633[i].led_cdev);
> + if (err < 0)
> + goto exit;
> + }
> +
> + /* Disable LED all-call address and set normal mode */
> + i2c_smbus_write_byte_data(client, PCA9633_MODE1, 0x00);
> +
> + /* Turn off LEDs */
> + i2c_smbus_write_byte_data(client, PCA9633_LEDOUT, 0x00);
> +
> + return 0;
> +
> +exit:
> + while (i--) {
> + led_classdev_unregister(&pca9633[i].led_cdev);
> + cancel_work_sync(&pca9633[i].work);
> + }

This (untested!) loop has an off-by-one error. You want

do {
...
} while (i--);


> + kfree(pca9633);
> +
> + return err;
> +}
> +
>
> ...
>



\
 
 \ /
  Last update: 2012-01-31 01:31    [W:0.044 / U:30.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site