On 23 August 2018 at 12:11, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Tue, Aug 21, 2018 at 4:57 PM Loic Poulain <loic.poulain@xxxxxxxxxx> 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
