Hi Marcel, > Am 10.01.2017 um 13:02 schrieb Marcel Holtmann <mar...@holtmann.org>: > > Hi Nikolaus, > >>> Here goes another attempt at a serial device bus (aka uart slaves, tty >>> slaves, etc.). >>> >>> After some discussions with Dmitry at LPC, I decided to move away from >>> extending serio and moved back to making a new bus type instead. He didn't >>> think using serio was a good fit, and serio has a number of peculiarities >>> in regards to sysfs and it's driver model. I don't think we want to inherit >>> those for serial slave devices. >>> >>> This version sits on top of tty_port rather than uart_port as Alan >>> requested. Once I created a struct tty rather than moving everything >>> needed to tty_port, it became a lot easier and less invasive to the tty >>> core code. >>> >>> I have hacked up versions of the BT ldisc and TI ST drivers moved over to >>> use the serdev bus. I have BT working on the HiKey board which has TI BT. >>> With the serdev bus support, it eliminates the need for the TI userspace >>> UIM daemon. >>> >>> This series and the mentioned drivers can be found here[1]. >>> >>> Rob >>> >>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git >>> serial-bus-v2 >>> >>> Alan Cox (1): >>> tty_port: allow a port to be opened with a tty that has no file handle >>> >>> Rob Herring (8): >>> tty: move the non-file related parts of tty_release to new >>> tty_release_struct >>> tty_port: make tty_port_register_device wrap >>> tty_port_register_device_attr >>> tty: constify tty_ldisc_receive_buf buffer pointer >>> tty_port: Add port client functions >>> dt/bindings: Add a serial/UART attached device binding >>> serdev: Introduce new bus for serial attached devices >>> serdev: add a tty port controller driver >>> tty_port: register tty ports with serdev bus >>> >>> .../devicetree/bindings/serial/slave-device.txt | 34 ++ >>> MAINTAINERS | 8 + >>> drivers/char/Kconfig | 1 + >>> drivers/tty/Makefile | 1 + >>> drivers/tty/serdev/Kconfig | 16 + >>> drivers/tty/serdev/Makefile | 5 + >>> drivers/tty/serdev/core.c | 388 >>> +++++++++++++++++++++ >>> drivers/tty/serdev/serdev-ttyport.c | 244 +++++++++++++ >>> drivers/tty/tty_buffer.c | 19 +- >>> drivers/tty/tty_io.c | 44 ++- >>> drivers/tty/tty_port.c | 60 +++- >>> include/linux/serdev.h | 227 ++++++++++++ >>> include/linux/tty.h | 12 +- >>> 13 files changed, 1017 insertions(+), 42 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt >>> create mode 100644 drivers/tty/serdev/Kconfig >>> create mode 100644 drivers/tty/serdev/Makefile >>> create mode 100644 drivers/tty/serdev/core.c >>> create mode 100644 drivers/tty/serdev/serdev-ttyport.c >>> create mode 100644 include/linux/serdev.h >>> >>> -- >>> 2.10.1 >>> >> >> First of all many thanks for making another proposal! >> >> Instead of looking into the implementation details of your code I have >> hacked my w2sg0004 GPS driver (which works based on my proposed uart_slave >> driver) so that it makes use of your new serdev API. >> >> Here are some observations which I hope they give directions where your >> work can be improved: >> >> 1. it was quite easy to convert the driver to a serdev_device_driver :) >> >> The general driver structure could be taken unchanged and it was mainly >> platform_device -> serdev_device_driver and replacing my notification >> handlers by serdev_device_ops. >> >> Communication with the chip seems to work well. At least if it is >> unexpectedly >> turned on the driver receives the wrong NMEA records and turns the GPS >> chip off. That is the core of our power management scheme and why >> we need a serdev driver for this chip at all. >> >> So the general API for getting read/write access to the serial interface >> (and setting baud rate) from a device driver seems to be fine! >> >> >> 2. When I try to open the tty from user space to fetch the serial data I >> just get >> >> root@letux:~# cat /dev/ttyO1 >> [ 659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1) >> [ 665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1) >> ^C >> root@letux:~# >> >> So it does not seem to be possible to read the data from the tty any more. >> >> Maybe there can be some function serdev_device_set_shared(dev, flag). >> If set to exclusive the /dev node would be hidden from user-space. > > I would welcome hiding the /dev node completely as an option. That is > especially useful for systems where an upstream driver exists and hooks it up > directly into the Bluetooth subsystem already.
Yes, that is why I would like to see it hidden/exposed as an option. I don't really care if it is done by some DT property or such a function. > >> 3. for completely implementing my w2sg0004 driver (and two others) it would >> be nice to have additional serdev_device_ops: >> >> a) to be notified about user-space clients doing open/close on the tty >> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR) >> >> There may be other means (ldisc?) to get these notifications, but that >> needs the serdev driver to register with two different subsystems. >> >> Another approach could be to completely rewrite the driver so that it wraps >> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for >> user-space >> communication. Then it would be notified for all user-space and serial >> interface activities as a man-in-the-middle. >> >> But I expect that it delays the communication and is quite some overhead. > > My important thing to fix with GPS devices is that we can enumerate them from > userspace daemons correctly. So it either becomes its own GPS subsystem or we > need sysfs attributes like DEVTYPE clearly identifying them as GPS devices > (similar to what we did with network interfaces). > > So the question is really if a driver only needs to do power management on > open() and close() or if it also has to translate or transform the packets. > There are devices who speak NMEA and all is good. The device I want to upstream the driver speaks NMEA but should be powered down unless accessed... > And there are others that get plain raw data and need extra work in a daemon > to translate it into NMEA or some form of position information. Indeed and that should also be possible. In that case you likely must follow the man-in-the-middle approach, quite similar to how Rob has updated the ti-st driver. I have seen code where it creates some /dev/hci (if I remember correctly). BR, Nikolaus
