| Message ID | 20191211145226.25074-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] ARM: dts: exynos: Correct USB3503 GPIOs polarity | expand |
On Wed, 2019-12-11 at 15:52 +0100, Marek Szyprowski wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > 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> > [mszyprow: invert the logic behind reset GPIO line] > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > 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..116bd789e568 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); What about preparing another patch for @state before this path? > > /* 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_HIGH); > + 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
Hi Chunfeng, On 12.12.2019 03:10, Chunfeng Yun wrote: > On Wed, 2019-12-11 at 15:52 +0100, Marek Szyprowski wrote: >> From: Linus Walleij <linus.walleij@linaro.org> >> >> 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> >> [mszyprow: invert the logic behind reset GPIO line] >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> 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..116bd789e568 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); > What about preparing another patch for @state before this path? In such case the driver will be broken after such patch until a conversion to descriptor based GPIO api is done. ... Best regards
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c index 72f39a9751b5..116bd789e568 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_HIGH); + 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