[PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode

[kishon@ti.com (Kishon Vijay Abraham I)]

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