Hi Arnd,

Am 15.11.2017 um 18:03 schrieb Arnd Bergmann [off-list ref]:

On Wed, Nov 15, 2017 at 5:27 PM, H. Nikolaus Schaller [off-list ref] wrote:

quoted

quoted

Am 15.11.2017 um 16:54 schrieb Arnd Bergmann [off-list ref]:
On Wed, Nov 15, 2017 at 4:19 PM, H. Nikolaus Schaller [off-list ref] wrote:

quoted

Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.

There is one more goal. Some people are dreaming about a generic GPS interface.
Then, the driver wouldn't have to register a /dev/GPS tty any more but a
gps_interface and mangle serial data as requested by that API. This will become
a simple upgrade.

So you can consider creating a new tty as sort of temporary solution. Like we
had for years for UART HCI based bluetooth devices using a user-space daemon.

It shouldn't be hard to split out the tty_driver portion of your file from the
part that registers the port, basically getting two files that each handle
half of the work, and the second one would be generic from the start.

Hm. Sounds like a big hack to me instead of using existing API (serdev and tty_port)
and making the best out of it.

But I may have misunderstood what you mean by splitting out parts of
a tty (which one?) and why I need two files?

The structure of the driver is:

UART --> serdev magic ---> this device driver ---> register something to present data to user space ---> user space read()

Data should flow following this arrows.
And power control happens this way:

UART --> serdev magic ---> this device driver <--- register something to present data to user space <--- user space open()
GPIO <----------------------------+

So we need one serdev port to communicate with the device and something to present serial data to user-space (where gpsd runs).

quoted

quoted

quoted

+       /* initialize the tty driver */
+       data->tty_drv->owner = THIS_MODULE;
+       data->tty_drv->driver_name = "w2sg0004";
+       data->tty_drv->name = "ttyGPS";
+       data->tty_drv->major = 0;
+       data->tty_drv->minor_start = 0;
+       data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
+       data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
+       data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
+       data->tty_drv->init_termios = tty_std_termios;
+       data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
+                                             HUPCL | CLOCAL;
+       /*
+        * optional:
+        * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
+                                       115200, 115200);
+        * w2sg_tty_termios(&data->tty_drv->init_termios);
+        */
+       tty_set_operations(data->tty_drv, &w2sg_serial_ops);

While I'm still not sure about why we want nested tty port, it
seems that we should have only one tty_driver that gets initialized
at module load time, rather than one driver structure per port.

If we have several such chips connected to different serdev UARTs
we need different /dev/GPS to separate them in user-space.

I understand that you need multiple devices, but I don't see
what having multiple drivers that all share the same name
"w2sg0004" helps. I would have expected that to fail in
tty_register_driver() when you get to the second driver,
but looking at the code, it doesn't actually try to make the name
unique

Yes, that is missing because I copied that from other drivers
and have no full understanding what is needed to make it really
work with multiple /dev/ttyGPS0 tty ports.

Therefore each probe (for each device connected to a different uart
of the SoC) should create a different /dev/ttyGPSn. Like you can have
multiple and independent i2c clients of the same type and driver.

proc_tty_register_driver() will fail to create the second
procfs file, but we ignore that failure.

Why not call tty_register_driver() in the init function rather than probe()?

We have no dedicated init function. Should we have one?

And if I understand correctly it would prohibit to fix the driver for the
multiple gps-devices situation. Or makes more work if the device is to be
registered as a future GPS interface.

So if the ->driver_name or ->name should have a dynamic sequence number,
please help me to get it correct.

quoted

quoted

quoted

+       /* register the tty driver */
+       err = tty_register_driver(data->tty_drv);
+       if (err) {
+               pr_err("%s - tty_register_driver failed(%d)\n",
+                       __func__, err);
+               put_tty_driver(data->tty_drv);
+               goto err_rfkill;
+       }
+
+       tty_port_init(&data->port);
+       data->port.ops = &w2sg_port_ops;
+
+/*
+ * FIXME: this appears to reenter this probe() function a second time
+ * which only fails because the gpio is already assigned
+ */
+
+       data->dev = tty_port_register_device(&data->port,
+                       data->tty_drv, minor, &serdev->dev);

This seems to be a result of having nested tty ports, and both
ports point to the same device.

The UART tty would be e.g. /dev/ttyO2 (on OMAP3) if no driver is
installed. And the new one that is registered is /dev/GPS0. So the
tty subsystem doesn't (or shouldn't) know they are related. They
are only related/connected inside this driver. So I assume that
some locking or reentrancy happens in tty_port_register_device().

I meant the serdev->dev pointer that you pass into
tty_port_register_device() seems to be the same one that
got passed into the first tty_port_register_device() in the
parent uart_port.

Ah, interesting!

Well, I copied that from other drivers registering a tty without
understanding all side-effects of everything.

Documentations of tty_port_register_device() says:
@device: parent if exists, otherwise NULL

Do we really need a "parent" here? Could we safely pass NULL?

I just checked the current mainline code, and it doesn't seem
to actually call serdev_tty_port_register() from
tty_port_register_device(), so maybe the comment was
based on an older version of the serdev framework?

Maybe. We rewrote the driver in parallel to v4.11-rc where this
was observed. Then we only rebased it to now v4.14 but didn't
verify this detail.

I did now test the driver with debugging enabled (after removing
the pdata stuff) on top of v4.14.

But I could not find a trace of this issue (there was a
double w2sg_probe() right after tty_port_register_device):

dmesg|fgrep w2sg
[    8.039184] w2sg_probe()
[    8.039184] w2sg serdev_device_set_drvdata
[    8.039398] w2sg_probe() lna_regulator = dc944c80
[    8.039398] w2sg devm_gpio_request
[    8.039703] w2sg rfkill_alloc
[    8.039733] w2sg register rfkill
[    8.039886] w2sg alloc_tty_driver
[    8.039916] w2sg tty_register_driver
[    8.039916] w2sg call tty_port_init
[    8.039916] w2sg call tty_port_register_device
[    8.040130] w2sg_rfkill_set_block: blocked: 0
[    8.040161] w2sg_set_lna_power: off
[    8.040222] w2sg tty_port_register_device -> ddeda800
[    8.040222] w2sg port.tty =   (null)
[    8.040222] w2sg probed
[    8.040222] w2sg DEBUGGING MODE enabled
[    8.040222] w2sg power gpio ON
[    8.252227] w2sg power gpio OFF
[    8.876617] w2sg_set_power to state=0 (requested=0)
[    9.127410] w2sg00x4 has sent 124 characters data although it should be off!
[    9.127471] w2sg_set_lna_power: off
[    9.127471] w2sg: power gpio ON
[    9.142700] w2sg: power gpio OFF
[    9.162689] w2sg: idle
[  239.280212] w2sg_tty_install() tty = ddfa3a00
[  239.284759] w2sg_tty_install() data = dc858810
[  239.290344] w2sg_tty_open() data = dc858810 open_count = ++0
[  239.296264] w2sg_set_power to state=1 (requested=0)
[  239.301940] w2sg00x4 scheduled for 1
[  239.305725] w2sg_set_lna_power: on
[  239.310913] w2sg: power gpio ON
[  239.327362] w2sg: power gpio OFF
[  239.347351] w2sg: idle
[  239.385162] w2sg00x4: push 1 chars to tty port
[  239.390228] w2sg00x4: push 4 chars to tty port
[  239.395141] w2sg00x4: push 5 chars to tty port
[  239.401184] w2sg00x4: push 5 chars to tty port
[  239.406097] w2sg00x4: push 4 chars to tty port
[  239.412994] w2sg00x4: push 6 chars to tty port
[  239.418731] w2sg00x4: push 5 chars to tty port
[  240.241821] w2sg_tty_close()
[  240.244873] w2sg_set_power to state=0 (requested=1)
[  240.251281] w2sg00x4 scheduled for 0
[  240.255065] w2sg_set_lna_power: off
[  240.261322] w2sg: power gpio ON
[  240.277435] w2sg: power gpio OFF
[  240.297424] w2sg: idle

So it is probed only once. Maybe we did note a bug in early
serdev that already has been fixed?

Hence we are discussing a problem that already has disappeared.

It seems like something that should be fixed, so maybe
put a WARN_ON() at the beginning of the probe
function to see where we come from.

Well, I'd say this notice can be removed as well.

So I'll post a v4 asap.

BR,
Nikolaus