From: "Ilpo Järvinen" <ilpo.jarvinen@linux•intel.com>
To: Jiri Slaby <jirislaby@kernel•org>
Cc: linux-arm-kernel@lists•infradead.org,
Ulf Hansson <ulf.hansson@linaro•org>,
Alex Elder <elder@kernel•org>,
Pengutronix Kernel Team <kernel@pengutronix•de>,
Arnd Bergmann <arnd@arndb•de>,
linuxppc-dev@lists•ozlabs.org,
Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
Sascha Hauer <s.hauer@pengutronix•de>,
Oliver Neukum <oneukum@suse•com>,
linux-mmc@vger•kernel.org, Johan Hovold <johan@kernel•org>,
David Lin <dtwlin@gmail•com>,
linux-staging@lists•linux.dev, greybus-dev@lists•linaro.org,
linux-usb@vger•kernel.org, NXP Linux Team <linux-imx@nxp•com>,
linux-serial <linux-serial@vger•kernel.org>,
Fabio Estevam <festevam@gmail•com>,
Shawn Guo <shawnguo@kernel•org>,
LKML <linux-kernel@vger•kernel.org>
Subject: Re: [PATCH 07/10] tty: Convert ->dtr_rts() to take bool argument
Date: Thu, 5 Jan 2023 10:51:10 +0200 (EET) [thread overview]
Message-ID: <2cb9b67e-9bb-4fb2-e974-17050457d3@linux.intel.com> (raw)
In-Reply-To: <09043f30-c516-e173-3836-5e5dd5f5c472@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3064 bytes --]
On Thu, 5 Jan 2023, Jiri Slaby wrote:
> On 04. 01. 23, 16:15, Ilpo Järvinen wrote:
> > Convert the raise/on parameter in ->dtr_rts() to bool through the
> > callchain. The parameter is used like bool. In USB serial, there
> > remains a few implicit bool -> larger type conversions because some
> > devices use u8 in their control messages.
>
> Reviewed-by: Jiri Slaby <jirislaby@kernel•org>
>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux•intel.com>
> > ---
> ...
> > --- a/drivers/char/pcmcia/synclink_cs.c
> > +++ b/drivers/char/pcmcia/synclink_cs.c
> > @@ -378,7 +378,7 @@ static void async_mode(MGSLPC_INFO *info);
> > static void tx_timeout(struct timer_list *t);
> > static bool carrier_raised(struct tty_port *port);
> > -static void dtr_rts(struct tty_port *port, int onoff);
> > +static void dtr_rts(struct tty_port *port, bool onoff);
>
> Not anything for this patch, but having this dubbed "onoff" instead of "on"
> makes it really confusing.
>
> > --- a/drivers/mmc/core/sdio_uart.c
> > +++ b/drivers/mmc/core/sdio_uart.c
> > @@ -548,14 +548,14 @@ static bool uart_carrier_raised(struct tty_port
> > *tport)
> > * adjusted during an open, close and hangup.
> > */
> > -static void uart_dtr_rts(struct tty_port *tport, int onoff)
> > +static void uart_dtr_rts(struct tty_port *tport, bool onoff)
> > {
> > struct sdio_uart_port *port =
> > container_of(tport, struct sdio_uart_port, port);
> > int ret = sdio_uart_claim_func(port);
> > if (ret)
> > return;
> > - if (onoff == 0)
> > + if (!onoff)
> > sdio_uart_clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
> > else
> > sdio_uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
>
> Especially here. What does "!onoff" mean? If it were:
>
> if (on)
> sdio_uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
> else
> sdio_uart_clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
>
> it would be a lot more clear.
>
> > --- a/drivers/tty/amiserial.c
> > +++ b/drivers/tty/amiserial.c
> > @@ -1459,7 +1459,7 @@ static bool amiga_carrier_raised(struct tty_port
> > *port)
> > return !(ciab.pra & SER_DCD);
> > }
> > -static void amiga_dtr_rts(struct tty_port *port, int raise)
> > +static void amiga_dtr_rts(struct tty_port *port, bool raise)
>
> Or "raise". That makes sense too and we call it as such in
> tty_port_operations:
>
> > --- a/include/linux/tty_port.h
> > +++ b/include/linux/tty_port.h
> ...
> > @@ -32,7 +32,7 @@ struct tty_struct;
> > */
> > struct tty_port_operations {
> > bool (*carrier_raised)(struct tty_port *port);
> > - void (*dtr_rts)(struct tty_port *port, int raise);
> > + void (*dtr_rts)(struct tty_port *port, bool raise);
> > void (*shutdown)(struct tty_port *port);
> > int (*activate)(struct tty_port *port, struct tty_struct *tty);
> > void (*destruct)(struct tty_port *port);
>
> Care to fix that up too?
Sure. I noticed they were inconsistent but it didn't feel like changing
the name "while at it" would be good as this is long already. I think I'll
make another patch out of the name changes.
--
i.
prev parent reply other threads:[~2023-01-05 8:52 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230104151531.73994-1-ilpo.jarvinen@linux.intel.com>
2023-01-04 15:15 ` [PATCH 07/10] tty: Convert ->dtr_rts() to take bool argument Ilpo Järvinen
2023-01-05 6:19 ` Jiri Slaby
2023-01-05 8:51 ` Ilpo Järvinen [this message]
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=2cb9b67e-9bb-4fb2-e974-17050457d3@linux.intel.com \
--to=ilpo.jarvinen@linux$(echo .)intel.com \
--cc=arnd@arndb$(echo .)de \
--cc=dtwlin@gmail$(echo .)com \
--cc=elder@kernel$(echo .)org \
--cc=festevam@gmail$(echo .)com \
--cc=gregkh@linuxfoundation$(echo .)org \
--cc=greybus-dev@lists$(echo .)linaro.org \
--cc=jirislaby@kernel$(echo .)org \
--cc=johan@kernel$(echo .)org \
--cc=kernel@pengutronix$(echo .)de \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-imx@nxp$(echo .)com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-mmc@vger$(echo .)kernel.org \
--cc=linux-serial@vger$(echo .)kernel.org \
--cc=linux-staging@lists$(echo .)linux.dev \
--cc=linux-usb@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=oneukum@suse$(echo .)com \
--cc=s.hauer@pengutronix$(echo .)de \
--cc=shawnguo@kernel$(echo .)org \
--cc=ulf.hansson@linaro$(echo .)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