Re: [PATCH] leds: add driver for PCA9663 I2C chip

[Peter Meerwald]

Hello,

> > simple driver for the PCA9633 I2C chip supporting four LEDs and
> > 255 brightness levels

> Looks good, just a few minor issues.

thanks for the timely review and suggestions; I'll respond to our comments 
below and resubmit a v2 shortly

the driver is based on leds-pca955x.c, all your comments apply there as 
well :)

> > +static void pca9633_led_set(struct led_classdev *led_cdev,
> > +	enum led_brightness value)
> > +	spin_lock(&pca9633->lock);
> Does the spinlock actually protect against anything?

removed it

> > +	printk(KERN_INFO "leds-pca9633: LED driver at slave address 0x%02x\n",
> > +		client->addr);
> dev_info, but it probably wouldn't hurt either if the message is removed
> altogether.

dropped as suggested

> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> > +		return -EIO;
> You check for I2C functionality, but only ever use the smbus subset, which
> should be supported by all adapters anyway, so you should be safe even without
> the check.

dropped

> > +			dev_err(&client->dev, "board info must claim at most 4 LEDs");
> > +			return -ENODEV;
> -EINVAL?

changed

> > +			if (pdata->leds[i].name)
> > +				snprintf(pca9633[i].name,
> > +					 sizeof(pca9633[i].name), "pca9633:%s",
> > +					 pdata->leds[i].name);
> Why the prefix?

pca955x does it the same way; I'd rather keep it so one knowns where a LED 
is connected

> > +module_init(pca9633_leds_init);
> > +module_exit(pca9633_leds_exit);
> Use the new module_i2c_driver macro.

agree; I've submitted a patch to linux-i2c to mentionen the macro in 
Documentation/i2c/writing-clients

thanks, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/