From: Johan Hovold <johan@kernel.org>
To: Mathieu OTHACEHE <m.othacehe@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v2 1/4] USB: serial: fix compare_const_fl.cocci warnings
Date: Sun, 28 Feb 2016 14:27:04 +0100 [thread overview]
Message-ID: <20160228132704.GA32461@localhost> (raw)
In-Reply-To: <1454608890-22938-2-git-send-email-m.othacehe@gmail.com>
On Thu, Feb 04, 2016 at 07:01:27PM +0100, Mathieu OTHACEHE wrote:
> Move constants to the right of binary operators.
>
> Generated by: scripts/coccinelle/misc/compare_const_fl.cocci
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
> Move constants to the right improve readability in my opinion.
> But it's a matter of taste, and nothing is specified in CodingStyle.
>
> drivers/usb/serial/ch341.c | 2 +-
> drivers/usb/serial/f81232.c | 2 +-
> drivers/usb/serial/ftdi_sio.c | 16 +++++++-------
> drivers/usb/serial/ftdi_sio.h | 8 +++----
> drivers/usb/serial/garmin_gps.c | 48 ++++++++++++++++++++---------------------
> drivers/usb/serial/mos7840.c | 4 ++--
> 6 files changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c73808f..f139488 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -370,7 +370,7 @@ static void ch341_set_termios(struct tty_struct *tty,
> static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> {
> const uint16_t ch341_break_reg =
> - CH341_REG_BREAK1 | ((uint16_t) CH341_REG_BREAK2 << 8);
> + ((uint16_t) CH341_REG_BREAK2 << 8) | CH341_REG_BREAK1;
> struct usb_serial_port *port = tty->driver_data;
> int r;
> uint16_t reg_contents;
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 972f5a5..589d856 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -190,7 +190,7 @@ static int f81232_set_mctrl(struct usb_serial_port *port,
>
> /* force enable interrupt with OUT2 */
> mutex_lock(&priv->lock);
> - val = UART_MCR_OUT2 | priv->modem_control;
> + val = priv->modem_control | UART_MCR_OUT2;
I don't consider this an improvement as the modem-control bits are the
least significant bits of the register.
>
> if (clear & TIOCM_DTR)
> val &= ~UART_MCR_DTR;
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 8c660ae..07b0040 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1320,11 +1320,11 @@ static __u32 get_ftdi_divisor(struct tty_struct *tty,
> if (baud <= 3000000) {
> __u16 product_id = le16_to_cpu(
> port->serial->dev->descriptor.idProduct);
> - if (((FTDI_NDI_HUC_PID == product_id) ||
> - (FTDI_NDI_SPECTRA_SCU_PID == product_id) ||
> - (FTDI_NDI_FUTURE_2_PID == product_id) ||
> - (FTDI_NDI_FUTURE_3_PID == product_id) ||
> - (FTDI_NDI_AURORA_SCU_PID == product_id)) &&
> + if (((product_id == FTDI_NDI_HUC_PID) ||
> + (product_id == FTDI_NDI_SPECTRA_SCU_PID) ||
> + (product_id == FTDI_NDI_FUTURE_2_PID) ||
> + (product_id == FTDI_NDI_FUTURE_3_PID) ||
> + (product_id == FTDI_NDI_AURORA_SCU_PID)) &&
> (baud == 19200)) {
> baud = 1200000;
> }
> @@ -2325,7 +2325,7 @@ no_c_cflag_changes:
> usb_sndctrlpipe(dev, 0),
> FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> - 0 , (FTDI_SIO_RTS_CTS_HS | priv->interface),
> + 0, (priv->interface | FTDI_SIO_RTS_CTS_HS),
Similarly, here FTDI_SIO_RTS_CTS_HS is in the most significant byte.
> NULL, 0, WDR_TIMEOUT) < 0) {
> dev_err(ddev, "urb failed to set to rts/cts flow control\n");
> }
> @@ -2354,8 +2354,8 @@ no_c_cflag_changes:
> usb_sndctrlpipe(dev, 0),
> FTDI_SIO_SET_FLOW_CTRL_REQUEST,
> FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
> - urb_value , (FTDI_SIO_XON_XOFF_HS
> - | priv->interface),
> + urb_value, (priv->interface |
> + FTDI_SIO_XON_XOFF_HS),
Same here.
> NULL, 0, WDR_TIMEOUT) < 0) {
> dev_err(&port->dev, "urb failed to set to "
> "xon/xoff flow control\n");
> diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
> index ed58c6f..bbcc13df 100644
> --- a/drivers/usb/serial/ftdi_sio.h
> +++ b/drivers/usb/serial/ftdi_sio.h
> @@ -239,11 +239,11 @@ enum ftdi_sio_baudrate {
> */
>
> #define FTDI_SIO_SET_DTR_MASK 0x1
> -#define FTDI_SIO_SET_DTR_HIGH (1 | (FTDI_SIO_SET_DTR_MASK << 8))
> -#define FTDI_SIO_SET_DTR_LOW (0 | (FTDI_SIO_SET_DTR_MASK << 8))
> +#define FTDI_SIO_SET_DTR_HIGH ((FTDI_SIO_SET_DTR_MASK << 8) | 1)
> +#define FTDI_SIO_SET_DTR_LOW ((FTDI_SIO_SET_DTR_MASK << 8) | 0)
> #define FTDI_SIO_SET_RTS_MASK 0x2
> -#define FTDI_SIO_SET_RTS_HIGH (2 | (FTDI_SIO_SET_RTS_MASK << 8))
> -#define FTDI_SIO_SET_RTS_LOW (0 | (FTDI_SIO_SET_RTS_MASK << 8))
> +#define FTDI_SIO_SET_RTS_HIGH ((FTDI_SIO_SET_RTS_MASK << 8) | 2)
> +#define FTDI_SIO_SET_RTS_LOW ((FTDI_SIO_SET_RTS_MASK << 8) | 0)
>
> /*
> * ControlValue
> diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
> index db591d1..c0bbe44 100644
> --- a/drivers/usb/serial/garmin_gps.c
> +++ b/drivers/usb/serial/garmin_gps.c
> @@ -237,10 +237,10 @@ static inline int getDataLength(const __u8 *usbPacket)
> */
> static inline int isAbortTrfCmnd(const unsigned char *buf)
> {
> - if (0 == memcmp(buf, GARMIN_STOP_TRANSFER_REQ,
> - sizeof(GARMIN_STOP_TRANSFER_REQ)) ||
> - 0 == memcmp(buf, GARMIN_STOP_TRANSFER_REQ_V2,
> - sizeof(GARMIN_STOP_TRANSFER_REQ_V2)))
> + if (memcmp(buf, GARMIN_STOP_TRANSFER_REQ,
> + sizeof(GARMIN_STOP_TRANSFER_REQ)) == 0 ||
> + memcmp(buf, GARMIN_STOP_TRANSFER_REQ_V2,
> + sizeof(GARMIN_STOP_TRANSFER_REQ_V2)) == 0)
> return 1;
> else
> return 0;
> @@ -350,7 +350,7 @@ static int gsp_send_ack(struct garmin_data *garmin_data_p, __u8 pkt_id)
> unsigned l = 0;
>
> dev_dbg(&garmin_data_p->port->dev, "%s - pkt-id: 0x%X.\n", __func__,
> - 0xFF & pkt_id);
> + pkt_id & 0xFF);
pkt_id is u8
>
> *ptr++ = DLE;
> *ptr++ = ACK;
> @@ -366,7 +366,7 @@ static int gsp_send_ack(struct garmin_data *garmin_data_p, __u8 pkt_id)
> *ptr++ = DLE;
>
> *ptr++ = 0;
> - *ptr++ = 0xFF & (-cksum);
> + *ptr++ = (-cksum) & 0xFF;
> *ptr++ = DLE;
> *ptr++ = ETX;
>
> @@ -423,9 +423,9 @@ static int gsp_rec_packet(struct garmin_data *garmin_data_p, int count)
> n++;
> }
>
> - if ((0xff & (cksum + *recpkt)) != 0) {
> + if (((cksum + *recpkt) & 0xff) != 0) {
> dev_dbg(dev, "%s - invalid checksum, expected %02x, got %02x\n",
> - __func__, 0xff & -cksum, 0xff & *recpkt);
> + __func__, -cksum & 0xff, *recpkt & 0xff);
and so is recpkt
> return -EINVPKT;
> }
>
> @@ -528,7 +528,7 @@ static int gsp_receive(struct garmin_data *garmin_data_p,
> dev_dbg(dev, "NAK packet complete.\n");
> } else {
> dev_dbg(dev, "packet complete - id=0x%X.\n",
> - 0xFF & data);
> + data & 0xFF);
and data
> gsp_rec_packet(garmin_data_p, size);
> }
>
> @@ -636,7 +636,7 @@ static int gsp_send(struct garmin_data *garmin_data_p,
>
> garmin_data_p->outsize = 0;
>
> - if (GARMIN_LAYERID_APPL != getLayerId(garmin_data_p->outbuffer)) {
> + if (getLayerId(garmin_data_p->outbuffer) != GARMIN_LAYERID_APPL) {
> dev_dbg(dev, "not an application packet (%d)\n",
> getLayerId(garmin_data_p->outbuffer));
> return -1;
> @@ -688,7 +688,7 @@ static int gsp_send(struct garmin_data *garmin_data_p,
> *dst++ = DLE;
> }
>
> - cksum = 0xFF & -cksum;
> + cksum = -cksum & 0xFF;
> *dst++ = cksum;
> if (cksum == DLE)
> *dst++ = DLE;
> @@ -970,7 +970,7 @@ static void garmin_write_bulk_callback(struct urb *urb)
> struct garmin_data *garmin_data_p =
> usb_get_serial_port_data(port);
>
> - if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {
> + if (getLayerId(urb->transfer_buffer) == GARMIN_LAYERID_APPL) {
>
> if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
> gsp_send_ack(garmin_data_p,
> @@ -1025,7 +1025,7 @@ static int garmin_write_bulk(struct usb_serial_port *port,
> dismiss_ack ? NULL : port);
> urb->transfer_flags |= URB_ZERO_PACKET;
>
> - if (GARMIN_LAYERID_APPL == getLayerId(buffer)) {
> + if (getLayerId(buffer) == GARMIN_LAYERID_APPL) {
>
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags |= APP_REQ_SEEN;
> @@ -1077,9 +1077,9 @@ static int garmin_write(struct tty_struct *tty, struct usb_serial_port *port,
> pktsiz = getDataLength(garmin_data_p->privpkt);
> pktid = getPacketId(garmin_data_p->privpkt);
>
> - if (count == (GARMIN_PKTHDR_LENGTH+pktsiz)
> - && GARMIN_LAYERID_PRIVATE ==
> - getLayerId(garmin_data_p->privpkt)) {
> + if ((GARMIN_PKTHDR_LENGTH+pktsiz) == count &&
Here I believe count should remain on the left side.
> + getLayerId(garmin_data_p->privpkt) ==
> + GARMIN_LAYERID_PRIVATE) {
>
> dev_dbg(dev, "%s - processing private request %d\n",
> __func__, pktid);
> @@ -1192,7 +1192,7 @@ static void garmin_read_bulk_callback(struct urb *urb)
> garmin_read_process(garmin_data_p, data, urb->actual_length, 1);
>
> if (urb->actual_length == 0 &&
> - 0 != (garmin_data_p->flags & FLAGS_BULK_IN_RESTART)) {
> + (garmin_data_p->flags & FLAGS_BULK_IN_RESTART) != 0) {
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags &= ~FLAGS_BULK_IN_RESTART;
> spin_unlock_irqrestore(&garmin_data_p->lock, flags);
> @@ -1203,7 +1203,7 @@ static void garmin_read_bulk_callback(struct urb *urb)
> __func__, retval);
> } else if (urb->actual_length > 0) {
> /* Continue trying to read until nothing more is received */
> - if (0 == (garmin_data_p->flags & FLAGS_THROTTLED)) {
> + if ((garmin_data_p->flags & FLAGS_THROTTLED) == 0) {
> retval = usb_submit_urb(port->read_urb, GFP_ATOMIC);
> if (retval)
> dev_err(&port->dev,
> @@ -1249,12 +1249,12 @@ static void garmin_read_int_callback(struct urb *urb)
> urb->transfer_buffer);
>
> if (urb->actual_length == sizeof(GARMIN_BULK_IN_AVAIL_REPLY) &&
> - 0 == memcmp(data, GARMIN_BULK_IN_AVAIL_REPLY,
> - sizeof(GARMIN_BULK_IN_AVAIL_REPLY))) {
> + memcmp(data, GARMIN_BULK_IN_AVAIL_REPLY,
> + sizeof(GARMIN_BULK_IN_AVAIL_REPLY)) == 0) {
>
> dev_dbg(&port->dev, "%s - bulk data available.\n", __func__);
>
> - if (0 == (garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE)) {
> + if ((garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE) == 0) {
>
> /* bulk data available */
> retval = usb_submit_urb(port->read_urb, GFP_ATOMIC);
> @@ -1276,8 +1276,8 @@ static void garmin_read_int_callback(struct urb *urb)
> }
>
> } else if (urb->actual_length == (4+sizeof(GARMIN_START_SESSION_REPLY))
> - && 0 == memcmp(data, GARMIN_START_SESSION_REPLY,
> - sizeof(GARMIN_START_SESSION_REPLY))) {
> + && memcmp(data, GARMIN_START_SESSION_REPLY,
> + sizeof(GARMIN_START_SESSION_REPLY)) == 0) {
>
> spin_lock_irqsave(&garmin_data_p->lock, flags);
> garmin_data_p->flags |= FLAGS_SESSION_REPLY1_SEEN;
> @@ -1356,7 +1356,7 @@ static void garmin_unthrottle(struct tty_struct *tty)
> if (garmin_data_p->mode == MODE_NATIVE)
> garmin_flush_queue(garmin_data_p);
>
> - if (0 != (garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE)) {
> + if ((garmin_data_p->flags & FLAGS_BULK_IN_ACTIVE) != 0) {
> status = usb_submit_urb(port->read_urb, GFP_KERNEL);
> if (status)
> dev_err(&port->dev,
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index 2c69bfc..6a2fab8 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -2039,7 +2039,7 @@ static int mos7810_check(struct usb_serial *serial)
> /* Send the 1-bit test pattern out to MCS7810 test pin */
> usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> MCS_WRREQ, MCS_WR_RTYPE,
> - (0x0300 | (((test_pattern >> i) & 0x0001) << 1)),
> + ((((test_pattern >> i) & 0x0001) << 1) | 0x0300),
Again, I find it more natural to keep the most-significant bits to the
left.
> MODEM_CONTROL_REGISTER, NULL, 0, MOS_WDR_TIMEOUT);
>
> /* Read the test pattern back */
> @@ -2059,7 +2059,7 @@ static int mos7810_check(struct usb_serial *serial)
>
> /* Restore MCR setting */
> usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), MCS_WRREQ,
> - MCS_WR_RTYPE, 0x0300 | mcr_data, MODEM_CONTROL_REGISTER, NULL,
> + MCS_WR_RTYPE, mcr_data | 0x0300, MODEM_CONTROL_REGISTER, NULL,
Ditto.
> 0, MOS_WDR_TIMEOUT);
>
> kfree(buf);
Ok, so maybe partially accepting this patch wasn't such a good idea, but
I do find that at least the "y == f()" => "f() == y" transformations
increase readability.
Thanks,
Johan
next prev parent reply other threads:[~2016-02-28 13:27 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-04 18:01 [PATCH v2 0/4] USB: serial: Fix coccinelle warnings Mathieu OTHACEHE
2016-02-04 18:01 ` [PATCH v2 1/4] USB: serial: fix compare_const_fl.cocci warnings Mathieu OTHACEHE
2016-02-28 13:27 ` Johan Hovold [this message]
2016-02-04 18:01 ` [PATCH v2 2/4] USB: serial: fix returnvar.cocci warnings Mathieu OTHACEHE
2016-02-04 18:01 ` [PATCH v2 3/4] USB: serial: fix boolinit.cocci warnings Mathieu OTHACEHE
2016-02-04 18:01 ` [PATCH v2 4/4] USB: serial: fix semicolon.cocci warnings Mathieu OTHACEHE
2016-02-28 13:34 ` Johan Hovold
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=20160228132704.GA32461@localhost \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.othacehe@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.