lkml.org 
[lkml]   [2014]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output
From
On Mon, Jul 7, 2014 at 10:03 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Sun, Jun 29, 2014 at 2:13 AM, Antonio Borneo
> <borneo.antonio@gmail.com> wrote:
>> CP2112 does not offer an atomic method to set both gpio
>> direction and value.
>> Also it does not permit to set gpio value before putting
>> gpio in output. In fact, accordingly to Silicon Labs
>> AN495, Rev. 0.2, cpt. 4.4, the HID report to set gpio
>> values "does not affect any pins that are not configured
>> as outputs".
>>
>> This is confirmed on evaluation board CP2112-EK.
>> With current driver, after execute:
>> echo in > /sys/class/gpio/gpio248/direction
>> echo low > /sys/class/gpio/gpio248/direction
>> gpio output is still high. Only after a following
>> echo low > /sys/class/gpio/gpio248/direction
>> gpio output gets low.
>>
>> Fix driver by changing order of operations; first set
>> direction then set value.
>>
>> The drawback of this new sequence is that we can have
>> a pulse on gpio pin when direction is changed from
>> input to output-low, but this cannot be avoided on
>> current CP2112.
>
> In this case, does keeping the first cp2112_gpio_set() before setting
> the output direction prevents the pulse?
> If so, then you can just keep the current code, and simply add the
> cp2112_gpio_set() at the end of cp2112_gpio_direction_output().

Hi Benjamin,

unfortunately this would not prevent the pulse.
In fact:

a) if gpio is already output
- the first cp2112_gpio_set() will set the value
- then cp2112_gpio_direction_output() would be redundant
- finally the second cp2112_gpio_set() would be redundant too.

b) if gpio is input
- the first cp2112_gpio_set() would be ignored, as explained in AN495
- then cp2112_gpio_direction_output() would put gpio in output.
Accordingly to my experiments the value is set high
- finally the second cp2112_gpio_set() would set the proper value. If
value is different from what is set by cp2112_gpio_direction_output()
we get a pulse.

That's why I removed the first cp2112_gpio_set() or, better, I moved
it after setting gpio direction.

Thanks for the review.
Antonio

>
> In both case, this is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
>
>>
>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> ---
>> drivers/hid/hid-cp2112.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>> index 56be85a..3952d90 100644
>> --- a/drivers/hid/hid-cp2112.c
>> +++ b/drivers/hid/hid-cp2112.c
>> @@ -240,8 +240,6 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
>> u8 buf[5];
>> int ret;
>>
>> - cp2112_gpio_set(chip, offset, value);
>> -
>> ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
>> sizeof(buf), HID_FEATURE_REPORT,
>> HID_REQ_GET_REPORT);
>> @@ -260,6 +258,12 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
>> return ret;
>> }
>>
>> + /*
>> + * Set gpio value when output direction is already set,
>> + * as specified in AN495, Rev. 0.2, cpt. 4.4
>> + */
>> + cp2112_gpio_set(chip, offset, value);
>> +
>> return 0;
>> }
>>
>> --
>> 2.0.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html


\
 
 \ /
  Last update: 2014-07-11 18:41    [W:0.732 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site