| Message ID | 1471288892-21702-5-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 08/15/2016 10:21 PM, Hans de Goede wrote: > Together with some musb sunxi glue changes this allows run-time dr_mode > switching support via the "mode" musb sysfs attribute. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c > index fb2d4f3..d369081 100644 > --- a/drivers/phy/phy-sun4i-usb.c > +++ b/drivers/phy/phy-sun4i-usb.c > @@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy) > return 0; > } > > +static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode) > +{ > + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); > + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); > + > + if (phy->index != 0) > + return -EINVAL; > + > + switch (mode) { > + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; > + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; > + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; > + default: > + return -EINVAL; > + } > + > + dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode); > + data->force_session_end = true; > + queue_delayed_work(system_wq, &data->detect, 0); > + > + return 0; > +} > + > void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) > { > struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); [...] $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch ERROR: trailing statements should be on next line #29: FILE: drivers/phy/phy-sun4i-usb.c:439: + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; ERROR: trailing statements should be on next line #30: FILE: drivers/phy/phy-sun4i-usb.c:440: + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; ERROR: trailing statements should be on next line #31: FILE: drivers/phy/phy-sun4i-usb.c:441: + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; total: 3 errors, 0 warnings, 36 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /home/headless/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. MBR, Sergei
Hi, On 08/16/2016 03:48 PM, Sergei Shtylyov wrote: > Hello. > > On 08/15/2016 10:21 PM, Hans de Goede wrote: > >> Together with some musb sunxi glue changes this allows run-time dr_mode >> switching support via the "mode" musb sysfs attribute. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >> index fb2d4f3..d369081 100644 >> --- a/drivers/phy/phy-sun4i-usb.c >> +++ b/drivers/phy/phy-sun4i-usb.c >> @@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy) >> return 0; >> } >> >> +static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode) >> +{ >> + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + >> + if (phy->index != 0) >> + return -EINVAL; >> + >> + switch (mode) { >> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >> + default: >> + return -EINVAL; >> + } >> + >> + dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode); >> + data->force_session_end = true; >> + queue_delayed_work(system_wq, &data->detect, 0); >> + >> + return 0; >> +} >> + >> void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); > [...] > > $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch > ERROR: trailing statements should be on next line > #29: FILE: drivers/phy/phy-sun4i-usb.c:439: > + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; > > ERROR: trailing statements should be on next line > #30: FILE: drivers/phy/phy-sun4i-usb.c:440: > + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; > > ERROR: trailing statements should be on next line > #31: FILE: drivers/phy/phy-sun4i-usb.c:441: > + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; This is normal codeing style for a switch-case assigning a single value per case, but checkpatch does not know this. Regaards, Hans
Hi, Hans de Goede <hdegoede@redhat.com> writes: > Hi, > > On 08/16/2016 03:48 PM, Sergei Shtylyov wrote: >> Hello. >> >> On 08/15/2016 10:21 PM, Hans de Goede wrote: >> >>> Together with some musb sunxi glue changes this allows run-time dr_mode >>> switching support via the "mode" musb sysfs attribute. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >>> index fb2d4f3..d369081 100644 >>> --- a/drivers/phy/phy-sun4i-usb.c >>> +++ b/drivers/phy/phy-sun4i-usb.c >>> @@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy) >>> return 0; >>> } >>> >>> +static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode) >>> +{ >>> + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >>> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >>> + >>> + if (phy->index != 0) >>> + return -EINVAL; >>> + >>> + switch (mode) { >>> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >>> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >>> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode); >>> + data->force_session_end = true; >>> + queue_delayed_work(system_wq, &data->detect, 0); >>> + >>> + return 0; >>> +} >>> + >>> void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) >>> { >>> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> [...] >> >> $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch >> ERROR: trailing statements should be on next line >> #29: FILE: drivers/phy/phy-sun4i-usb.c:439: >> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >> >> ERROR: trailing statements should be on next line >> #30: FILE: drivers/phy/phy-sun4i-usb.c:440: >> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >> >> ERROR: trailing statements should be on next line >> #31: FILE: drivers/phy/phy-sun4i-usb.c:441: >> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; > > This is normal codeing style for a switch-case assigning a single value per case, > but checkpatch does not know this. I don't see that in CodingStyle and it's quite ugly. In fact, CodingStyle states clearly that you shouldn't put multiple statements in one line.
HI, On 18-08-16 09:40, Felipe Balbi wrote: > > Hi, > > Hans de Goede <hdegoede@redhat.com> writes: >> Hi, >> >> On 08/16/2016 03:48 PM, Sergei Shtylyov wrote: >>> Hello. >>> >>> On 08/15/2016 10:21 PM, Hans de Goede wrote: >>> >>>> Together with some musb sunxi glue changes this allows run-time dr_mode >>>> switching support via the "mode" musb sysfs attribute. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >>>> index fb2d4f3..d369081 100644 >>>> --- a/drivers/phy/phy-sun4i-usb.c >>>> +++ b/drivers/phy/phy-sun4i-usb.c >>>> @@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy) >>>> return 0; >>>> } >>>> >>>> +static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode) >>>> +{ >>>> + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >>>> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >>>> + >>>> + if (phy->index != 0) >>>> + return -EINVAL; >>>> + >>>> + switch (mode) { >>>> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >>>> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >>>> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> + >>>> + dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode); >>>> + data->force_session_end = true; >>>> + queue_delayed_work(system_wq, &data->detect, 0); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) >>>> { >>>> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >>> [...] >>> >>> $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch >>> ERROR: trailing statements should be on next line >>> #29: FILE: drivers/phy/phy-sun4i-usb.c:439: >>> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >>> >>> ERROR: trailing statements should be on next line >>> #30: FILE: drivers/phy/phy-sun4i-usb.c:440: >>> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >>> >>> ERROR: trailing statements should be on next line >>> #31: FILE: drivers/phy/phy-sun4i-usb.c:441: >>> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >> >> This is normal codeing style for a switch-case assigning a single value per case, >> but checkpatch does not know this. > > I don't see that in CodingStyle It is an exception to the rule as such it is not listed, but this really is quite a normal thing to do in C code. > and it's quite ugly. So this is ugly: switch (mode) { case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; default: return -EINVAL; } Where as this is not: switch (mode) { case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; default: return -EINVAL; } ??? IMHO the original version is much easier to read / makes it much clearer what the code is doing. But if you insist I can do a v3 changing the coding style to the (IMHO) uglier version. Also note that the real ugliness is that we've 3 different enums for host / device / dual-role. For some reason the musb code has 2 all of its own and then there is "enum phy_mode". Anyways let me know if you want a v3 with check-patch warnings fixed. Regards, Hans
Hi, Hans de Goede <hdegoede@redhat.com> writes: [...] >>>>> void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) >>>>> { >>>>> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >>>> [...] >>>> >>>> $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch >>>> ERROR: trailing statements should be on next line >>>> #29: FILE: drivers/phy/phy-sun4i-usb.c:439: >>>> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >>>> >>>> ERROR: trailing statements should be on next line >>>> #30: FILE: drivers/phy/phy-sun4i-usb.c:440: >>>> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >>>> >>>> ERROR: trailing statements should be on next line >>>> #31: FILE: drivers/phy/phy-sun4i-usb.c:441: >>>> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >>> >>> This is normal codeing style for a switch-case assigning a single value per case, >>> but checkpatch does not know this. >> >> I don't see that in CodingStyle > > It is an exception to the rule as such it is not listed, but this > really is quite a normal thing to do in C code. > >> and it's quite ugly. > > So this is ugly: > > switch (mode) { > case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; > case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; > case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; > default: > return -EINVAL; > } > > Where as this is not: > > switch (mode) { > case PHY_MODE_USB_HOST: > data->dr_mode = USB_DR_MODE_HOST; > break; > case PHY_MODE_USB_DEVICE: > data->dr_mode = USB_DR_MODE_PERIPHERAL; > break; > case PHY_MODE_USB_OTG: > data->dr_mode = USB_DR_MODE_OTG; > break; > default: > return -EINVAL; > } > > ??? > > IMHO the original version is much easier to read / makes it much > clearer what the code is doing. > > But if you insist I can do a v3 changing the coding style to > the (IMHO) uglier version. > > Also note that the real ugliness is that we've 3 different enums > for host / device / dual-role. For some reason the musb code has > 2 all of its own and then there is "enum phy_mode". > > Anyways let me know if you want a v3 with check-patch warnings > fixed. I see it's somewhat common even in drivers/usb: $ git grep -ce "case \w+:.*break;" -- drivers/usb/ drivers/usb/gadget/udc/net2272.c:4 drivers/usb/host/ehci-hcd.c:3 drivers/usb/host/isp116x.h:2 drivers/usb/host/ohci-dbg.c:14 drivers/usb/host/sl811-hcd.c:7 drivers/usb/host/uhci-debug.c:8 drivers/usb/image/microtek.c:64 drivers/usb/mon/mon_text.c:6 drivers/usb/musb/musb_gadget.c:2 drivers/usb/serial/digi_acceleport.c:23 drivers/usb/serial/ftdi_sio.c:10 drivers/usb/serial/mct_u232.c:10 drivers/usb/serial/spcp8x5.c:17 drivers/usb/serial/whiteheat.c:4 drivers/usb/storage/debug.c:86 so I'm okay either way. Kishon has the final say here since he's drivers/phy/ maintainer.
Hi, On Thursday 18 August 2016 03:47 PM, Felipe Balbi wrote: > > Hi, > > Hans de Goede <hdegoede@redhat.com> writes: > > [...] > >>>>>> void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) >>>>>> { >>>>>> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >>>>> [...] >>>>> >>>>> $ scripts/checkpatch.pl ~/patches/phy-sun4i-usb-Add-support-for-phy_set_mode.patch >>>>> ERROR: trailing statements should be on next line >>>>> #29: FILE: drivers/phy/phy-sun4i-usb.c:439: >>>>> + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >>>>> >>>>> ERROR: trailing statements should be on next line >>>>> #30: FILE: drivers/phy/phy-sun4i-usb.c:440: >>>>> + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >>>>> >>>>> ERROR: trailing statements should be on next line >>>>> #31: FILE: drivers/phy/phy-sun4i-usb.c:441: >>>>> + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >>>> >>>> This is normal codeing style for a switch-case assigning a single value per case, >>>> but checkpatch does not know this. >>> >>> I don't see that in CodingStyle >> >> It is an exception to the rule as such it is not listed, but this >> really is quite a normal thing to do in C code. >> >>> and it's quite ugly. >> >> So this is ugly: >> >> switch (mode) { >> case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; >> case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; >> case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; >> default: >> return -EINVAL; >> } >> >> Where as this is not: >> >> switch (mode) { >> case PHY_MODE_USB_HOST: >> data->dr_mode = USB_DR_MODE_HOST; >> break; >> case PHY_MODE_USB_DEVICE: >> data->dr_mode = USB_DR_MODE_PERIPHERAL; >> break; >> case PHY_MODE_USB_OTG: >> data->dr_mode = USB_DR_MODE_OTG; >> break; >> default: >> return -EINVAL; >> } >> >> ??? >> >> IMHO the original version is much easier to read / makes it much >> clearer what the code is doing. >> >> But if you insist I can do a v3 changing the coding style to >> the (IMHO) uglier version. >> >> Also note that the real ugliness is that we've 3 different enums >> for host / device / dual-role. For some reason the musb code has >> 2 all of its own and then there is "enum phy_mode". >> >> Anyways let me know if you want a v3 with check-patch warnings >> fixed. > > I see it's somewhat common even in drivers/usb: > > $ git grep -ce "case \w+:.*break;" -- drivers/usb/ > drivers/usb/gadget/udc/net2272.c:4 > drivers/usb/host/ehci-hcd.c:3 > drivers/usb/host/isp116x.h:2 > drivers/usb/host/ohci-dbg.c:14 > drivers/usb/host/sl811-hcd.c:7 > drivers/usb/host/uhci-debug.c:8 > drivers/usb/image/microtek.c:64 > drivers/usb/mon/mon_text.c:6 > drivers/usb/musb/musb_gadget.c:2 > drivers/usb/serial/digi_acceleport.c:23 > drivers/usb/serial/ftdi_sio.c:10 > drivers/usb/serial/mct_u232.c:10 > drivers/usb/serial/spcp8x5.c:17 > drivers/usb/serial/whiteheat.c:4 > drivers/usb/storage/debug.c:86 > > so I'm okay either way. Kishon has the final say here since he's > drivers/phy/ maintainer. hmm.. I'd prefer without checkpatch errors or warnings. Thanks Kishon
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c index fb2d4f3..d369081 100644 --- a/drivers/phy/phy-sun4i-usb.c +++ b/drivers/phy/phy-sun4i-usb.c @@ -427,6 +427,29 @@ static int sun4i_usb_phy_power_off(struct phy *_phy) return 0; } +static int sun4i_usb_phy_set_mode(struct phy *_phy, enum phy_mode mode) +{ + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); + + if (phy->index != 0) + return -EINVAL; + + switch (mode) { + case PHY_MODE_USB_HOST: data->dr_mode = USB_DR_MODE_HOST; break; + case PHY_MODE_USB_DEVICE: data->dr_mode = USB_DR_MODE_PERIPHERAL; break; + case PHY_MODE_USB_OTG: data->dr_mode = USB_DR_MODE_OTG; break; + default: + return -EINVAL; + } + + dev_info(&_phy->dev, "Changing dr_mode to %d\n", (int)data->dr_mode); + data->force_session_end = true; + queue_delayed_work(system_wq, &data->detect, 0); + + return 0; +} + void sun4i_usb_phy_set_squelch_detect(struct phy *_phy, bool enabled) { struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); @@ -440,6 +463,7 @@ static const struct phy_ops sun4i_usb_phy_ops = { .exit = sun4i_usb_phy_exit, .power_on = sun4i_usb_phy_power_on, .power_off = sun4i_usb_phy_power_off, + .set_mode = sun4i_usb_phy_set_mode, .owner = THIS_MODULE, };
Together with some musb sunxi glue changes this allows run-time dr_mode switching support via the "mode" musb sysfs attribute. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/phy/phy-sun4i-usb.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)