From: Rob Herring <robh@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: 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>,
Loic Poulain <loic.poulain@intel.com>,
Pavel Machek <pavel@ucw.cz>, NeilBrown <neil@brown.name>,
Linus Walleij <linus.walleij@linaro.org>,
"open list:BLUETOOTH DRIVERS" <linux-bluetooth@vger.kernel.org>,
"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
"linux-kernel@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 09:03:20 -0600 [thread overview]
Message-ID: <CAL_JsqKhmMa3Vs7fnfqzVYJbz-0=AwT6DckxSuf5ykDvCmNqBQ@mail.gmail.com> (raw)
In-Reply-To: <1484743377.2133.204.camel@linux.intel.com>
On Wed, Jan 18, 2017 at 6:42 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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.
It is not. It is the standard pattern of
menuconfig BLAH
if BLAH
...
endif
<EOF>
If I remove the "if", then SERIAL_DEV_CTRL_TTYPORT can be enabled when
SERIAL_DEV_BUS=n which breaks the build
> 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?
It's just to be clear it's the tty driver and index rather than this
driver's driver or index.
>> +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?
Probably. It's just what every ldisc uses. I suppose we could need
clients to set this, but we can add that as needed.
>> + 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?
Paranoia. Actually, looking at this closer, we can get rid of the
mutex altogether.
>> +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?
Didn't you ask this last time? See patch #9. tty_port_destructor()
calls this unconditionally as it doesn't know whether there's a serdev
or not. ctrl may be NULL, and then serport may be NULL.
Rob
next prev parent reply other threads:[~2017-01-18 15:03 UTC|newest]
Thread overview: 42+ 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-18 21:26 ` Rob Herring
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
2017-01-18 15:03 ` Rob Herring [this message]
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 1:36 ` msuchanek
2017-01-20 9:50 ` Sebastian Reichel
2017-01-20 9:50 ` Sebastian Reichel
2017-01-20 13:44 ` Greg Kroah-Hartman
2017-01-20 13:44 ` Greg Kroah-Hartman
2017-01-20 13:55 ` Linus Walleij
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:23 ` H. Nikolaus Schaller
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 15:26 ` Linus Walleij
2017-01-20 16:16 ` Andy Shevchenko
2017-01-20 16:16 ` Andy Shevchenko
2017-01-27 20:02 ` Pavel Machek
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='CAL_JsqKhmMa3Vs7fnfqzVYJbz-0=AwT6DckxSuf5ykDvCmNqBQ@mail.gmail.com' \
--to=robh@kernel.org \
--cc=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=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 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.