Hi Rob,

Am 13.01.2017 um 15:48 schrieb Rob Herring [off-list ref]:

On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller [off-list ref] wrote:

quoted

Hi Rob,
was it intentional to answer privately only?

Damn gmail. Added everyone back.

No problem. Happens to everyone every now and then.

quoted

quoted

Am 12.01.2017 um 23:07 schrieb Rob Herring [off-list ref]:

On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller [off-list ref] wrote:

quoted

Hi Rob,

quoted

Am 06.01.2017 um 17:26 schrieb Rob Herring [off-list ref]:

[...]

quoted

quoted

quoted

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 don't think sharing should be allowed. Either you have an in-kernel
driver or you handle it in userspace. Sharing is just asking for
trouble IMO.

My tty-slave patch series works and has no trouble with sharing (the UART)
because it was designed with this in mind.

The only trouble is that it did not find maintainer's acceptance...

quoted

Though it could be supported later.

Firstly, let me point out once again that we have a mobile device, battery
powered and every component should be and stay turned off, if not needed by
any consumer.

That's every device...

quoted

The only component that can reliably detect if there is no consumer in user-space
is the kernel. Hence it must be a kernel driver that powers the device on/off.

On the other side, it should simply pass data unmodified to user space (because the
chip provides cooked data), so we do not need a driver for doing any data processing.
The data stream is provided perfectly (without proper power control) by simply
accessing /dev/ttyO1 from user-space.

So if there were no power control topic, we would not even ask for a driver.

I think this reasoning is exactly why we have no proper subsystem
today. We *almost* don't need one. It all works fine except I have
this one GPIO to control. Oh, and a regulator and firmware and
suspend/resume control and ...

In our case we have no firmware, just the gpio.

quoted

We only need the driver to detect the power state of the chip by *monitoring*
the data stream that goes to user space.

Of course shared writing to the chip would give trouble, but we do not need it.
Therefore I am happy if this sharing is an option (with a big WARNING sign).

If we would try to achieve the same power management in user-space we would have
to run a power-consuming daemon and make sure that it *never* crashes (or people
come and complain about short battery life even if they think they have GPS
turned off).

And we need to be able to maintain the daemon for many different distributions
people want to run on our device. This takes years to get it into Debian and
others where we are not developers at all.

Exactly one of the problems we're trying to solve. With your desired
approach though, you are still leaving the problem of having to know
which tty device the GPS chip is connected to which varies with each
board.
Either you hardcode the tty device in userspace, provide some
sysfs file with the tty name (like TI-ST) or link, provide the tty
name in DT (which I'll NAK) or have userspace parse the DT to find the
connection. I've seen all but the last case. I want to solve this
problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
device.

Yes, this is indeed an issue. There are also USB or Bluetooth based external
GPS devices which do not need a driver because they speak some standard
profile, identify themselves as GPS and create some tty port through standard
mechanisms.

quoted

So this data stream sharing/monitoring is the most important part for getting our
chip supported by a kernel driver.

quoted

I've updated the series to skip creating the /dev nodes.

That is exactly what we do NOT need for this chip. Now I can't even access it
any more when powered on...

It's a minor change. Essentially, it is call tty_register_device_attr
or not.

Fine!

There's the issue of the file open count warning which I don't
know how to solve.

Unfortunately I am not experienced with the tty layer to help here.

quoted

quoted

quoted

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.

This was my thinking. If we only need data read and write, I don't
think we gain much using a tty vs. a new char dev.

We gain re-use of the existing tty for its key purpose to mediate between an uart
device and user-space.

I have looked into the chardev approach and it appears to be quite some overkill
to copy serial data from the UART to a user space file handle.

About buffering: with the chardev approach we have to implement our own buffer
in the driver and decide what happens on overflow while the tty layer already
efficiently handles this.

Not exactly. There's already buffering in tty_buffer.c. That handles
overflow of the UART. Then there is a 4K circular buffer in n_tty. The
question really is how much of the per character and flag processing
of n_tty do you need as that is where the complexity is. I'd guess not
much of it. If it is needed, then perhaps n_tty.c could be refactored
to provide common functions.

Ah, that is good if the serdev driver can rely on / reuse this buffer.

Where I don't see immediately is how I can make the chardev read file operation
block until I get some receive_buf serdev_device_op and how I manage different
data size requests without introducing another driver-internal buffer.

So it could be better to alloc_tty_driver another tty and copy to its
read buffer. 

quoted

And it is a little strange that the device can either be accessed through
/dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.

We have similar things with SPI, I2C, and USB. Either you have a
kernel driver or you have a userspace driver. The userspace drivers
have limitations and if those limitations are a problem, we right
kernel drivers instead.

quoted

New problems arise if there were two such chips. Then we must be prepared
that several instances provide different /dev/gps[1-9] nodes and that they
can be identified and are stable. Maybe we have to introduce udev-rules...

That's a solved problem generally (though not all like the answer).

Well, I have no problem doing it that way but it increases complexity of the
driver and makes it more difficult to configure in user space.

quoted

So what seems to be an obvious and straightforward solution (from serdev perspective)
is not, if we look into implementation details of the driver.

I can't solve all problems for all possible drivers on day one. It
does provide a solution for drivers that are already in the kernel.

Well, I hope you understand that I am a little impatient here...

It is more than 2 years ago that we proposed the first driver for this chip
for upstreaming (while we have something running on our own kernels much longer
time):

	http://lkml.iu.edu/hypermail/linux/kernel/1410.2/00634.html

And if I counted correctly, I am now working on the fourth completely different
proposal with no finalization in sight.

So this situation is a little circular: because we are not in kernel we have to
wait longer until we can get in...

I'd suggest we debate adding sharing capability vs. a GPS subsystem
separately.

Well, I would be perfectly happy without having to wait for a fully worked out
GPS subsystem. If it arrives in my (device's) life-time we can rework the driver
to fit into it.

This approach seems not to be an exception to me. For example the original misc/bmp085
driver has completely gone in 4.9 and been replaced by a generic iio driver.

If there was already a GPS subsystem there would be no
debate. I don't think what's here is preventing either case.
Similarly, I don't pretend the configuration API (baud rate and
flow-control) is complete. I'm sure someone will need additional
functions, but those can all be incrementally added as needed.

quoted

quoted

quoted

But I expect that it delays the communication and is quite some overhead.


4. It seems as if I have to modprobe the driver explicitly (it is not
located and loaded automatically based on the compatible string in DT
like i2c clients).

I've added what I think should be needed for that. I pushed out a new
branch[1]. Can you give it a try?

Yes, sure. I have tried and now our driver module is loaded as expected :)

But in general we are turning away from a solution for our w2sg0004 driver
(see above).

BTW: I see an issue in our kernel (config?) that the console and initd
blocks for approx. 60 seconds during boot when serdev is merged (even
if not configured). This issue disappears when using the omap2plus_defconfig.

But I have not digged into that (because it may be spurious or EPROBE_DEFER
related).

I've not seen anything like that. Do you have a diff of your configs?
And what is your init system?

I did use Debian with sysv init and a getty on /dev/ttyO2 (OMAP3 console UART).

But let me cross-check some things before digging deeper into it. To exclude
that it is our fault and indeed in the serdev patch set.

BR and thanks,
Nikolaus