[PATCH] usb: usb3503: Convert to use GPIO descriptors

[PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

This converts the USB3503 to pick GPIO descriptors from the
device tree instead of iteratively picking out GPIO number
references and then referencing these from the global GPIO
numberspace.

The USB3503 is only used from device tree among the in-tree
platforms. If board files would still desire to use it they can
provide machine descriptor tables.

Make sure to preserve semantics such as the reset delay
introduced by Stefan.

Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/usb/misc/usb3503.c            | 94 ++++++++++-----------------
 include/linux/platform_data/usb3503.h |  3 -
 2 files changed, 35 insertions(+), 62 deletions(-)

diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index 72f39a9751b5..c3c1f65f4196 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -7,11 +7,10 @@
 
 #include <linux/clk.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/usb3503.h>
 #include <linux/regmap.h>
@@ -47,19 +46,19 @@ struct usb3503 {
 	struct device		*dev;
 	struct clk		*clk;
 	u8	port_off_mask;
-	int	gpio_intn;
-	int	gpio_reset;
-	int	gpio_connect;
+	struct gpio_desc	*intn;
+	struct gpio_desc 	*reset;
+	struct gpio_desc 	*connect;
 	bool	secondary_ref_clk;
 };
 
 static int usb3503_reset(struct usb3503 *hub, int state)
 {
-	if (!state && gpio_is_valid(hub->gpio_connect))
-		gpio_set_value_cansleep(hub->gpio_connect, 0);
+	if (!state && hub->connect)
+		gpiod_set_value_cansleep(hub->connect, 0);
 
-	if (gpio_is_valid(hub->gpio_reset))
-		gpio_set_value_cansleep(hub->gpio_reset, state);
+	if (hub->reset)
+		gpiod_set_value_cansleep(hub->reset, state);
 
 	/* Wait T_HUBINIT == 4ms for hub logic to stabilize */
 	if (state)
@@ -115,8 +114,8 @@ static int usb3503_connect(struct usb3503 *hub)
 		}
 	}
 
-	if (gpio_is_valid(hub->gpio_connect))
-		gpio_set_value_cansleep(hub->gpio_connect, 1);
+	if (hub->connect)
+		gpiod_set_value_cansleep(hub->connect, 1);
 
 	hub->mode = USB3503_MODE_HUB;
 	dev_info(dev, "switched to HUB mode\n");
@@ -163,13 +162,11 @@ static int usb3503_probe(struct usb3503 *hub)
 	int err;
 	u32 mode = USB3503_MODE_HUB;
 	const u32 *property;
+	enum gpiod_flags flags;
 	int len;
 
 	if (pdata) {
 		hub->port_off_mask	= pdata->port_off_mask;
-		hub->gpio_intn		= pdata->gpio_intn;
-		hub->gpio_connect	= pdata->gpio_connect;
-		hub->gpio_reset		= pdata->gpio_reset;
 		hub->mode		= pdata->initial_mode;
 	} else if (np) {
 		u32 rate = 0;
@@ -230,59 +227,38 @@ static int usb3503_probe(struct usb3503 *hub)
 			}
 		}
 
-		hub->gpio_intn	= of_get_named_gpio(np, "intn-gpios", 0);
-		if (hub->gpio_intn == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		hub->gpio_connect = of_get_named_gpio(np, "connect-gpios", 0);
-		if (hub->gpio_connect == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
-		if (hub->gpio_reset == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
 		of_property_read_u32(np, "initial-mode", &mode);
 		hub->mode = mode;
 	}
 
-	if (hub->port_off_mask && !hub->regmap)
-		dev_err(dev, "Ports disabled with no control interface\n");
-
-	if (gpio_is_valid(hub->gpio_intn)) {
-		int val = hub->secondary_ref_clk ? GPIOF_OUT_INIT_LOW :
-						   GPIOF_OUT_INIT_HIGH;
-		err = devm_gpio_request_one(dev, hub->gpio_intn, val,
-					    "usb3503 intn");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as interrupt pin (%d)\n",
-				hub->gpio_intn, err);
-			return err;
-		}
-	}
-
-	if (gpio_is_valid(hub->gpio_connect)) {
-		err = devm_gpio_request_one(dev, hub->gpio_connect,
-				GPIOF_OUT_INIT_LOW, "usb3503 connect");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as connect pin (%d)\n",
-				hub->gpio_connect, err);
-			return err;
-		}
-	}
-
-	if (gpio_is_valid(hub->gpio_reset)) {
-		err = devm_gpio_request_one(dev, hub->gpio_reset,
-				GPIOF_OUT_INIT_LOW, "usb3503 reset");
+	if (hub->secondary_ref_clk)
+		flags = GPIOD_OUT_LOW;
+	else
+		flags = GPIOD_OUT_HIGH;
+	hub->intn = devm_gpiod_get_optional(dev, "intn", flags);
+	if (IS_ERR(hub->intn))
+		return PTR_ERR(hub->intn);
+	if (hub->intn)
+		gpiod_set_consumer_name(hub->intn, "usb3503 intn");
+
+	hub->connect = devm_gpiod_get_optional(dev, "connect", GPIOD_OUT_LOW);
+	if (IS_ERR(hub->connect))
+		return PTR_ERR(hub->connect);
+	if (hub->connect)
+		gpiod_set_consumer_name(hub->connect, "usb3503 connect");
+
+	hub->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(hub->reset))
+		return PTR_ERR(hub->reset);
+	if (hub->reset) {
 		/* Datasheet defines a hardware reset to be at least 100us */
 		usleep_range(100, 10000);
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as reset pin (%d)\n",
-				hub->gpio_reset, err);
-			return err;
-		}
+		gpiod_set_consumer_name(hub->reset, "usb3503 reset");
 	}
 
+	if (hub->port_off_mask && !hub->regmap)
+		dev_err(dev, "Ports disabled with no control interface\n");
+
 	usb3503_switch_mode(hub, hub->mode);
 
 	dev_info(dev, "%s: probed in %s mode\n", __func__,
diff --git a/include/linux/platform_data/usb3503.h b/include/linux/platform_data/usb3503.h
index e049d51c1353..d01ef97ddf36 100644
--- a/include/linux/platform_data/usb3503.h
+++ b/include/linux/platform_data/usb3503.h
@@ -17,9 +17,6 @@ enum usb3503_mode {
 struct usb3503_platform_data {
 	enum usb3503_mode	initial_mode;
 	u8	port_off_mask;
-	int	gpio_intn;
-	int	gpio_connect;
-	int	gpio_reset;
 };
 
 #endif
-- 
2.23.0

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 05.12.2019 15:56, Linus Walleij wrote:
> This converts the USB3503 to pick GPIO descriptors from the
> device tree instead of iteratively picking out GPIO number
> references and then referencing these from the global GPIO
> numberspace.
>
> The USB3503 is only used from device tree among the in-tree
> platforms. If board files would still desire to use it they can
> provide machine descriptor tables.
>
> Make sure to preserve semantics such as the reset delay
> introduced by Stefan.
>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

NAK.

Sorry, but this patch breaks USB3503 HUB operation on Arndale5250 board. 
A brief scan through the code reveals that the whole control logic for 
the 'intn' gpio is lost.

> ---
>   drivers/usb/misc/usb3503.c            | 94 ++++++++++-----------------
>   include/linux/platform_data/usb3503.h |  3 -
>   2 files changed, 35 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
> index 72f39a9751b5..c3c1f65f4196 100644
> --- a/drivers/usb/misc/usb3503.c
> +++ b/drivers/usb/misc/usb3503.c
> @@ -7,11 +7,10 @@
>   
>   #include <linux/clk.h>
>   #include <linux/i2c.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/delay.h>
>   #include <linux/slab.h>
>   #include <linux/module.h>
> -#include <linux/of_gpio.h>
>   #include <linux/platform_device.h>
>   #include <linux/platform_data/usb3503.h>
>   #include <linux/regmap.h>
> @@ -47,19 +46,19 @@ struct usb3503 {
>   	struct device		*dev;
>   	struct clk		*clk;
>   	u8	port_off_mask;
> -	int	gpio_intn;
> -	int	gpio_reset;
> -	int	gpio_connect;
> +	struct gpio_desc	*intn;
> +	struct gpio_desc 	*reset;
> +	struct gpio_desc 	*connect;
>   	bool	secondary_ref_clk;
>   };
>   
>   static int usb3503_reset(struct usb3503 *hub, int state)
>   {
> -	if (!state && gpio_is_valid(hub->gpio_connect))
> -		gpio_set_value_cansleep(hub->gpio_connect, 0);
> +	if (!state && hub->connect)
> +		gpiod_set_value_cansleep(hub->connect, 0);
>   
> -	if (gpio_is_valid(hub->gpio_reset))
> -		gpio_set_value_cansleep(hub->gpio_reset, state);
> +	if (hub->reset)
> +		gpiod_set_value_cansleep(hub->reset, state);
>   
>   	/* Wait T_HUBINIT == 4ms for hub logic to stabilize */
>   	if (state)
> @@ -115,8 +114,8 @@ static int usb3503_connect(struct usb3503 *hub)
>   		}
>   	}
>   
> -	if (gpio_is_valid(hub->gpio_connect))
> -		gpio_set_value_cansleep(hub->gpio_connect, 1);
> +	if (hub->connect)
> +		gpiod_set_value_cansleep(hub->connect, 1);
>   
>   	hub->mode = USB3503_MODE_HUB;
>   	dev_info(dev, "switched to HUB mode\n");
> @@ -163,13 +162,11 @@ static int usb3503_probe(struct usb3503 *hub)
>   	int err;
>   	u32 mode = USB3503_MODE_HUB;
>   	const u32 *property;
> +	enum gpiod_flags flags;
>   	int len;
>   
>   	if (pdata) {
>   		hub->port_off_mask	= pdata->port_off_mask;
> -		hub->gpio_intn		= pdata->gpio_intn;
> -		hub->gpio_connect	= pdata->gpio_connect;
> -		hub->gpio_reset		= pdata->gpio_reset;
>   		hub->mode		= pdata->initial_mode;
>   	} else if (np) {
>   		u32 rate = 0;
> @@ -230,59 +227,38 @@ static int usb3503_probe(struct usb3503 *hub)
>   			}
>   		}
>   
> -		hub->gpio_intn	= of_get_named_gpio(np, "intn-gpios", 0);
> -		if (hub->gpio_intn == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> -		hub->gpio_connect = of_get_named_gpio(np, "connect-gpios", 0);
> -		if (hub->gpio_connect == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
> -		hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
> -		if (hub->gpio_reset == -EPROBE_DEFER)
> -			return -EPROBE_DEFER;
>   		of_property_read_u32(np, "initial-mode", &mode);
>   		hub->mode = mode;
>   	}
>   
> -	if (hub->port_off_mask && !hub->regmap)
> -		dev_err(dev, "Ports disabled with no control interface\n");
> -
> -	if (gpio_is_valid(hub->gpio_intn)) {
> -		int val = hub->secondary_ref_clk ? GPIOF_OUT_INIT_LOW :
> -						   GPIOF_OUT_INIT_HIGH;
> -		err = devm_gpio_request_one(dev, hub->gpio_intn, val,
> -					    "usb3503 intn");
> -		if (err) {
> -			dev_err(dev,
> -				"unable to request GPIO %d as interrupt pin (%d)\n",
> -				hub->gpio_intn, err);
> -			return err;
> -		}
> -	}
> -
> -	if (gpio_is_valid(hub->gpio_connect)) {
> -		err = devm_gpio_request_one(dev, hub->gpio_connect,
> -				GPIOF_OUT_INIT_LOW, "usb3503 connect");
> -		if (err) {
> -			dev_err(dev,
> -				"unable to request GPIO %d as connect pin (%d)\n",
> -				hub->gpio_connect, err);
> -			return err;
> -		}
> -	}
> -
> -	if (gpio_is_valid(hub->gpio_reset)) {
> -		err = devm_gpio_request_one(dev, hub->gpio_reset,
> -				GPIOF_OUT_INIT_LOW, "usb3503 reset");
> +	if (hub->secondary_ref_clk)
> +		flags = GPIOD_OUT_LOW;
> +	else
> +		flags = GPIOD_OUT_HIGH;
> +	hub->intn = devm_gpiod_get_optional(dev, "intn", flags);
> +	if (IS_ERR(hub->intn))
> +		return PTR_ERR(hub->intn);
> +	if (hub->intn)
> +		gpiod_set_consumer_name(hub->intn, "usb3503 intn");
> +
> +	hub->connect = devm_gpiod_get_optional(dev, "connect", GPIOD_OUT_LOW);
> +	if (IS_ERR(hub->connect))
> +		return PTR_ERR(hub->connect);
> +	if (hub->connect)
> +		gpiod_set_consumer_name(hub->connect, "usb3503 connect");
> +
> +	hub->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(hub->reset))
> +		return PTR_ERR(hub->reset);
> +	if (hub->reset) {
>   		/* Datasheet defines a hardware reset to be at least 100us */
>   		usleep_range(100, 10000);
> -		if (err) {
> -			dev_err(dev,
> -				"unable to request GPIO %d as reset pin (%d)\n",
> -				hub->gpio_reset, err);
> -			return err;
> -		}
> +		gpiod_set_consumer_name(hub->reset, "usb3503 reset");
>   	}
>   
> +	if (hub->port_off_mask && !hub->regmap)
> +		dev_err(dev, "Ports disabled with no control interface\n");
> +
>   	usb3503_switch_mode(hub, hub->mode);
>   
>   	dev_info(dev, "%s: probed in %s mode\n", __func__,
> diff --git a/include/linux/platform_data/usb3503.h b/include/linux/platform_data/usb3503.h
> index e049d51c1353..d01ef97ddf36 100644
> --- a/include/linux/platform_data/usb3503.h
> +++ b/include/linux/platform_data/usb3503.h
> @@ -17,9 +17,6 @@ enum usb3503_mode {
>   struct usb3503_platform_data {
>   	enum usb3503_mode	initial_mode;
>   	u8	port_off_mask;
> -	int	gpio_intn;
> -	int	gpio_connect;
> -	int	gpio_reset;
>   };
>   
>   #endif

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi

On 06.12.2019 08:55, Marek Szyprowski wrote:
> Hi Linus,
>
> On 05.12.2019 15:56, Linus Walleij wrote:
>> This converts the USB3503 to pick GPIO descriptors from the
>> device tree instead of iteratively picking out GPIO number
>> references and then referencing these from the global GPIO
>> numberspace.
>>
>> The USB3503 is only used from device tree among the in-tree
>> platforms. If board files would still desire to use it they can
>> provide machine descriptor tables.
>>
>> Make sure to preserve semantics such as the reset delay
>> introduced by Stefan.
>>
>> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Krzysztof Kozlowski <krzk@kernel.org>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> NAK.
>
> Sorry, but this patch breaks USB3503 HUB operation on Arndale5250 
> board. A brief scan through the code reveals that the whole control 
> logic for the 'intn' gpio is lost.

Well, I've checked further and 'intn' logic is there. The issue with 
Arndale5250 board is something different. Changing the gpio active 
values in arch/arm/boot/dts/exynos5250-arndale.dts from GPIO_ACTIVE_LOW 
to GPIO_ACTIVE_HIGH fixed operation of usb3503 HUB. I really wonder why 
it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...

I'm not sure how to handle this. Old code works also fine with DT flags 
changed to GPIO_ACTIVE_HIGH, which seems to be a proper value for those 
gpio lines.

>> ---
>>   drivers/usb/misc/usb3503.c            | 94 ++++++++++-----------------
>>   include/linux/platform_data/usb3503.h |  3 -
>>   2 files changed, 35 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
>> index 72f39a9751b5..c3c1f65f4196 100644
>> --- a/drivers/usb/misc/usb3503.c
>> +++ b/drivers/usb/misc/usb3503.c
>> @@ -7,11 +7,10 @@
>>     #include <linux/clk.h>
>>   #include <linux/i2c.h>
>> -#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/delay.h>
>>   #include <linux/slab.h>
>>   #include <linux/module.h>
>> -#include <linux/of_gpio.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/platform_data/usb3503.h>
>>   #include <linux/regmap.h>
>> @@ -47,19 +46,19 @@ struct usb3503 {
>>       struct device        *dev;
>>       struct clk        *clk;
>>       u8    port_off_mask;
>> -    int    gpio_intn;
>> -    int    gpio_reset;
>> -    int    gpio_connect;
>> +    struct gpio_desc    *intn;
>> +    struct gpio_desc     *reset;
>> +    struct gpio_desc     *connect;
>>       bool    secondary_ref_clk;
>>   };
>>     static int usb3503_reset(struct usb3503 *hub, int state)
>>   {
>> -    if (!state && gpio_is_valid(hub->gpio_connect))
>> -        gpio_set_value_cansleep(hub->gpio_connect, 0);
>> +    if (!state && hub->connect)
>> +        gpiod_set_value_cansleep(hub->connect, 0);
>>   -    if (gpio_is_valid(hub->gpio_reset))
>> -        gpio_set_value_cansleep(hub->gpio_reset, state);
>> +    if (hub->reset)
>> +        gpiod_set_value_cansleep(hub->reset, state);
>>         /* Wait T_HUBINIT == 4ms for hub logic to stabilize */
>>       if (state)
>> @@ -115,8 +114,8 @@ static int usb3503_connect(struct usb3503 *hub)
>>           }
>>       }
>>   -    if (gpio_is_valid(hub->gpio_connect))
>> -        gpio_set_value_cansleep(hub->gpio_connect, 1);
>> +    if (hub->connect)
>> +        gpiod_set_value_cansleep(hub->connect, 1);
>>         hub->mode = USB3503_MODE_HUB;
>>       dev_info(dev, "switched to HUB mode\n");
>> @@ -163,13 +162,11 @@ static int usb3503_probe(struct usb3503 *hub)
>>       int err;
>>       u32 mode = USB3503_MODE_HUB;
>>       const u32 *property;
>> +    enum gpiod_flags flags;
>>       int len;
>>         if (pdata) {
>>           hub->port_off_mask    = pdata->port_off_mask;
>> -        hub->gpio_intn        = pdata->gpio_intn;
>> -        hub->gpio_connect    = pdata->gpio_connect;
>> -        hub->gpio_reset        = pdata->gpio_reset;
>>           hub->mode        = pdata->initial_mode;
>>       } else if (np) {
>>           u32 rate = 0;
>> @@ -230,59 +227,38 @@ static int usb3503_probe(struct usb3503 *hub)
>>               }
>>           }
>>   -        hub->gpio_intn    = of_get_named_gpio(np, "intn-gpios", 0);
>> -        if (hub->gpio_intn == -EPROBE_DEFER)
>> -            return -EPROBE_DEFER;
>> -        hub->gpio_connect = of_get_named_gpio(np, "connect-gpios", 0);
>> -        if (hub->gpio_connect == -EPROBE_DEFER)
>> -            return -EPROBE_DEFER;
>> -        hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
>> -        if (hub->gpio_reset == -EPROBE_DEFER)
>> -            return -EPROBE_DEFER;
>>           of_property_read_u32(np, "initial-mode", &mode);
>>           hub->mode = mode;
>>       }
>>   -    if (hub->port_off_mask && !hub->regmap)
>> -        dev_err(dev, "Ports disabled with no control interface\n");
>> -
>> -    if (gpio_is_valid(hub->gpio_intn)) {
>> -        int val = hub->secondary_ref_clk ? GPIOF_OUT_INIT_LOW :
>> -                           GPIOF_OUT_INIT_HIGH;
>> -        err = devm_gpio_request_one(dev, hub->gpio_intn, val,
>> -                        "usb3503 intn");
>> -        if (err) {
>> -            dev_err(dev,
>> -                "unable to request GPIO %d as interrupt pin (%d)\n",
>> -                hub->gpio_intn, err);
>> -            return err;
>> -        }
>> -    }
>> -
>> -    if (gpio_is_valid(hub->gpio_connect)) {
>> -        err = devm_gpio_request_one(dev, hub->gpio_connect,
>> -                GPIOF_OUT_INIT_LOW, "usb3503 connect");
>> -        if (err) {
>> -            dev_err(dev,
>> -                "unable to request GPIO %d as connect pin (%d)\n",
>> -                hub->gpio_connect, err);
>> -            return err;
>> -        }
>> -    }
>> -
>> -    if (gpio_is_valid(hub->gpio_reset)) {
>> -        err = devm_gpio_request_one(dev, hub->gpio_reset,
>> -                GPIOF_OUT_INIT_LOW, "usb3503 reset");
>> +    if (hub->secondary_ref_clk)
>> +        flags = GPIOD_OUT_LOW;
>> +    else
>> +        flags = GPIOD_OUT_HIGH;
>> +    hub->intn = devm_gpiod_get_optional(dev, "intn", flags);
>> +    if (IS_ERR(hub->intn))
>> +        return PTR_ERR(hub->intn);
>> +    if (hub->intn)
>> +        gpiod_set_consumer_name(hub->intn, "usb3503 intn");
>> +
>> +    hub->connect = devm_gpiod_get_optional(dev, "connect", 
>> GPIOD_OUT_LOW);
>> +    if (IS_ERR(hub->connect))
>> +        return PTR_ERR(hub->connect);
>> +    if (hub->connect)
>> +        gpiod_set_consumer_name(hub->connect, "usb3503 connect");
>> +
>> +    hub->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +    if (IS_ERR(hub->reset))
>> +        return PTR_ERR(hub->reset);
>> +    if (hub->reset) {
>>           /* Datasheet defines a hardware reset to be at least 100us */
>>           usleep_range(100, 10000);
>> -        if (err) {
>> -            dev_err(dev,
>> -                "unable to request GPIO %d as reset pin (%d)\n",
>> -                hub->gpio_reset, err);
>> -            return err;
>> -        }
>> +        gpiod_set_consumer_name(hub->reset, "usb3503 reset");
>>       }
>>   +    if (hub->port_off_mask && !hub->regmap)
>> +        dev_err(dev, "Ports disabled with no control interface\n");
>> +
>>       usb3503_switch_mode(hub, hub->mode);
>>         dev_info(dev, "%s: probed in %s mode\n", __func__,
>> diff --git a/include/linux/platform_data/usb3503.h 
>> b/include/linux/platform_data/usb3503.h
>> index e049d51c1353..d01ef97ddf36 100644
>> --- a/include/linux/platform_data/usb3503.h
>> +++ b/include/linux/platform_data/usb3503.h
>> @@ -17,9 +17,6 @@ enum usb3503_mode {
>>   struct usb3503_platform_data {
>>       enum usb3503_mode    initial_mode;
>>       u8    port_off_mask;
>> -    int    gpio_intn;
>> -    int    gpio_connect;
>> -    int    gpio_reset;
>>   };
>>     #endif

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 06.12.2019 08:55, Marek Szyprowski wrote:

> > NAK.
> >
> > Sorry, but this patch breaks USB3503 HUB operation on Arndale5250
> > board. A brief scan through the code reveals that the whole control
> > logic for the 'intn' gpio is lost.
>
> Well, I've checked further and 'intn' logic is there. The issue with
> Arndale5250 board is something different. Changing the gpio active
> values in arch/arm/boot/dts/exynos5250-arndale.dts from GPIO_ACTIVE_LOW
> to GPIO_ACTIVE_HIGH fixed operation of usb3503 HUB. I really wonder why
> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...
>
> I'm not sure how to handle this. Old code works also fine with DT flags
> changed to GPIO_ACTIVE_HIGH, which seems to be a proper value for those
> gpio lines.

We should of course fix up the device trees so the polarity in them
is correct.

If the compatibility with elder device trees is mandatory I will make
a quirk into the gpiolib-of.c that enforce active high on this specific
GPIO line. This is pretty straight-forward, I can just use the compatible
of the board and usb3503 in combination to enforce it.

Yours,
Linus Walleij

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 06.12.2019 10:56, Linus Walleij wrote:
> On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 06.12.2019 08:55, Marek Szyprowski wrote:
>>> NAK.
>>>
>>> Sorry, but this patch breaks USB3503 HUB operation on Arndale5250
>>> board. A brief scan through the code reveals that the whole control
>>> logic for the 'intn' gpio is lost.
>> Well, I've checked further and 'intn' logic is there. The issue with
>> Arndale5250 board is something different. Changing the gpio active
>> values in arch/arm/boot/dts/exynos5250-arndale.dts from GPIO_ACTIVE_LOW
>> to GPIO_ACTIVE_HIGH fixed operation of usb3503 HUB. I really wonder why
>> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...
>>
>> I'm not sure how to handle this. Old code works also fine with DT flags
>> changed to GPIO_ACTIVE_HIGH, which seems to be a proper value for those
>> gpio lines.
> We should of course fix up the device trees so the polarity in them
> is correct.

Okay. I've checked the driver and dts:

According to the USB3503 datasheet, reset-gpios should be ACTIVE_LOW 
probably for the all boards. The driver itself should be then fixed to 
set reset line to the opposite values: HIGH (ASSERTED) during probe and 
suspend, and LOW (DE-ASSERTED) during normal operation.

With the above assumptions, the following DTS should be fixed:

arch/arm/boot/dts/exynos4412-odroid-common.dtsi: invert RESET gpio 
polarity (to ACTIVE_LOW)

arch/arm/boot/dts/exynos5250-arndale.dts: invert CONNECT gpio polarity 
(to ACTIVE_HIGH)

arch/arm/boot/dts/exynos5410-odroidxu.dts: invert RESET gpio polarity 
(to ACTIVE_LOW)

arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dts: invert RESET 
gpio polarity (to ACTIVE_LOW), not sure about INTN gpio

arch/arm/boot/dts/sun8i-a83t-cubietruck-plus.dts: invert RESET gpio 
polarity (to ACTIVE_LOW)

I've tested such changes with your patch on Odroid X2, U3, XU and 
Arndale boards - USB3503 worked fine.

I can prepare patchset with the above changes (dts and the driver logic).

> If the compatibility with elder device trees is mandatory I will make
> a quirk into the gpiolib-of.c that enforce active high on this specific
> GPIO line. This is pretty straight-forward, I can just use the compatible
> of the board and usb3503 in combination to enforce it.

Frankly, I don't care about compatibility with old dtbs. It is already 
broken by other changes in the bindings.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Fri, Dec 6, 2019 at 12:43 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dts: invert RESET
> gpio polarity (to ACTIVE_LOW), not sure about INTN gpio

AFAICT INTN should be set to ACTIVE_HIGH if it is working with the
current code in the kernel.

However it is pretty confusing with the "N" at the end of INTN,
indicating negative polarity. Maybe it means something else,
I haven't checked the datasheet. Maybe all boards have inverters
on these lines so they come out active high.

Yours,
Linus Walleij

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 06.12.2019 14:43, Linus Walleij wrote:
> On Fri, Dec 6, 2019 at 12:43 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>
>> arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dts: invert RESET
>> gpio polarity (to ACTIVE_LOW), not sure about INTN gpio
> AFAICT INTN should be set to ACTIVE_HIGH if it is working with the
> current code in the kernel.
>
> However it is pretty confusing with the "N" at the end of INTN,
> indicating negative polarity. Maybe it means something else,
> I haven't checked the datasheet. Maybe all boards have inverters
> on these lines so they come out active high.

Well, this line is indeed active low. The datasheet names it 'int_n'. 
However I think it makes sense to keep it as ACTIVE_HIGH, because the 
'n' is already in the gpio name (and dt binding). In contrary, the reset 
gpio pin/binding is named without the 'n', thus I want to clarify it as 
active low. The datasheet names it 'reset_n'.

If you are okay with this approach, I will send a patchset fixing 
polarity in DTS together with your patch converting the driver to gpio 
descriptors with the fixup for the reset gpio polarity.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Mon, Dec 9, 2019 at 5:33 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 06.12.2019 14:43, Linus Walleij wrote:
> > On Fri, Dec 6, 2019 at 12:43 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >
> >> arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dts: invert RESET
> >> gpio polarity (to ACTIVE_LOW), not sure about INTN gpio
> > AFAICT INTN should be set to ACTIVE_HIGH if it is working with the
> > current code in the kernel.
> >
> > However it is pretty confusing with the "N" at the end of INTN,
> > indicating negative polarity. Maybe it means something else,
> > I haven't checked the datasheet. Maybe all boards have inverters
> > on these lines so they come out active high.
>
> Well, this line is indeed active low. The datasheet names it 'int_n'.
> However I think it makes sense to keep it as ACTIVE_HIGH, because the
> 'n' is already in the gpio name (and dt binding). In contrary, the reset
> gpio pin/binding is named without the 'n', thus I want to clarify it as
> active low. The datasheet names it 'reset_n'.

Agreed.

> If you are okay with this approach, I will send a patchset fixing
> polarity in DTS together with your patch converting the driver to gpio
> descriptors with the fixup for the reset gpio polarity.

Yes this approach should work, will you fold in fixes to my
patch (like asserting reset high) as well or do you want me
to send a v2?

Yours,
Linus Walleij

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 11.12.2019 00:13, Linus Walleij wrote:
> On Mon, Dec 9, 2019 at 5:33 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 06.12.2019 14:43, Linus Walleij wrote:
>>> On Fri, Dec 6, 2019 at 12:43 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>
>>>> arch/arm/boot/dts/qcom-mdm9615-wp8548-mangoh-green.dts: invert RESET
>>>> gpio polarity (to ACTIVE_LOW), not sure about INTN gpio
>>> AFAICT INTN should be set to ACTIVE_HIGH if it is working with the
>>> current code in the kernel.
>>>
>>> However it is pretty confusing with the "N" at the end of INTN,
>>> indicating negative polarity. Maybe it means something else,
>>> I haven't checked the datasheet. Maybe all boards have inverters
>>> on these lines so they come out active high.
>> Well, this line is indeed active low. The datasheet names it 'int_n'.
>> However I think it makes sense to keep it as ACTIVE_HIGH, because the
>> 'n' is already in the gpio name (and dt binding). In contrary, the reset
>> gpio pin/binding is named without the 'n', thus I want to clarify it as
>> active low. The datasheet names it 'reset_n'.
> Agreed.
>
>> If you are okay with this approach, I will send a patchset fixing
>> polarity in DTS together with your patch converting the driver to gpio
>> descriptors with the fixup for the reset gpio polarity.
> Yes this approach should work, will you fold in fixes to my
> patch (like asserting reset high) as well or do you want me
> to send a v2?

I will fold a fixup for your patch.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

BTW:

> I really wonder why
> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...

The old code ignored the polarity flags in the device tree and
assumed everything was active high, that's how. It could as well
be hardcoded to 1337.

Yours,
Linus Walleij

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 06.12.2019 10:58, Linus Walleij wrote:
> On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>
> BTW:
>
>> I really wonder why
>> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...
> The old code ignored the polarity flags in the device tree and
> assumed everything was active high, that's how. It could as well
> be hardcoded to 1337.

Okay, then to restore current driver behavior after your patch, one has 
to change gpio flags in all dts to ACTIVE_HIGH...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Fri, Dec 6, 2019 at 12:53 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 06.12.2019 10:58, Linus Walleij wrote:
> > On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >
> > BTW:
> >
> >> I really wonder why
> >> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...
> > The old code ignored the polarity flags in the device tree and
> > assumed everything was active high, that's how. It could as well
> > be hardcoded to 1337.
>
> Okay, then to restore current driver behavior after your patch, one has
> to change gpio flags in all dts to ACTIVE_HIGH...

Yeah :/

I think we should do a two-stage rocket here, if you make a patch to
all the DTS files I will make sure to add some logic enforcing the
right line levels in this patch as well.

I'll make sure to assert reset expecting it to be flagged as active low.

Yours,
Linus Walleij

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Marek Szyprowski]

Hi Linus,

On 06.12.2019 14:21, Linus Walleij wrote:
> On Fri, Dec 6, 2019 at 12:53 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 06.12.2019 10:58, Linus Walleij wrote:
>>> On Fri, Dec 6, 2019 at 10:14 AM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>
>>> BTW:
>>>
>>>> I really wonder why
>>>> it worked fine with non-descriptor code and the ACTIVE_LOW DT flags...
>>> The old code ignored the polarity flags in the device tree and
>>> assumed everything was active high, that's how. It could as well
>>> be hardcoded to 1337.
>> Okay, then to restore current driver behavior after your patch, one has
>> to change gpio flags in all dts to ACTIVE_HIGH...
> Yeah :/
>
> I think we should do a two-stage rocket here, if you make a patch to
> all the DTS files I will make sure to add some logic enforcing the
> right line levels in this patch as well.
>
> I'll make sure to assert reset expecting it to be flagged as active low.

Frankly, if delay applying this patch one release after the DTS changes 
are applied, no workarounds in gpio core are needed. In such case we 
combine your patch with a driver logic cleanup for the reset gpio, so 
DTS can use ACTIVE_LOW flag then.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors

[Linus Walleij]

On Fri, Dec 6, 2019 at 2:33 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 06.12.2019 14:21, Linus Walleij wrote:

> > I think we should do a two-stage rocket here, if you make a patch to
> > all the DTS files I will make sure to add some logic enforcing the
> > right line levels in this patch as well.
> >
> > I'll make sure to assert reset expecting it to be flagged as active low.
>
> Frankly, if delay applying this patch one release after the DTS changes
> are applied, no workarounds in gpio core are needed. In such case we
> combine your patch with a driver logic cleanup for the reset gpio, so
> DTS can use ACTIVE_LOW flag then.

OK I'm not overly commited to DT ABI stability with old bugs anyway.

Let's do like that, CC me on that patch, I'll wait for it to trickle in
and then postpone resending this patch until a safer point in time.

(I hope the DT ABI-stable-nonsense debate will not happen...)

Yours,
Linus Walleij