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@xxxxxxxxxxxx>
>> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Cc: Stefan Agner <stefan@xxxxxxxx>
>> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> 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
