Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

Hi!

> > with minor comments below.
> > 
> >> @@ -125,6 +130,7 @@ struct pca955x_led {
> >>  	struct led_classdev	led_cdev;
> >>  	int			led_num;	/* 0 .. 15 potentially */
> >>  	char			name[32];
> >> +	u32			type;
> > 
> > u32 is highly non-standard here. enum pca955x_led_type?
> >
> > 
> >> +/*
> >> + * Read the INPUT register, which contains the state of LEDs.
> >> + */
> >> +static u8 pca955x_read_input(struct i2c_client *client, int n)
> >> +{
> >> +	return (u8)i2c_smbus_read_byte_data(client, n);
> >> +}
> > 
> > Is the cast needed? Should you attempt to handle errors?
> 
> ah. this is a left over. I can fix in a resend.

No need to resend just for this.

Should you WARN_ON() if the read_byte_data returns < 0 or something?

https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html

> >> +#ifndef _DT_BINDINGS_LEDS_PCA955X_H
> >> +#define _DT_BINDINGS_LEDS_PCA955X_H
> >> +
> >> +#define PCA955X_TYPE_NONE         0
> >> +#define PCA955X_TYPE_LED          1
> >> +#define PCA955X_TYPE_GPIO         2
> > 
> > And make these into the new enum pca955x_led_type?
> 
> These are used in the device tree bindings, so we should keep the u32
> I think.

Aha, ok, makes sense.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]