Hi,
On Thursday 18 August 2016 03:47 PM, Felipe Balbi wrote:
>
> 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.
hmm.. I'd prefer without checkpatch errors or warnings.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
