* [PATCH] usb: usb3503: Convert to use GPIO descriptors
@ 2019-12-05 14:56 ` Linus Walleij
2019-12-06 7:55 ` Marek Szyprowski
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-05 14:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, Linus Walleij, Chunfeng Yun, Marek Szyprowski,
Stefan Agner, Krzysztof Kozlowski
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-05 14:56 ` [PATCH] usb: usb3503: Convert to use GPIO descriptors Linus Walleij
@ 2019-12-06 7:55 ` Marek Szyprowski
2019-12-06 9:14 ` Marek Szyprowski
0 siblings, 1 reply; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-06 7:55 UTC (permalink / raw)
To: Linus Walleij, Greg Kroah-Hartman
Cc: linux-usb, Chunfeng Yun, Stefan Agner, Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 7:55 ` Marek Szyprowski
@ 2019-12-06 9:14 ` Marek Szyprowski
2019-12-06 9:56 ` Linus Walleij
2019-12-06 9:58 ` Linus Walleij
0 siblings, 2 replies; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-06 9:14 UTC (permalink / raw)
To: Linus Walleij, Greg Kroah-Hartman
Cc: linux-usb, Chunfeng Yun, Stefan Agner, Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 9:14 ` Marek Szyprowski
@ 2019-12-06 9:56 ` Linus Walleij
2019-12-06 11:42 ` Marek Szyprowski
2019-12-06 9:58 ` Linus Walleij
1 sibling, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-06 9:56 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 9:14 ` Marek Szyprowski
2019-12-06 9:56 ` Linus Walleij
@ 2019-12-06 9:58 ` Linus Walleij
2019-12-06 11:52 ` Marek Szyprowski
1 sibling, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-06 9:58 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 9:56 ` Linus Walleij
@ 2019-12-06 11:42 ` Marek Szyprowski
2019-12-06 13:43 ` Linus Walleij
0 siblings, 1 reply; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-06 11:42 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 9:58 ` Linus Walleij
@ 2019-12-06 11:52 ` Marek Szyprowski
2019-12-06 13:21 ` Linus Walleij
0 siblings, 1 reply; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-06 11:52 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 11:52 ` Marek Szyprowski
@ 2019-12-06 13:21 ` Linus Walleij
2019-12-06 13:33 ` Marek Szyprowski
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-06 13:21 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 13:21 ` Linus Walleij
@ 2019-12-06 13:33 ` Marek Szyprowski
2019-12-06 13:38 ` Linus Walleij
0 siblings, 1 reply; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-06 13:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 13:33 ` Marek Szyprowski
@ 2019-12-06 13:38 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2019-12-06 13:38 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 11:42 ` Marek Szyprowski
@ 2019-12-06 13:43 ` Linus Walleij
2019-12-09 16:33 ` Marek Szyprowski
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-06 13:43 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-06 13:43 ` Linus Walleij
@ 2019-12-09 16:33 ` Marek Szyprowski
2019-12-10 23:13 ` Linus Walleij
0 siblings, 1 reply; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-09 16:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-09 16:33 ` Marek Szyprowski
@ 2019-12-10 23:13 ` Linus Walleij
2019-12-11 8:48 ` Marek Szyprowski
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2019-12-10 23:13 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: usb3503: Convert to use GPIO descriptors
2019-12-10 23:13 ` Linus Walleij
@ 2019-12-11 8:48 ` Marek Szyprowski
0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2019-12-11 8:48 UTC (permalink / raw)
To: Linus Walleij
Cc: Greg Kroah-Hartman, linux-usb, Chunfeng Yun, Stefan Agner,
Krzysztof Kozlowski
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-12-11 8:48 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20191205145641eucas1p1e3f40dff8a0c8e9ca47425e2370eabbb@eucas1p1.samsung.com>
2019-12-05 14:56 ` [PATCH] usb: usb3503: Convert to use GPIO descriptors Linus Walleij
2019-12-06 7:55 ` Marek Szyprowski
2019-12-06 9:14 ` Marek Szyprowski
2019-12-06 9:56 ` Linus Walleij
2019-12-06 11:42 ` Marek Szyprowski
2019-12-06 13:43 ` Linus Walleij
2019-12-09 16:33 ` Marek Szyprowski
2019-12-10 23:13 ` Linus Walleij
2019-12-11 8:48 ` Marek Szyprowski
2019-12-06 9:58 ` Linus Walleij
2019-12-06 11:52 ` Marek Szyprowski
2019-12-06 13:21 ` Linus Walleij
2019-12-06 13:33 ` Marek Szyprowski
2019-12-06 13:38 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).