| Message ID | 1534859756-6955-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| 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. -- With Best Regards, Andy Shevchenko
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 >
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(+) -- 2.7.4