[PATCH] HID: cp2112: fix gpio value in gpio_direction_output

[PATCH] HID: cp2112: fix gpio value in gpio_direction_output

[Antonio Borneo]

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.

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

Re: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output

[Benjamin Tissoires]

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().

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

Re: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output

[Jiri Kosina]

On Mon, 7 Jul 2014, Benjamin Tissoires 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().
> 
> In both case, this is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I am queuing this for 3.17, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: cp2112: fix gpio value in gpio_direction_output

[Antonio Borneo]

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