Hi,
Hans de Goede <hdegoede@xxxxxxxxxx> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
