From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Rob Herring <robh@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Marcel Holtmann <marcel@holtmann.org>,
Jiri Slaby <jslaby@suse.com>, Sebastian Reichel <sre@kernel.org>,
Arnd Bergmann <arnd@arndb.de>,
"Dr . H . Nikolaus Schaller" <hns@goldelico.com>,
Peter Hurley <peter@hurleysoftware.com>,
Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Cc: Loic Poulain <loic.poulain@intel.com>,
Pavel Machek <pavel@ucw.cz>, NeilBrown <neil@brown.name>,
Linus Walleij <linus.walleij@linaro.org>,
linux-bluetooth@vger.kernel.org, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 8/9] serdev: add a tty port controller driver
Date: Wed, 18 Jan 2017 14:42:57 +0200 [thread overview]
Message-ID: <1484743377.2133.204.camel@linux.intel.com> (raw)
In-Reply-To: <20170116225436.17505-9-robh@kernel.org>
On Mon, 2017-01-16 at 16:54 -0600, Rob Herring wrote:
> Add a serdev controller driver for tty ports.
>
> The controller is registered with serdev when tty ports are registered
> with the TTY core. As the TTY core is built-in only, this has the side
> effect of making serdev built-in as well.
>
>
> +if SERIAL_DEV_BUS
> +
> +config SERIAL_DEV_CTRL_TTYPORT
> + bool "Serial device TTY port controller"
> + depends on TTY
> + depends on SERIAL_DEV_BUS != m
Since you have this line the
if SERIAL_DEV_BUS
is redundant for it.
So, leave either one or another (as an example you can look at
DMADEVICES).
> +
> +#define SERPORT_BUSY 1
> +#define SERPORT_ACTIVE 2
> +#define SERPORT_DEAD 3
> +
> +struct serport {
> + struct tty_port *port;
> + struct tty_struct *tty;
> + struct tty_driver *tty_drv;
> + int tty_idx;
Do you need tty_ prefix for them?
> + struct mutex lock;
> + unsigned long flags;
> +};
> +
> +/*
> + * Callback functions from the tty port.
> + */
> +
> +static int ttyport_receive_buf(struct tty_port *port, const unsigned
> char *cp,
> + const unsigned char *fp, size_t
> count)
> +{
> + struct serdev_controller *ctrl = port->client_data;
> + struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> + mutex_lock(&serport->lock);
> +
> + if (!test_bit(SERPORT_ACTIVE, &serport->flags))
So, if you are going to use serport->flags always under lock, you don't
need to use atomic bit operations.
Either
__test_bit() and Co
Or
flags & BIT(x)
> + goto out_unlock;
> +
> + serdev_controller_receive_buf(ctrl, cp, count);
> +
> +out_unlock:
> + mutex_unlock(&serport->lock);
> + return count;
> +}
> +
> +static void ttyport_write_wakeup(struct tty_port *port)
> +{
> + struct serdev_controller *ctrl = port->client_data;
> + struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> + clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags);
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
Hmm...
> + serdev_controller_write_wakeup(ctrl);
> +}
>
> + return tty_write_room(tty);
> +}
> +
> +
One extra line.
> +static int ttyport_open(struct serdev_controller *ctrl)
> +{
> + struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty;
> + struct ktermios ktermios;
> +
> + tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
> + serport->tty = tty;
> +
> + serport->port->client_ops = &client_ops;
> + serport->port->client_data = ctrl;
> +
>
> + tty->receive_room = 65536;
Magic?
> +
> + if (tty->ops->open)
> + tty->ops->open(serport->tty, NULL);
> + else
> + tty_port_open(serport->port, tty, NULL);
> +
> + /* Bring the UART into a known 8 bits no parity hw fc state
> */
> + ktermios = tty->termios;
> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP |
> + INLCR | IGNCR | ICRNL | IXON);
> + ktermios.c_oflag &= ~OPOST;
> + ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG |
> IEXTEN);
> + ktermios.c_cflag &= ~(CSIZE | PARENB);
> + ktermios.c_cflag |= CS8;
> + ktermios.c_cflag |= CRTSCTS;
> + tty_set_termios(tty, &ktermios);
> +
> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +
>
> + mutex_lock(&serport->lock);
> + set_bit(SERPORT_ACTIVE, &serport->flags);
> + mutex_unlock(&serport->lock);
So, some clarification would be good to have to understand why you need
mutex _and_ atomic operation together.
What does mutex protect?
> +
> + tty_unlock(serport->tty);
> + return 0;
> +}
> +void serdev_tty_port_unregister(struct tty_port *port)
> +{
> + struct serdev_controller *ctrl = port->client_data;
> + struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> +
> + if (!serport)
> + return;
What this check prevents from?
> +
> + serdev_controller_remove(ctrl);
> + port->client_ops = NULL;
> + port->client_data = NULL;
> + serdev_controller_put(ctrl);
> +}
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
next prev parent reply other threads:[~2017-01-18 12:43 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-16 22:54 [PATCH v2 0/9] Serial slave device bus Rob Herring
2017-01-16 22:54 ` [PATCH v2 1/9] tty: move the non-file related parts of tty_release to new tty_release_struct Rob Herring
2017-01-16 22:54 ` [PATCH v2 2/9] tty_port: allow a port to be opened with a tty that has no file handle Rob Herring
2017-01-17 14:57 ` One Thousand Gnomes
2017-01-19 13:37 ` Greg Kroah-Hartman
2017-01-19 15:05 ` Rob Herring
2017-01-19 15:23 ` Rob Herring
2017-01-16 22:54 ` [PATCH v2 3/9] tty_port: make tty_port_register_device wrap tty_port_register_device_attr Rob Herring
2017-01-16 22:54 ` [PATCH v2 4/9] tty: constify tty_ldisc_receive_buf buffer pointer Rob Herring
2017-01-19 16:24 ` Greg Kroah-Hartman
2017-01-16 22:54 ` [PATCH v2 5/9] tty_port: Add port client functions Rob Herring
2017-01-16 22:54 ` [PATCH v2 6/9] dt/bindings: Add a serial/UART attached device binding Rob Herring
2017-01-16 22:54 ` [PATCH v2 7/9] serdev: Introduce new bus for serial attached devices Rob Herring
2017-01-18 11:53 ` Frédéric Danis
2017-01-18 21:26 ` Rob Herring
2017-01-16 22:54 ` [PATCH v2 8/9] serdev: add a tty port controller driver Rob Herring
2017-01-18 12:42 ` Andy Shevchenko [this message]
2017-01-18 15:03 ` Rob Herring
2017-01-16 22:54 ` [PATCH v2 9/9] tty_port: register tty ports with serdev bus Rob Herring
2017-01-17 11:07 ` [PATCH v2 0/9] Serial slave device bus Pavel Machek
2017-01-20 1:36 ` msuchanek
2017-01-20 9:50 ` Sebastian Reichel
2017-01-20 13:44 ` Greg Kroah-Hartman
2017-01-20 13:55 ` Linus Walleij
2017-01-20 14:14 ` Marcel Holtmann
2017-01-20 14:23 ` H. Nikolaus Schaller
2017-01-20 14:22 ` GPS drivers (was Re: [PATCH v2 0/9] Serial slave device bus) Pavel Machek
2017-01-20 15:26 ` Linus Walleij
2017-01-20 16:16 ` Andy Shevchenko
2017-01-27 20:02 ` Pavel Machek
2017-01-20 20:23 ` Pavel Machek
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=1484743377.2133.204.camel@linux.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=gnomes@lxorguk.ukuu.org.uk \
--cc=gregkh@linuxfoundation.org \
--cc=hns@goldelico.com \
--cc=jslaby@suse.com \
--cc=linus.walleij@linaro.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=loic.poulain@intel.com \
--cc=marcel@holtmann.org \
--cc=neil@brown.name \
--cc=pavel@ucw.cz \
--cc=peter@hurleysoftware.com \
--cc=robh@kernel.org \
--cc=sre@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).