Hi Arnd,
> Am 15.11.2017 um 18:03 schrieb Arnd Bergmann <arnd@xxxxxxxx>:
>
> On Wed, Nov 15, 2017 at 5:27 PM, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>> Am 15.11.2017 um 16:54 schrieb Arnd Bergmann <arnd@xxxxxxxx>:
>>> On Wed, Nov 15, 2017 at 4:19 PM, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>>> 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).
>
>>>> + /* 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.
>
>>>> + /* 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
