Re: [PATCH v3 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

[H. Nikolaus Schaller]

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