| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55DBBB3F.6030205@ti.com>
Date: Mon, 24 Aug 2015 19:47:59 -0500
From: "Franklin S Cooper Jr." <fcooper@...com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: <robh+dt@...nel.org>, <pawel.moll@....com>, <mark.rutland@....com>,
<ijc+devicetree@...lion.org.uk>, <galak@...eaurora.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-input@...r.kernel.org>
Subject: Re: [PATCH] Input: edt-ft5x06 - Switch to newer gpio framework
On 08/24/2015 07:17 PM, Dmitry Torokhov wrote:
> On Mon, Aug 24, 2015 at 05:16:15PM -0500, Franklin S Cooper Jr. wrote:
>>
>> On 08/24/2015 03:56 PM, Dmitry Torokhov wrote:
>>> On Mon, Aug 24, 2015 at 03:23:43PM -0500, Franklin S Cooper Jr. wrote:
>>>> On 08/24/2015 03:16 PM, Franklin S Cooper Jr. wrote:
>>>>> On 08/24/2015 03:01 PM, Dmitry Torokhov wrote:
>>>>>> On Mon, Aug 24, 2015 at 02:48:36PM -0500, Franklin S Cooper Jr. wrote:
>>>>>>> On 08/24/2015 02:41 PM, Dmitry Torokhov wrote:
>>>>>>>> On Fri, Aug 21, 2015 at 02:08:32PM -0500, Franklin S Cooper Jr wrote:
>>>>>>>>> The current/old gpio framework used doesn't properly listen to
>>>>>>>>> ACTIVE_LOW and ACTIVE_HIGH flags. The newer gpio framework takes into
>>>>>>>>> account these flags when setting gpio values.
>>>>>>>>>
>>>>>>>>> Also use gpiod_set_value_cansleep since wake and reset pins can be
>>>>>>>>> provided by bus based io expanders.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@...com>
>>>>>>>>> ---
>>>>>>>>> .../bindings/input/touchscreen/edt-ft5x06.txt | 4 +-
>>>>>>>>> drivers/input/touchscreen/edt-ft5x06.c | 115 +++++++--------------
>>>>>>>>> include/linux/input/edt-ft5x06.h | 4 +-
>>>>>>>>> 3 files changed, 43 insertions(+), 80 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> index 76db967..9330d4d 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>>>>>>>> @@ -50,6 +50,6 @@ Example:
>>>>>>>>> pinctrl-0 = <&edt_ft5x06_pins>;
>>>>>>>>> interrupt-parent = <&gpio2>;
>>>>>>>>> interrupts = <5 0>;
>>>>>>>>> - reset-gpios = <&gpio2 6 1>;
>>>>>>>>> - wake-gpios = <&gpio4 9 0>;
>>>>>>>>> + reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
>>>>>>>>> + wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>;
>>>>>>>>> };
>>>>>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> index 394b1de..6b128b3 100644
>>>>>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>>>>> @@ -91,9 +91,9 @@ struct edt_ft5x06_ts_data {
>>>>>>>>> u16 num_x;
>>>>>>>>> u16 num_y;
>>>>>>>>>
>>>>>>>>> - int reset_pin;
>>>>>>>>> - int irq_pin;
>>>>>>>>> - int wake_pin;
>>>>>>>>> + struct gpio_desc *reset_pin;
>>>>>>>>> + struct gpio_desc *wake_pin;
>>>>>>>>> + struct gpio_desc *irq_pin;
>>>>>>>>>
>>>>>>>>> #if defined(CONFIG_DEBUG_FS)
>>>>>>>>> struct dentry *debug_dir;
>>>>>>>>> @@ -755,36 +755,14 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
>>>>>>>>> static int edt_ft5x06_ts_reset(struct i2c_client *client,
>>>>>>>>> struct edt_ft5x06_ts_data *tsdata)
>>>>>>>>> {
>>>>>>>>> - int error;
>>>>>>>>> -
>>>>>>>>> - if (gpio_is_valid(tsdata->wake_pin)) {
>>>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>>>> - tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
>>>>>>>>> - "edt-ft5x06 wake");
>>>>>>>>> - if (error) {
>>>>>>>>> - dev_err(&client->dev,
>>>>>>>>> - "Failed to request GPIO %d as wake pin, error %d\n",
>>>>>>>>> - tsdata->wake_pin, error);
>>>>>>>>> - return error;
>>>>>>>>> - }
>>>>>>>>> -
>>>>>>>>> + if (tsdata->wake_pin) {
>>>>>>>>> msleep(5);
>>>>>>>>> - gpio_set_value(tsdata->wake_pin, 1);
>>>>>>>>> + gpiod_set_value_cansleep(tsdata->wake_pin, 1);
>>>>>>>>> }
>>>>>>>>> - if (gpio_is_valid(tsdata->reset_pin)) {
>>>>>>>>> - /* this pulls reset down, enabling the low active reset */
>>>>>>>>> - error = devm_gpio_request_one(&client->dev,
>>>>>>>>> - tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
>>>>>>>>> - "edt-ft5x06 reset");
>>>>>>>>> - if (error) {
>>>>>>>>> - dev_err(&client->dev,
>>>>>>>>> - "Failed to request GPIO %d as reset pin, error %d\n",
>>>>>>>>> - tsdata->reset_pin, error);
>>>>>>>>> - return error;
>>>>>>>>> - }
>>>>>>>>>
>>>>>>>>> + if (tsdata->reset_pin) {
>>>>>>>>> msleep(5);
>>>>>>>>> - gpio_set_value(tsdata->reset_pin, 1);
>>>>>>>>> + gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>>>>> So this leaves the reset pin active. How exactly was this tested?
>>>>>>> Normally if the output gpio connected to the reset pin is ACTIVE_HIGH then this will take the tsc out of reset since
>>>>>>> the reset pin is active low. However, I have a board that has an inverter between the gpio and reset pin. So if I leave
>>>>>>> the gpio as ACTIVE_HIGH then the inverter would cause the reset pin to go low which will keep it in reset. So instead
>>>>>>> I set the gpio to ACTIVE_LOW which gives me the expected result.
>>>>>> I do not really care about particular board. Assuming that polarity of
>>>>>> the GPIO in DTS is specified correctly the effect of:
>>>>>>
>>>>>> gpiod_set_value_cansleep(tsdata->reset_pin, 1);
>>>>>>
>>>>>> is reset pin being _active_, i.e. the chip is staying in reset state.
>>>>> Setting the reset pin to 1/high will take the tsc out of reset. Setting the pin to 0/low will put the tsc into reset mode.
>>> It seems you are not quite getting gpiod API. Consider:
>>>
>>> For gpios described as GPIO_ACTIVE_LOW:
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is LOW
>>> gpiod_set_value_cansleep(gpio, 0) => GPIO is HIGH
>>>
>>> For gpios described as GPIO_ACTIVE_HIGH:
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is HIGH
>>> gpiod_set_value_cansleep(gpio, 0) => GPIO is LOW
>>>
>>> IOW with gpiod API 1 means logical active and can be either HIGH or LOW,
>>> depending on GPIO definition in DTS.
>>>
>>>>>> By the way, both reset and wake pins are active low according to the
>>>>>> data sheet I found.
>>>>> Your right. Reset and wake are both active low. However, that part of the code is trying to get the tsc out of reset which is
>>>>> why its setting the value to 1. Setting the pin to 1 was being done even before my patch.
>>>>>
>>>>> What has changed was removing the below line which puts the tsc in reset.
>>>>>
>>>>> error = devm_gpio_request_one(&client->dev, tsdata->reset_pin, GPIOF_OUT_INIT_LOW,"edt-ft5x06 reset");
>>>>>
>>>>> However, the above line isn't needed since when I request the gpio I already configured it as an output
>>>>> with a default value of 0.
>>> So the old sequence resulted in GPIO going XXX->0->1 which brought the
>>> chip into reset and out of it. With the sequence you have, assuming
>>> that the GPIO is described as GPIO_ACTIVE_LOW (to match data sheet),
>> I'm not setting the GPIO_ACTIVE_LOW to match the datasheet. I'm setting it to ACTIVE_LOW because
>> of the inverter that is between the gpio and reset pin makes it "appear" as if the gpio is ACTIVE_LOW.
> If the "normal" behavior (according to the data sheet) is active low and
> you have an invertor, then your DTS should be GPIO_ACTIVE_HIGH. And
> people with hardware that does not have invertor will use
> GPIO_ACTIVE_LOW. And the driver should work with both settings.
>
>>> we'll be getting XXX->1->0:
>>>
>>> devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW) =>
>>> GPIO is *HIGH*
>>>
>>> gpiod_set_value_cansleep(gpio, 1) => GPIO is *LOW*
>> So everything you stated is exactly what I wanted and expected when you take into account the inverter will invert the gpio signal before
>> it reaches the reset pin.
> I do not want to take into account invertor, that is what DTS polarity
> flag for. Regular setup for this touchscreen - GPIO_ACTIVE_LOW, setup
> with invertor - GPIO_ACTIVE_HIGH. From the driver perspective you should
> be leaving the power on/reset function with reset and wake GPIOs
> *inactive* (as in gpiod_set_value_cansleep(gpio, 0)).
Ok. I was talking to someone about this and I believe I understand what your saying. So
essentially the set_value call before my patch was based on voltage while the new gpio
is based on logic.
So the solution would be to change gpiod_set_value_cansleep to 0. Then my hardware inverted GPIO
signal will be set to GPIO_ACTIVE_HIGH in dt. I can then tweak all the DTS files currently using this
driver in mainline to reflect this change.
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists