| Message ID | 20191205142200.145252-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | musb: davinci: Convert to use GPIO descriptor | expand |
Hi Linus, On Thu, Dec 05, 2019 at 03:22:00PM +0100, Linus Walleij wrote: > The DaVinci MUSB glue contains an optional GPIO line to > control VBUS power, convert this to use a GPIO descriptor > and augment the EVM board file to provide this descriptor. > > I can't get this driver to compile properly and it depends > on broken but when I didn get it to compile brokenly, it > did at least not complain about THIS code being broken so > I don't think I broke the driver any more than what it > already is. > > I did away with the ifdefs that do not work with > multiplatform anyway so the day someone decides to > resurrect the code, the path to get it working should be > easier as well since DaVinci is now multiplatform. Generally I don't take such patches unless they are part of the effort to un-broken the driver. But is this patch a prerequisite of any cleanup in DaVinci family or ARM architecture? What is the motivation of this patch? Sekhar/Tony, This DaVinci glue driver has been marked as BROKEN since 2013 and seems no one complained about it. Is there any development on the DaVinci devices still active? Does it make sense to remove this driver out of the kernel instead? -Bin.
On Thu, Dec 5, 2019 at 9:20 PM Bin Liu <b-liu@ti.com> wrote: > On Thu, Dec 05, 2019 at 03:22:00PM +0100, Linus Walleij wrote: > > The DaVinci MUSB glue contains an optional GPIO line to > > control VBUS power, convert this to use a GPIO descriptor > > and augment the EVM board file to provide this descriptor. > > > > I can't get this driver to compile properly and it depends > > on broken but when I didn get it to compile brokenly, it > > did at least not complain about THIS code being broken so > > I don't think I broke the driver any more than what it > > already is. > > > > I did away with the ifdefs that do not work with > > multiplatform anyway so the day someone decides to > > resurrect the code, the path to get it working should be > > easier as well since DaVinci is now multiplatform. > > Generally I don't take such patches unless they are part of the effort > to un-broken the driver. But is this patch a prerequisite of any cleanup > in DaVinci family or ARM architecture? What is the motivation of this > patch? The motivation is the clean-up of the internal GPIO API for the kernel. It is described in drivers/gpio/TODO I can propose a patch simply deleting the code if you prefer that, then whoever want to resurrect it can still get it out of git if they need. As long as nothing includes <linux/gpio.h> anymore, I am happy. Yours, Linus Walleij
pon., 9 gru 2019 o 01:25 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > On Thu, Dec 5, 2019 at 9:20 PM Bin Liu <b-liu@ti.com> wrote: > > On Thu, Dec 05, 2019 at 03:22:00PM +0100, Linus Walleij wrote: > > > The DaVinci MUSB glue contains an optional GPIO line to > > > control VBUS power, convert this to use a GPIO descriptor > > > and augment the EVM board file to provide this descriptor. > > > > > > I can't get this driver to compile properly and it depends > > > on broken but when I didn get it to compile brokenly, it > > > did at least not complain about THIS code being broken so > > > I don't think I broke the driver any more than what it > > > already is. > > > > > > I did away with the ifdefs that do not work with > > > multiplatform anyway so the day someone decides to > > > resurrect the code, the path to get it working should be > > > easier as well since DaVinci is now multiplatform. > > > > Generally I don't take such patches unless they are part of the effort > > to un-broken the driver. But is this patch a prerequisite of any cleanup > > in DaVinci family or ARM architecture? What is the motivation of this > > patch? > > The motivation is the clean-up of the internal GPIO API for the > kernel. It is described in drivers/gpio/TODO > > I can propose a patch simply deleting the code if you prefer that, > then whoever want to resurrect it can still get it out of git if > they need. As long as nothing includes <linux/gpio.h> anymore, > I am happy. > > Yours, > Linus Walleij I have a related WiP series that removes calls to legacy GPIO API from the dm355-evm which uses the same driver. Hopefully this week I'll have some time to take a look at it, especially since I've got the relevant HW now. Who knows, maybe I'll even resurrect this code. :) In other words: please hold off removing of this driver. Best regards, Bartosz Golaszewski
On Mon, Dec 09, 2019 at 11:41:29AM +0100, Bartosz Golaszewski wrote: > pon., 9 gru 2019 o 01:25 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > On Thu, Dec 5, 2019 at 9:20 PM Bin Liu <b-liu@ti.com> wrote: > > > On Thu, Dec 05, 2019 at 03:22:00PM +0100, Linus Walleij wrote: > > > > The DaVinci MUSB glue contains an optional GPIO line to > > > > control VBUS power, convert this to use a GPIO descriptor > > > > and augment the EVM board file to provide this descriptor. > > > > > > > > I can't get this driver to compile properly and it depends > > > > on broken but when I didn get it to compile brokenly, it > > > > did at least not complain about THIS code being broken so > > > > I don't think I broke the driver any more than what it > > > > already is. > > > > > > > > I did away with the ifdefs that do not work with > > > > multiplatform anyway so the day someone decides to > > > > resurrect the code, the path to get it working should be > > > > easier as well since DaVinci is now multiplatform. > > > > > > Generally I don't take such patches unless they are part of the effort > > > to un-broken the driver. But is this patch a prerequisite of any cleanup > > > in DaVinci family or ARM architecture? What is the motivation of this > > > patch? > > > > The motivation is the clean-up of the internal GPIO API for the > > kernel. It is described in drivers/gpio/TODO > > > > I can propose a patch simply deleting the code if you prefer that, > > then whoever want to resurrect it can still get it out of git if > > they need. As long as nothing includes <linux/gpio.h> anymore, > > I am happy. > > > > Yours, > > Linus Walleij > > I have a related WiP series that removes calls to legacy GPIO API from > the dm355-evm which uses the same driver. Hopefully this week I'll > have some time to take a look at it, especially since I've got the > relevant HW now. Who knows, maybe I'll even resurrect this code. :) > > In other words: please hold off removing of this driver. Sounds good. I will review and consider taking this patch then. Thanks. -Bin.
On Mon, Dec 9, 2019 at 11:41 AM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > pon., 9 gru 2019 o 01:25 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > On Thu, Dec 5, 2019 at 9:20 PM Bin Liu <b-liu@ti.com> wrote: > > > On Thu, Dec 05, 2019 at 03:22:00PM +0100, Linus Walleij wrote: > > > > The DaVinci MUSB glue contains an optional GPIO line to > > > > control VBUS power, convert this to use a GPIO descriptor > > > > and augment the EVM board file to provide this descriptor. > > > > > > > > I can't get this driver to compile properly and it depends > > > > on broken but when I didn get it to compile brokenly, it > > > > did at least not complain about THIS code being broken so > > > > I don't think I broke the driver any more than what it > > > > already is. > > > > > > > > I did away with the ifdefs that do not work with > > > > multiplatform anyway so the day someone decides to > > > > resurrect the code, the path to get it working should be > > > > easier as well since DaVinci is now multiplatform. > > > > > > Generally I don't take such patches unless they are part of the effort > > > to un-broken the driver. But is this patch a prerequisite of any cleanup > > > in DaVinci family or ARM architecture? What is the motivation of this > > > patch? > > > > The motivation is the clean-up of the internal GPIO API for the > > kernel. It is described in drivers/gpio/TODO > > > > I can propose a patch simply deleting the code if you prefer that, > > then whoever want to resurrect it can still get it out of git if > > they need. As long as nothing includes <linux/gpio.h> anymore, > > I am happy. > > > > Yours, > > Linus Walleij > > I have a related WiP series that removes calls to legacy GPIO API from > the dm355-evm which uses the same driver. Hopefully this week I'll > have some time to take a look at it, especially since I've got the > relevant HW now. Who knows, maybe I'll even resurrect this code. :) Excellent Bartosz, thanks for looking into this. Yours, Linus Walleij
diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c index 9d87d4e440ea..040c949414fa 100644 --- a/arch/arm/mach-davinci/board-dm644x-evm.c +++ b/arch/arm/mach-davinci/board-dm644x-evm.c @@ -823,6 +823,17 @@ static int davinci_phy_fixup(struct phy_device *phydev) #define HAS_NAND IS_ENABLED(CONFIG_MTD_NAND_DAVINCI) +#define GPIO_nVBUS_DRV 160 + +static struct gpiod_lookup_table dm644evm_usb_gpio_table = { + .dev_id = "musb-davinci", + .table = { + GPIO_LOOKUP("davinci_gpio", GPIO_nVBUS_DRV, NULL, + GPIO_ACTIVE_HIGH), + { } + }, +}; + static __init void davinci_evm_init(void) { int ret; @@ -875,6 +886,7 @@ static __init void davinci_evm_init(void) dm644x_init_asp(); /* irlml6401 switches over 1A, in under 8 msec */ + gpiod_add_lookup_table(&dm644evm_usb_gpio_table); davinci_setup_usb(1000, 8); if (IS_BUILTIN(CONFIG_PHYLIB)) { diff --git a/drivers/usb/musb/davinci.c b/drivers/usb/musb/davinci.c index fb6bbd254ab7..546ffaac6ebf 100644 --- a/drivers/usb/musb/davinci.c +++ b/drivers/usb/musb/davinci.c @@ -13,7 +13,7 @@ #include <linux/clk.h> #include <linux/err.h> #include <linux/io.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/platform_device.h> #include <linux/dma-mapping.h> #include <linux/usb/usb_phy_generic.h> @@ -25,10 +25,6 @@ #include "musb_core.h" -#ifdef CONFIG_MACH_DAVINCI_EVM -#define GPIO_nVBUS_DRV 160 -#endif - #include "davinci.h" #include "cppi_dma.h" @@ -40,6 +36,9 @@ struct davinci_glue { struct device *dev; struct platform_device *musb; struct clk *clk; + bool vbus_state; + struct gpio_desc *vbus; + struct work_struct vbus_work; }; /* REVISIT (PM) we should be able to keep the PHY in low power mode most @@ -135,43 +134,44 @@ static void davinci_musb_disable(struct musb *musb) * when J10 is out, and TI documents it as handling OTG. */ -#ifdef CONFIG_MACH_DAVINCI_EVM - -static int vbus_state = -1; - /* I2C operations are always synchronous, and require a task context. * With unloaded systems, using the shared workqueue seems to suffice * to satisfy the 100msec A_WAIT_VRISE timeout... */ -static void evm_deferred_drvvbus(struct work_struct *ignored) +static void evm_deferred_drvvbus(struct work_struct *work) { - gpio_set_value_cansleep(GPIO_nVBUS_DRV, vbus_state); - vbus_state = !vbus_state; -} + struct davinci_glue *glue = container_of(work, struct davinci_glue, + vbus_work); -#endif /* EVM */ + gpiod_set_value_cansleep(glue->vbus, glue->vbus_state); + glue->vbus_state = !glue->vbus_state; +} -static void davinci_musb_source_power(struct musb *musb, int is_on, int immediate) +static void davinci_musb_source_power(struct musb *musb, int is_on, + int immediate) { -#ifdef CONFIG_MACH_DAVINCI_EVM + struct davinci_glue *glue = dev_get_drvdata(musb->controller->parent); + + /* This GPIO handling is entirely optional */ + if (!glue->vbus) + return; + if (is_on) is_on = 1; if (vbus_state == is_on) return; - vbus_state = !is_on; /* 0/1 vs "-1 == unknown/init" */ + /* 0/1 vs "-1 == unknown/init" */ + glue->vbus_state = !is_on; if (machine_is_davinci_evm()) { - static DECLARE_WORK(evm_vbus_work, evm_deferred_drvvbus); - if (immediate) - gpio_set_value_cansleep(GPIO_nVBUS_DRV, vbus_state); + gpiod_set_value_cansleep(glue->vbus, glue->vbus_state); else - schedule_work(&evm_vbus_work); + schedule_work(&glue->vbus_work); } if (immediate) - vbus_state = is_on; -#endif + glue->vbus_state = is_on; } static void davinci_musb_set_vbus(struct musb *musb, int is_on) @@ -524,6 +524,15 @@ static int davinci_probe(struct platform_device *pdev) pdata->platform_ops = &davinci_ops; + glue->vbus = devm_gpiod_get_optional(&pdev->dev, NULL, GPIOD_OUT_LOW); + if (IS_ERR(glue->vbus)) { + ret = PTR_ERR(glue->vbus); + goto err0; + } else { + glue->vbus_state = -1; + INIT_WORK(&glue->vbus_work, evm_deferred_drvvbus); + } + usb_phy_generic_register(); platform_set_drvdata(pdev, glue);
The DaVinci MUSB glue contains an optional GPIO line to control VBUS power, convert this to use a GPIO descriptor and augment the EVM board file to provide this descriptor. I can't get this driver to compile properly and it depends on broken but when I didn get it to compile brokenly, it did at least not complain about THIS code being broken so I don't think I broke the driver any more than what it already is. I did away with the ifdefs that do not work with multiplatform anyway so the day someone decides to resurrect the code, the path to get it working should be easier as well since DaVinci is now multiplatform. Cc: Sekhar Nori <nsekhar@ti.com> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- arch/arm/mach-davinci/board-dm644x-evm.c | 12 ++++++ drivers/usb/musb/davinci.c | 55 ++++++++++++++---------- 2 files changed, 44 insertions(+), 23 deletions(-)