lkml.org 
[lkml]   [2008]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] leds: Add support for Philips PCA955x I2C LED drivers
    On Wed, 14 May 2008 18:47:46 -0500
    Nate Case <ncase@xes-inc.com> wrote:

    > This driver supports the PCA9550, PCA9551, PCA9552, and PCA9553
    > LED driver chips.
    >
    > ...
    >
    > +/* Set only the two bits in the LED selector register of the specified LED */
    > +#define LED_SET(reg, led, state) \
    > + ((reg & (~(0x3 << (led << 1)))) | ((state & 0x0F) << (led << 1)))

    Please prefer to implement code in C rather than as a macro. Only use
    macros when the code *must* be a macro.

    For many reasons, one of which is: the above expression references
    `led' twice and will misbehave if passed an expression which has
    side-effects.

    static inline void led_set(suitabletype reg, suitabletype led,
    suitabletype state)

    would suit.

    (ditto NUM_INPUT_REGS, etc. Writing it in C will generate equivalent
    code but we get a nicely lower-cased C function)

    > +enum pca955x_type {
    > + pca9550,
    > + pca9551,
    > + pca9552,
    > + pca9553,
    > +};
    > +
    > +struct pca955x_chipdef {
    > + int bits;
    > + u8 slv_addr; /* 7-bit slave address mask */
    > + int slv_addr_shift; /* Number of bits to ignore */
    > +};
    > +
    > +static struct pca955x_chipdef pca955x_chipdefs[] = {
    > +[pca9550] = {
    > + .bits = 2,
    > + .slv_addr = /* 110000x */ 0x60,
    > + .slv_addr_shift = 1,
    > +},
    > +[pca9551] = {
    > + .bits = 8,
    > + .slv_addr = /* 1100xxx */ 0x60,
    > + .slv_addr_shift = 3,
    > +},
    > +[pca9552] = {
    > + .bits = 16,
    > + .slv_addr = /* 1100xxx */ 0x60,
    > + .slv_addr_shift = 3,
    > +},
    > +[pca9553] = {
    > + .bits = 4,
    > + .slv_addr = /* 110001x */ 0x62,
    > + .slv_addr_shift = 1,
    > +},
    > +};

    the innards of the above should be tabbed one stop to the right.

    > +static const struct i2c_device_id pca955x_id[] = {
    > + { "pca9550", pca9550 },
    > + { "pca9551", pca9551 },
    > + { "pca9552", pca9552 },
    > + { "pca9553", pca9553 },
    > +};

    oops, we forgot the terminating { } entry. It will crash...

    > +MODULE_DEVICE_TABLE(i2c, pca955x_id);
    >
    > +struct pca955x_led {
    > + struct pca955x_chipdef *chipdef;
    > + struct i2c_client *client;
    > + struct work_struct work;
    > + spinlock_t lock;
    > + enum led_brightness brightness;
    > + struct led_classdev led_cdev;
    > + int led_num; /* 0 .. 15 potentially */
    > + char name[32];
    > +};
    > +
    > +/*
    > + * Write to frequency prescaler register, used to program the
    > + * period of the PWM output. period = (PSCx + 1) / 38
    > + */
    > +static void pca955x_write_psc(struct i2c_client *client, int n, u8 val)
    > +{
    > + struct pca955x_led *pca955x = i2c_get_clientdata(client);

    nanonit: most kernel code uses a single space between the type and the
    identifier.

    > +
    > + i2c_smbus_write_byte_data(client,
    > + NUM_INPUT_REGS(pca955x->chipdef->bits) + 2*n,
    > + val);
    > +}
    >
    > ...
    >
    > +void pca955x_led_set(struct led_classdev *led_cdev, enum led_brightness value)
    > +{
    > + struct pca955x_led *pca955x =
    > + container_of(led_cdev, struct pca955x_led, led_cdev);

    You're allowed to do

    struct pca955x_led *pca955x;

    pca955x = container_of(led_cdev, struct pca955x_led, led_cdev);

    ;)

    > + spin_lock(&(pca955x->lock));

    unneeded parens

    > + pca955x->brightness = value;
    > +
    > + /*
    > + * Must use workqueue for the actual I/O since I2C operations
    > + * can sleep.
    > + */
    > + schedule_work(&(pca955x->work));
    > +
    > + spin_unlock(&(pca955x->lock));

    dittoes.

    > +}
    > +
    > +static int __devinit pca955x_probe(struct i2c_client *client,
    > + const struct i2c_device_id *id)
    > +{
    > + struct pca955x_led *pca955x;
    > + int i;
    > + int err = -ENODEV;
    > + struct pca955x_chipdef *chip =
    > + &pca955x_chipdefs[id->driver_data];
    > + struct i2c_adapter *adapter =
    > + to_i2c_adapter(client->dev.parent);
    > + struct led_platform_data *pdata = client->dev.platform_data;
    > +
    > + DBG("probe() enter: chip 0x%02x, with type='%s'\n", client->addr,
    > + id->name);
    > +
    > + /* Make sure the slave address / chip type combo given is possible */
    > + if (((client->addr >> chip->slv_addr_shift) << chip->slv_addr_shift)
    > + != chip->slv_addr) {

    equivalent to the simpler

    if (client->addr & ((1 << chip->slv_addr_shift) - 1)) {

    I think?

    > + dev_err(&client->dev, "invalid slave address %02x\n",
    > + client->addr);
    > + return -ENODEV;
    > + }
    > +
    > + printk(KERN_INFO "leds-pca955x: Using %s %d-bit LED driver at "
    > + "slave address 0x%02x\n",
    > + id->name, chip->bits, client->addr);
    > +
    > + DBG("probe() checking i2c functionality\n");
    > + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
    > + return -EIO;
    > +
    > + if (pdata) {
    > + if (pdata->num_leds != chip->bits) {
    > + dev_err(&client->dev, "board info claims %d LEDs"
    > + " on a %d-bit chip\n",
    > + pdata->num_leds, chip->bits);
    > + return -ENODEV;
    > + }
    > + }
    > +
    > + for (i = 0; i < chip->bits; i++) {
    > + pca955x = kzalloc(sizeof(struct pca955x_led), GFP_KERNEL);
    > + if (!pca955x) {
    > + err = -ENOMEM;
    > + goto exit;
    > + }
    > +
    > + pca955x->chipdef = chip;
    > + pca955x->client = client;
    > + pca955x->led_num = i;
    > + /* Platform data can specify LED names and default triggers */
    > + if (pdata) {
    > + if (pdata->leds[i].name)
    > + snprintf(pca955x->name, 32, "pca955x:%s",
    > + pdata->leds[i].name);
    > + if (pdata->leds[i].default_trigger)
    > + pca955x->led_cdev.default_trigger =
    > + pdata->leds[i].default_trigger;
    > + } else {
    > + snprintf(pca955x->name, 32, "pca955x:%d", i);
    > + }
    > + spin_lock_init(&pca955x->lock);
    > +
    > + pca955x->led_cdev.name = pca955x->name;
    > + pca955x->led_cdev.brightness_set =
    > + pca955x_led_set;
    > +
    > + /*
    > + * Client data is a pointer to the _first_ pca955x_led
    > + * struct
    > + */
    > + if (i == 0)
    > + i2c_set_clientdata(client, pca955x);
    > +
    > + INIT_WORK(&(pca955x->work), pca955x_led_work);
    > +
    > + led_classdev_register(&client->dev, &(pca955x->led_cdev));
    > + }
    > +
    > + /* Turn off LEDs */
    > + for (i = 0; i < NUM_LED_REGS(chip->bits); i++)
    > + pca955x_write_ls(client, i, 0x55);
    > +
    > + /* PWM0 is used for half brightness or 50% duty cycle */
    > + pca955x_write_pwm(client, 0, 255-LED_HALF);
    > +
    > + /* PWM1 is used for variable brightness, default to OFF */
    > + pca955x_write_pwm(client, 1, 0);
    > +
    > + /* Set to fast frequency so we do not see flashing */
    > + pca955x_write_psc(client, 0, 0);
    > + pca955x_write_psc(client, 1, 0);
    > +
    > + DBG("probe() returning\n");
    > +
    > + return 0;
    > +exit:
    > + return err;
    > +}
    > +


    It's a neat-looking driver.


    \
     
     \ /
      Last update: 2008-05-16 01:37    [W:0.037 / U:0.528 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site