| Message ID | 1534859756-6955-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | [1/6] usb: chipidea: Add dynamic pinctrl selection | expand |
On Tue, Aug 21, 2018 at 4:57 PM Loic Poulain <loic.poulain@linaro.org> wrote: > > Some hardware implementations require to configure pins differently > according to the USB role (host/device), this can be an update of the > pins routing or a simple GPIO value change. > > This patch introduces new optional "host" and "device" pinctrls. > If these pinctrls are defined by the device, they are respectively > selected on host/device role start. > > If a default pinctrl exist, it is restored on host/device role stop. > #include <linux/extcon.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/pinctrl/consumer.h> A nit: Even in this context it would be nice to keep *some* order. > #include <linux/module.h> > #include <linux/idr.h> > #include <linux/interrupt.h> > + p = pinctrl_lookup_state(platdata->pctl, "default"); > + p = pinctrl_lookup_state(platdata->pctl, "host"); > + p = pinctrl_lookup_state(platdata->pctl, "device"); I'm wondering if we have any rules applied to these names. > #include <linux/usb/hcd.h> > #include <linux/usb/chipidea.h> > #include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> Ditto about ordering. > + if (ci->platdata->pins_host) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_host); Hmm... Do we need to have a condition above? > + if (ci->platdata->pins_host && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); Ditto about conditional. > #include <linux/usb/gadget.h> > #include <linux/usb/otg-fsm.h> > #include <linux/usb/chipidea.h> > +#include <linux/pinctrl/consumer.h> Ditto about ordering. > + if (ci->platdata->pins_device) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_device); > + if (ci->platdata->pins_device && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); Ditto about conditional.
On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote: > Some hardware implementations require to configure pins differently > according to the USB role (host/device), this can be an update of the > pins routing or a simple GPIO value change. > > This patch introduces new optional "host" and "device" pinctrls. > If these pinctrls are defined by the device, they are respectively > selected on host/device role start. > > If a default pinctrl exist, it is restored on host/device role stop. > The implementation looks reasonable, but we're actually just toggling a gpio using pinctrl states. Do you see any reason not to control this mux through the gpio api? Regards, Bjorn > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/usb/chipidea/core.c | 19 +++++++++++++++++++ > drivers/usb/chipidea/host.c | 9 +++++++++ > drivers/usb/chipidea/udc.c | 9 +++++++++ > include/linux/usb/chipidea.h | 6 ++++++ > 4 files changed, 43 insertions(+) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 85fc6db..03e52fc 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -46,6 +46,7 @@ > #include <linux/extcon.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/module.h> > #include <linux/idr.h> > #include <linux/interrupt.h> > @@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev, > else > cable->connected = false; > } > + > + platdata->pctl = devm_pinctrl_get(dev); > + if (!IS_ERR(platdata->pctl)) { > + struct pinctrl_state *p; > + > + p = pinctrl_lookup_state(platdata->pctl, "default"); > + if (!IS_ERR(p)) > + platdata->pins_default = p; > + > + p = pinctrl_lookup_state(platdata->pctl, "host"); > + if (!IS_ERR(p)) > + platdata->pins_host = p; > + > + p = pinctrl_lookup_state(platdata->pctl, "device"); > + if (!IS_ERR(p)) > + platdata->pins_device = p; > + } > + > return 0; > } > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index af45aa32..55dbd49 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -13,6 +13,7 @@ > #include <linux/usb/hcd.h> > #include <linux/usb/chipidea.h> > #include <linux/regulator/consumer.h> > +#include <linux/pinctrl/consumer.h> > > #include "../host/ehci.h" > > @@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci) > } > } > > + if (ci->platdata->pins_host) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_host); > + > ret = usb_add_hcd(hcd, 0, 0); > if (ret) { > goto disable_reg; > @@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci) > } > ci->hcd = NULL; > ci->otg.host = NULL; > + > + if (ci->platdata->pins_host && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); > } > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 9852ec5..c04384e 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -19,6 +19,7 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/otg-fsm.h> > #include <linux/usb/chipidea.h> > +#include <linux/pinctrl/consumer.h> > > #include "ci.h" > #include "udc.h" > @@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > { > + if (ci->platdata->pins_device) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_device); > + > if (ci->is_otg) > /* Clear and enable BSV irq */ > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > @@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci) > hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > > ci->vbus_active = 0; > + > + if (ci->platdata->pins_device && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); > } > > /** > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > index 07f9936..63758c3 100644 > --- a/include/linux/usb/chipidea.h > +++ b/include/linux/usb/chipidea.h > @@ -77,6 +77,12 @@ struct ci_hdrc_platform_data { > struct ci_hdrc_cable vbus_extcon; > struct ci_hdrc_cable id_extcon; > u32 phy_clkgate_delay_us; > + > + /* pins */ > + struct pinctrl *pctl; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_host; > + struct pinctrl_state *pins_device; > }; > > /* Default offset of capability registers */ > -- > 2.7.4 >
Hi Bjorn, On 23 August 2018 at 16:53, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote: > >> Some hardware implementations require to configure pins differently >> according to the USB role (host/device), this can be an update of the >> pins routing or a simple GPIO value change. >> >> This patch introduces new optional "host" and "device" pinctrls. >> If these pinctrls are defined by the device, they are respectively >> selected on host/device role start. >> >> If a default pinctrl exist, it is restored on host/device role stop. >> > > The implementation looks reasonable, but we're actually just toggling a > gpio using pinctrl states. Do you see any reason not to control this mux > through the gpio api? > Actually, two gpios (including hub reset one), but you're right. Point is that these gpios are very specific to the Dragonboard layout and not related to USB controller itself (external mux), so I'm not sure it makes sense to control a 'mux' GPIO from the chipidea driver. That's why I used pinctrl which is more generic and hides the underlying operations (a simple GPIO toggling in our case). But let me know if there is a better way. Regards, Loic
On 23 August 2018 at 12:11, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Aug 21, 2018 at 4:57 PM Loic Poulain <loic.poulain@linaro.org> wrote: >> >> Some hardware implementations require to configure pins differently >> according to the USB role (host/device), this can be an update of the >> pins routing or a simple GPIO value change. >> >> This patch introduces new optional "host" and "device" pinctrls. >> If these pinctrls are defined by the device, they are respectively >> selected on host/device role start. >> >> If a default pinctrl exist, it is restored on host/device role stop. > >> #include <linux/extcon.h> >> #include <linux/phy/phy.h> >> #include <linux/platform_device.h> >> +#include <linux/pinctrl/consumer.h> > > A nit: Even in this context it would be nice to keep *some* order. > Ok. >> #include <linux/module.h> >> #include <linux/idr.h> >> #include <linux/interrupt.h> > >> + p = pinctrl_lookup_state(platdata->pctl, "default"); >> + p = pinctrl_lookup_state(platdata->pctl, "host"); >> + p = pinctrl_lookup_state(platdata->pctl, "device"); > > I'm wondering if we have any rules applied to these names. I suppose i have document this in the chipidea DT binding doc. > >> #include <linux/usb/hcd.h> >> #include <linux/usb/chipidea.h> >> #include <linux/regulator/consumer.h> >> +#include <linux/pinctrl/consumer.h> > > Ditto about ordering. > >> + if (ci->platdata->pins_host) >> + pinctrl_select_state(ci->platdata->pctl, >> + ci->platdata->pins_host); > > Hmm... Do we need to have a condition above? > Yes, since these pinctrls are optional and can be null. >> + if (ci->platdata->pins_host && ci->platdata->pins_default) >> + pinctrl_select_state(ci->platdata->pctl, >> + ci->platdata->pins_default); > > Ditto about conditional. > >> #include <linux/usb/gadget.h> >> #include <linux/usb/otg-fsm.h> >> #include <linux/usb/chipidea.h> >> +#include <linux/pinctrl/consumer.h> > > Ditto about ordering. > >> + if (ci->platdata->pins_device) >> + pinctrl_select_state(ci->platdata->pctl, >> + ci->platdata->pins_device); > > >> + if (ci->platdata->pins_device && ci->platdata->pins_default) >> + pinctrl_select_state(ci->platdata->pctl, >> + ci->platdata->pins_default); > > Ditto about conditional. > > -- > With Best Regards, > Andy Shevchenko Regards, Loic
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 85fc6db..03e52fc 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -46,6 +46,7 @@ #include <linux/extcon.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> +#include <linux/pinctrl/consumer.h> #include <linux/module.h> #include <linux/idr.h> #include <linux/interrupt.h> @@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev, else cable->connected = false; } + + platdata->pctl = devm_pinctrl_get(dev); + if (!IS_ERR(platdata->pctl)) { + struct pinctrl_state *p; + + p = pinctrl_lookup_state(platdata->pctl, "default"); + if (!IS_ERR(p)) + platdata->pins_default = p; + + p = pinctrl_lookup_state(platdata->pctl, "host"); + if (!IS_ERR(p)) + platdata->pins_host = p; + + p = pinctrl_lookup_state(platdata->pctl, "device"); + if (!IS_ERR(p)) + platdata->pins_device = p; + } + return 0; } diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index af45aa32..55dbd49 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -13,6 +13,7 @@ #include <linux/usb/hcd.h> #include <linux/usb/chipidea.h> #include <linux/regulator/consumer.h> +#include <linux/pinctrl/consumer.h> #include "../host/ehci.h" @@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci) } } + if (ci->platdata->pins_host) + pinctrl_select_state(ci->platdata->pctl, + ci->platdata->pins_host); + ret = usb_add_hcd(hcd, 0, 0); if (ret) { goto disable_reg; @@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci) } ci->hcd = NULL; ci->otg.host = NULL; + + if (ci->platdata->pins_host && ci->platdata->pins_default) + pinctrl_select_state(ci->platdata->pctl, + ci->platdata->pins_default); } diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 9852ec5..c04384e 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -19,6 +19,7 @@ #include <linux/usb/gadget.h> #include <linux/usb/otg-fsm.h> #include <linux/usb/chipidea.h> +#include <linux/pinctrl/consumer.h> #include "ci.h" #include "udc.h" @@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) static int udc_id_switch_for_device(struct ci_hdrc *ci) { + if (ci->platdata->pins_device) + pinctrl_select_state(ci->platdata->pctl, + ci->platdata->pins_device); + if (ci->is_otg) /* Clear and enable BSV irq */ hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, @@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci) hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); ci->vbus_active = 0; + + if (ci->platdata->pins_device && ci->platdata->pins_default) + pinctrl_select_state(ci->platdata->pctl, + ci->platdata->pins_default); } /** diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 07f9936..63758c3 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -77,6 +77,12 @@ struct ci_hdrc_platform_data { struct ci_hdrc_cable vbus_extcon; struct ci_hdrc_cable id_extcon; u32 phy_clkgate_delay_us; + + /* pins */ + struct pinctrl *pctl; + struct pinctrl_state *pins_default; + struct pinctrl_state *pins_host; + struct pinctrl_state *pins_device; }; /* Default offset of capability registers */
Some hardware implementations require to configure pins differently according to the USB role (host/device), this can be an update of the pins routing or a simple GPIO value change. This patch introduces new optional "host" and "device" pinctrls. If these pinctrls are defined by the device, they are respectively selected on host/device role start. If a default pinctrl exist, it is restored on host/device role stop. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/usb/chipidea/core.c | 19 +++++++++++++++++++ drivers/usb/chipidea/host.c | 9 +++++++++ drivers/usb/chipidea/udc.c | 9 +++++++++ include/linux/usb/chipidea.h | 6 ++++++ 4 files changed, 43 insertions(+)