On 08/11/2017 05:37 PM, Pavel Machek wrote:
> 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
I don't think so as this is just to read the gpio value.
I suppose that the i2c layer will output some errors before
if it catches some invalid state.
C.
>>>> +#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
>
