Message-Id: <20080515163331.0677f4b1.akpm@linux-foundation.org>
Date:	Thu, 15 May 2008 16:33:31 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Nate Case <ncase@...-inc.com>
Cc:	rpurdie@...ys.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: Add support for Philips PCA955x I2C LED drivers

On Wed, 14 May 2008 18:47:46 -0500
Nate Case <ncase@...-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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists