From: kishon@ti•com (Kishon Vijay Abraham I)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode
Date: Fri, 19 Aug 2016 18:57:05 +0530 [thread overview]
Message-ID: <57B70929.5080707@ti.com> (raw)
In-Reply-To: <871t1m5qjs.fsf@linux.intel.com>
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
next prev parent reply other threads:[~2016-08-19 13:27 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-15 19:21 [PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs Hans de Goede
2016-08-15 19:21 ` [PATCH v2 1/7] phy-sun4i-usb: Use bool where appropriate Hans de Goede
2016-08-15 19:21 ` [PATCH v2 2/7] phy-sun4i-usb: Refactor forced session ending Hans de Goede
2016-08-15 19:21 ` [PATCH v2 3/7] phy-sun4i-usb: Simplify missing dr_mode handling Hans de Goede
2016-08-15 19:21 ` [PATCH v2 4/7] phy-sun4i-usb: Add support for phy_set_mode Hans de Goede
2016-08-16 13:48 ` Sergei Shtylyov
2016-08-16 20:01 ` Hans de Goede
2016-08-18 7:40 ` Felipe Balbi
2016-08-18 9:05 ` Hans de Goede
2016-08-18 10:17 ` Felipe Balbi
2016-08-19 13:27 ` Kishon Vijay Abraham I [this message]
2016-08-15 19:21 ` [PATCH v2 5/7] phy-sun4i-usb: Warn when external vbus is detected Hans de Goede
2016-08-15 19:21 ` [PATCH v2 6/7] phy-sun4i-usb: Add "allwinner, usb0-usb-a-connector" dt property Hans de Goede
2016-08-19 21:33 ` [PATCH v2 6/7] phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" " Bin Liu
2016-08-15 19:21 ` [PATCH v2 7/7] musb: sunxi: Add support for platform_set_mode Hans de Goede
2016-08-19 21:30 ` Bin Liu
2016-08-21 10:10 ` Hans de Goede
2016-08-22 14:11 ` Bin Liu
2016-08-22 15:08 ` Hans de Goede
2016-08-22 15:24 ` Bin Liu
2016-08-22 15:32 ` Hans de Goede
2016-08-22 15:38 ` Bin Liu
2016-08-22 15:55 ` Hans de Goede
2016-08-22 16:10 ` Bin Liu
2016-08-25 17:59 ` Hans de Goede
2016-08-19 21:25 ` [PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs Bin Liu
2016-08-21 9:29 ` Hans de Goede
2016-08-22 14:08 ` Bin Liu
2016-08-22 14:16 ` Bin Liu
2016-08-22 15:50 ` Hans de Goede
2016-08-22 16:03 ` Bin Liu
2016-08-22 19:16 ` Rask Ingemann Lambertsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57B70929.5080707@ti.com \
--to=kishon@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox