| Message ID | 20191205145633.187511-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 51d22e855ea3459d4b272e46aff95de0e59e65a7 |
| Headers | show |
| Series | usb: usb3503: Convert to use GPIO descriptors | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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(-) -- 2.23.0