[PATCH v4 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver
H. Nikolaus Schaller
hns at goldelico.com
Thu Nov 30 23:49:22 PST 2017
Hi,
> Am 29.11.2017 um 15:47 schrieb Andrew F. Davis <afd at ti.com>:
>
> On 11/28/2017 03:42 PM, H. Nikolaus Schaller wrote:
>> Hi Andrew,
>> now as 4.15-rc1 is out I find time to continue this work and prepare [PATCH v5].
>>
>>> Am 23.11.2017 um 17:06 schrieb Andrew F. Davis <afd at ti.com>:
>>>
>>> On 11/15/2017 03:37 PM, H. Nikolaus Schaller wrote:
>>>> Add driver for Wi2Wi W2SG0004/84 GPS module connected through uart.
>>>>
>>>> Use serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
>>>> and turn on/off the module. It also detects if the module is turned on (sends data)
>>>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>>>>
>>>> Additionally, rfkill block/unblock can be used to control an external LNA
>>>> (and power down the module if not needed).
>>>>
>>>> The driver concept is based on code developed by NeilBrown <neilb at suse.de>
>>>> but simplified and adapted to use the new serdev API introduced in 4.11.
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <hns at goldelico.com>
>>>> ---
>>>> drivers/misc/Kconfig | 10 +
>>>> drivers/misc/Makefile | 1 +
>>>> drivers/misc/w2sg0004.c | 545 ++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> I wonder if this shouldn't better go to
>>
>> drivers/gps
>>
>> But this directory does not yet exist and has no overall maintainer.
>> So it could be left in drivers/misc and moved as soon as drivers/gps
>> is created elsewhere.
>>
>
> Making that directory would imply the existence of a GPS driver
> framework, until one comes about (or if you would like to create one), I
> think misc is the most appropriate spot for now.
Understood and fine with it.
I just wanted to have asked and clarified.
>
>>>> 3 files changed, 556 insertions(+)
>>>> create mode 100644 drivers/misc/w2sg0004.c
>>>>
>>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>>> index 8136dc7e863d..09d171d68408 100644
>>>> --- a/drivers/misc/Kconfig
>>>> +++ b/drivers/misc/Kconfig
>>>> @@ -518,4 +518,14 @@ source "drivers/misc/mic/Kconfig"
>>>> source "drivers/misc/genwqe/Kconfig"
>>>> source "drivers/misc/echo/Kconfig"
>>>> source "drivers/misc/cxl/Kconfig"
>>>> +
>>>> +config W2SG0004
>>>> + tristate "W2SG00x4 on/off control"
>>>> + depends on GPIOLIB && SERIAL_DEV_BUS
>>>> + help
>>>> + Enable on/off control of W2SG00x4 GPS moduled connected
>>>> + to some SoC UART to allow powering up/down if the /dev/ttyGPSn
>>>> + is opened/closed.
>>>> + It also provides a rfkill gps name to control the LNA power.
>>>> +
>>>> endmenu
>>>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>>>> index ad0e64fdba34..abcb667e0ff0 100644
>>>> --- a/drivers/misc/Makefile
>>>> +++ b/drivers/misc/Makefile
>>>> @@ -51,6 +51,7 @@ obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
>>>> obj-y += mic/
>>>> obj-$(CONFIG_GENWQE) += genwqe/
>>>> obj-$(CONFIG_ECHO) += echo/
>>>> +obj-$(CONFIG_W2SG0004) += w2sg0004.o
>>>> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>>>> obj-$(CONFIG_CXL_BASE) += cxl/
>>>> obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
>>>> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
>>>> new file mode 100644
>>>> index 000000000000..12e14b5e0a99
>>>> --- /dev/null
>>>> +++ b/drivers/misc/w2sg0004.c
>>>> @@ -0,0 +1,545 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>
>>> Damn this looks ugly, oh well :/
>>
>> I could remove it for [PATCH v5] ... :\
>>
>
> Nah, you should leave it, this comment was just me venting, Greg KH has
> spoken, so this is the correct way now.
Yes, sigh...
>
>>>
>>>> +/*
>>>> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
>>>> + *
>>>
>>>
>>> Think you still need copyright tag here somewhere.
>>
>> At the bottom there is:
>>
>>>> +MODULE_AUTHOR("NeilBrown <neilb at suse.de>");
>>>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>>>> +MODULE_LICENSE("GPL v2");
>>
>> Isn't that enough any more?
>>
>
> Not sure.. someone needs to hold the copyright to this file and it
> really should be close to the top.
Ok, I'll add some (C) line(s) matching the author(s).
>
>>>
>>>> + * This receiver has an ON/OFF pin which must be toggled to
>>>> + * turn the device 'on' of 'off'. A high->low->high toggle
>>>> + * will switch the device on if it is off, and off if it is on.
>>>> + *
>>>> + * To enable receiving on/off requests we register with the
>>>> + * UART power management notifications.
>>>> + *
>>>> + * It is not possible to directly detect the state of the device.
>>>> + * However when it is on it will send characters on a UART line
>>>> + * regularly.
>>>> + *
>>>> + * To detect that the power state is out of sync (e.g. if GPS
>>>> + * was enabled before a reboot), we register for UART data received
>>>> + * notifications.
>>>> + *
>>>> + * In addition we register as a rfkill client so that we can
>>>> + * control the LNA power.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/of_gpio.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/rfkill.h>
>>>> +#include <linux/serdev.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/tty.h>
>>>> +#include <linux/tty_flip.h>
>>>> +#include <linux/workqueue.h>
>>>> +
>>>> +/*
>>>> + * There seems to be restrictions on how quickly we can toggle the
>>>> + * on/off line. data sheets says "two rtc ticks", whatever that means.
>>>> + * If we do it too soon it doesn't work.
>>>> + * So we have a state machine which uses the common work queue to ensure
>>>> + * clean transitions.
>>>> + * When a change is requested we record that request and only act on it
>>>> + * once the previous change has completed.
>>>> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
>>>> + * one change per second.
>>>> + */
>>>> +
>>>> +enum w2sg_state {
>>>> + W2SG_IDLE, /* is not changing state */
>>>> + W2SG_PULSE, /* activate on/off impulse */
>>>> + W2SG_NOPULSE /* deactivate on/off impulse */
>>>> +};
>>>> +
>>>> +struct w2sg_data {
>>>> + struct rfkill *rf_kill;
>>>> + struct regulator *lna_regulator;
>>>> + int lna_blocked; /* rfkill block gps active */
>>>> + int lna_is_off; /* LNA is currently off */
>>>> + int is_on; /* current state (0/1) */
>>>> + unsigned long last_toggle;
>>>> + unsigned long backoff; /* time to wait since last_toggle */
>>>> + int on_off_gpio; /* the on-off gpio number */
>>>> + struct serdev_device *uart; /* uart connected to the chip */
>>>> + struct tty_driver *tty_drv; /* this is the user space tty */
>>>> + struct device *dev; /* from tty_port_register_device() */
>>>> + struct tty_port port;
>>>> + int open_count; /* how often we were opened */
>>>> + enum w2sg_state state;
>>>> + int requested; /* requested state (0/1) */
>>>> + int suspended;
>>>> + struct delayed_work work;
>>>> + int discard_count;
>>>> +};
>>>> +
>>>
>>> Kernel doc style.
>>
>> Is this really needed here?
>>
>> For pure driver internal structs (they are not kernel infrastructure API) I usually
>> consult the source code of a driver and never well formatted kernel doc. Hence I think
>> readability by programmers looking into the source file is more important than serving
>> kernel doc tools.
>>
>> Yes, there are examples like:
>>
>> https://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/iio/accel/mma8452.c#L113
>>
>> But I find them more confusing than helpful because I have to jump between code lines
>> to match the comment with the data type.
>>
>
> I have no strong preference here as long as you think this is the most
> readable way.
Ok, then let's leave it as it is.
>
>>>
>>>> +static struct w2sg_data *w2sg_by_minor[1];
>>>> +
>>>
>>> If you can only have one right now then just drop the array.
>>
>> w2sg_get_by_minor() is prepared to access more than one record.
>> And there are plans to have more than one (but I don't know exactly
>> how to provide it).
>>
>
> Just add it back when you get that functionality.
I have added a BUG_ON if anyone tries to access an index different from 0.
Why BUG_ON? Because there is no way to cure this situation safely.
>
>> Making it a non-array seems to be an unnecessary hurdle for such
>> improvements. And regarding memory footprint, a single-element
>> array is equivalent to a non-array.
>>
>
> This comment was not about memory footprint, it's about sane code practices.
Well, I would consider using a 1-element array when the plan is to use an n-element
array as more future-proof...
But I have changed it for v5.
>
>>>
>>>> +static int w2sg_set_lna_power(struct w2sg_data *data)
>>>> +{
>>>> + int ret = 0;
>>>> + int off = data->suspended || !data->requested || data->lna_blocked;
>>>> +
>>>> + pr_debug("%s: %s\n", __func__, off ? "off" : "on");
>>>> +
>>>> + if (off != data->lna_is_off) {
>>>> + data->lna_is_off = off;
>>>> + if (!IS_ERR_OR_NULL(data->lna_regulator)) {
>>>> + if (off)
>>>> + regulator_disable(data->lna_regulator);
>>>> + else
>>>> + ret = regulator_enable(data->lna_regulator);
>>>> + }
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void w2sg_set_power(void *pdata, int val)
>>>> +{
>>>> + struct w2sg_data *data = (struct w2sg_data *) pdata;
>>>> +
>>>> + pr_debug("%s to state=%d (requested=%d)\n", __func__, val, data->requested);
>>>> +
>>>> + if (val && !data->requested) {
>>>> + data->requested = true;
>>>> + } else if (!val && data->requested) {
>>>> + data->backoff = HZ;
>>>> + data->requested = false;
>>>> + } else
>>>> + return;
>>>> +
>>>> + pr_debug("w2sg00x4 scheduled for %d\n", data->requested);
>>>> +
>>>> + if (!data->suspended)
>>>> + schedule_delayed_work(&data->work, 0);
>>>> +}
>>>> +
>>>> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
>>>> +
>>>> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
>>>> + const unsigned char *rxdata,
>>>> + size_t count)
>>>> +{
>>>> + struct w2sg_data *data =
>>>> + (struct w2sg_data *) serdev_device_get_drvdata(serdev);
>>>> +
>>>> + if (!data->requested && !data->is_on) {
>>>> + /*
>>>> + * we have received characters while the w2sg
>>>> + * should have been be turned off
>>>> + */
>>>> + data->discard_count += count;
>>>> + if ((data->state == W2SG_IDLE) &&
>>>> + time_after(jiffies,
>>>> + data->last_toggle + data->backoff)) {
>>>> + /* Should be off by now, time to toggle again */
>>>> + pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
>>>> + data->discard_count);
>>>> +
>>>> + data->discard_count = 0;
>>>> +
>>>> + data->is_on = true;
>>>> + data->backoff *= 2;
>>>> + if (!data->suspended)
>>>> + schedule_delayed_work(&data->work, 0);
>>>> + }
>>>> + } else if (data->open_count > 0) {
>>>> + int n;
>>>> +
>>>> + pr_debug("w2sg00x4: push %lu chars to tty port\n",
>>>> + (unsigned long) count);
>>>> +
>>>> + /* pass to user-space */
>>>> + n = tty_insert_flip_string(&data->port, rxdata, count);
>>>> + if (n != count)
>>>> + pr_err("w2sg00x4: did loose %lu characters\n",
>>>> + (unsigned long) (count - n));
>>>> + tty_flip_buffer_push(&data->port);
>>>> + return n;
>>>> + }
>>>> +
>>>> + /* assume we have processed everything */
>>>> + return count;
>>>> +}
>>>> +
>>>> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
>>>> +
>>>
>>> drop extra line, same everywhere
>>
>> Ok! Was just this and one more location.
>>
>>>
>>>> +static void toggle_work(struct work_struct *work)
>>>> +{
>>>> + struct w2sg_data *data = container_of(work, struct w2sg_data,
>>>> + work.work);
>>>> +
>>>> + switch (data->state) {
>>>> + case W2SG_IDLE:
>>>> + if (data->requested == data->is_on)
>>>> + return;
>>>> +
>>>> + w2sg_set_lna_power(data); /* update LNA power state */
>>>> + gpio_set_value_cansleep(data->on_off_gpio, 0);
>>>> + data->state = W2SG_PULSE;
>>>> +
>>>> + pr_debug("w2sg: power gpio ON\n");
>>>> +
>>>> + schedule_delayed_work(&data->work,
>>>> + msecs_to_jiffies(10));
>>>> + break;
>>>> +
>>>> + case W2SG_PULSE:
>>>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>>>> + data->last_toggle = jiffies;
>>>> + data->state = W2SG_NOPULSE;
>>>> + data->is_on = !data->is_on;
>>>> +
>>>> + pr_debug("w2sg: power gpio OFF\n");
>>>> +
>>>> + schedule_delayed_work(&data->work,
>>>> + msecs_to_jiffies(10));
>>>> + break;
>>>> +
>>>> + case W2SG_NOPULSE:
>>>> + data->state = W2SG_IDLE;
>>>> +
>>>> + pr_debug("w2sg: idle\n");
>>>> +
>>>> + break;
>>>> +
>>>> + }
>>>> +}
>>>> +
>>>> +static int w2sg_rfkill_set_block(void *pdata, bool blocked)
>>>> +{
>>>> + struct w2sg_data *data = pdata;
>>>> +
>>>> + pr_debug("%s: blocked: %d\n", __func__, blocked);
>>>> +
>>>> + data->lna_blocked = blocked;
>>>> +
>>>> + return w2sg_set_lna_power(data);
>>>> +}
>>>> +
>>>> +static struct rfkill_ops w2sg0004_rfkill_ops = {
>>>> + .set_block = w2sg_rfkill_set_block,
>>>> +};
>>>> +
>>>> +static struct serdev_device_ops serdev_ops = {
>>>> + .receive_buf = w2sg_uart_receive_buf,
>>>> +};
>>>> +
>>>> +/*
>>>> + * we are a man-in the middle between the user-space visible tty port
>>>> + * and the serdev tty where the chip is connected.
>>>> + * This allows us to recognise when the device should be powered on
>>>> + * or off and handle the "false" state that data arrives while no
>>>> + * users-space tty client exists.
>>>> + */
>>>> +
>>>> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
>>>> +{
>>>> + return w2sg_by_minor[minor];
>>>> +}
>>>> +
>>>> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>>>> +{
>>>> + struct w2sg_data *data;
>>>> + int retval;
>>>> +
>>>> + pr_debug("%s() tty = %p\n", __func__, tty);
>>>> +
>>>> + data = w2sg_get_by_minor(tty->index);
>>>> +
>>>> + if (!data)
>>>> + return -ENODEV;
>>>> +
>>>> + retval = tty_standard_install(driver, tty);
>>>> + if (retval)
>>>> + goto error_init_termios;
>>>> +
>>>> + tty->driver_data = data;
>>>> +
>>>> + return 0;
>>>> +
>>>> +error_init_termios:
>>>> + tty_port_put(&data->port);
>>>> + return retval;
>>>> +}
>>>> +
>>>> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
>>>> +{
>>>> + struct w2sg_data *data = tty->driver_data;
>>>> +
>>>> + pr_debug("%s() data = %p open_count = ++%d\n", __func__, data, data->open_count);
>>>> +
>>>> + w2sg_set_power(data, ++data->open_count > 0);
>>>> +
>>>> + return tty_port_open(&data->port, tty, file);
>>>> +}
>>>> +
>>>> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
>>>> +{
>>>> + struct w2sg_data *data = tty->driver_data;
>>>> +
>>>> + pr_debug("%s()\n", __func__);
>>>> +
>>>> + w2sg_set_power(data, --data->open_count > 0);
>>>> +
>>>> + tty_port_close(&data->port, tty, file);
>>>> +}
>>>> +
>>>> +static int w2sg_tty_write(struct tty_struct *tty,
>>>> + const unsigned char *buffer, int count)
>>>> +{
>>>> + struct w2sg_data *data = tty->driver_data;
>>>> +
>>>> + /* simply pass down to UART */
>>>> + return serdev_device_write_buf(data->uart, buffer, count);
>>>> +}
>>>> +
>>>> +static const struct tty_operations w2sg_serial_ops = {
>>>> + .install = w2sg_tty_install,
>>>> + .open = w2sg_tty_open,
>>>> + .close = w2sg_tty_close,
>>>> + .write = w2sg_tty_write,
>>>> +};
>>>> +
>>>> +static const struct tty_port_operations w2sg_port_ops = {
>>>> +};
>>>
>>> drop the brackets? or use NULL below, not sure if this needs memory.
>>
>> This allocates a struct tty_port_operations initialized with NULL for
>> all function pointers.
>>
>
> Then just drop the brackets, or allocate it with kzalloc.
>
>> I am not sure if this is the same as providing no w2sg_port_ops at all.
>>
>> Rather I think the serial core is only port == NULL safe but not
>> port->ops == NULL e.g.:
>>
>> http://elixir.free-electrons.com/linux/v4.15-rc1/source/drivers/tty/tty_port.c#L422
>>
>> A test confirms (see comment below):
>>
>>>
>>>> +
>>>> +static int w2sg_probe(struct serdev_device *serdev)
>>>> +{
>>>> + struct w2sg_data *data;
>>>> + struct rfkill *rf_kill;
>>>> + int err;
>>>> + int minor;
>>>> + enum of_gpio_flags flags;
>>>> +
>>>> + pr_debug("%s()\n", __func__);
>>>> +
>>>> + minor = 0;
>>>> + if (w2sg_by_minor[minor]) {
>>>> + pr_err("w2sg minor is already in use!\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
>>>> + if (data == NULL)
>>>> + return -ENOMEM;
>>>> +
>>>> + w2sg_by_minor[minor] = data;
>>
>> While looking through this, I found a potential issue if allocating the
>> gpio fails with -EPROBE_DEFER.
>>
>> Then, we have set w2sg_by_minor[minor] != NULL and the above test will already
>> find it allocated on next deferred probe attempt.
>>
>> So I'll move that to a position further down.
>>
>>>> +
>>>> + serdev_device_set_drvdata(serdev, data);
>>>> +
>>>> + data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
>>>> + "enable-gpios", 0,
>>>> + &flags);
>>>> + /* defer until we have all gpios */
>>>> + if (data->on_off_gpio == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
>>>> + "lna");
>>>> + if (IS_ERR(data->lna_regulator)) {
>>>> + /* defer until we can get the regulator */
>>>> + if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + data->lna_regulator = NULL;
>>>> + }
>>>> + pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
>>>> +
>>>> + data->lna_blocked = true;
>>>> + data->lna_is_off = true;
>>>> +
>>>> + data->is_on = false;
>>>> + data->requested = false;
>>>> + data->state = W2SG_IDLE;
>>>> + data->last_toggle = jiffies;
>>>> + data->backoff = HZ;
>>>> +
>>>> + data->uart = serdev;
>>>> +
>>>> + INIT_DELAYED_WORK(&data->work, toggle_work);
>>>> +
>>>> + err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
>>>> + "w2sg0004-on-off");
>>>> + if (err < 0)
>>>> + goto out;
>>>> +
>>>> + gpio_direction_output(data->on_off_gpio, false);
>>>> +
>>>> + serdev_device_set_client_ops(data->uart, &serdev_ops);
>>>> + serdev_device_open(data->uart);
>>>> +
>>>> + serdev_device_set_baudrate(data->uart, 9600);
>>>> + serdev_device_set_flow_control(data->uart, false);
>>>> +
>>>> + rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
>>>> + &w2sg0004_rfkill_ops, data);
>>>> + if (rf_kill == NULL) {
>>>> + err = -ENOMEM;
>>>> + goto err_rfkill;
>>>> + }
>>>> +
>>>> + err = rfkill_register(rf_kill);
>>>> + if (err) {
>>>> + dev_err(&serdev->dev, "Cannot register rfkill device\n");
>>>> + goto err_rfkill;
>>>> + }
>>>> +
>>>> + data->rf_kill = rf_kill;
>>>> +
>>>> + /* allocate the tty driver */
>>>> + data->tty_drv = alloc_tty_driver(1);
>>>> + if (!data->tty_drv)
>>>> + return -ENOMEM;
>>>> +
>>>> + /* 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);
>>>> +
>>>> + /* 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;
>>
>> A test setting this to NULL leads to a kernel oops in:
>>
>> [ 3048.821258] [<c046c598>] (tty_port_open) from [<c0466328>] (tty_open+0x1e8/0x2d8)
>>
>> So we must define this completely empty struct w2sg_port_ops
>> and pass a reference.
>>
>>>> +
>>>> + pr_debug("w2sg call tty_port_register_device\n");
>>>> +
>>>> + data->dev = tty_port_register_device(&data->port,
>>>> + data->tty_drv, minor, &serdev->dev);
>>>> +
>>>> + pr_debug("w2sg probed\n");
>>>> +
>>>> + /* keep off until user space requests the device */
>>>> + w2sg_set_power(data, false);
>>>> +
>>>> + return 0;
>>>> +
>>>> +err_rfkill:
>>>> + rfkill_destroy(rf_kill);
>>>> + serdev_device_close(data->uart);
>>>> +out:
>>>> + return err;
>>>> +}
>>>> +
>>>> +static void w2sg_remove(struct serdev_device *serdev)
>>>> +{
>>>> + struct w2sg_data *data = serdev_device_get_drvdata(serdev);
>>>> + int minor;
>>>> +
>>>> + cancel_delayed_work_sync(&data->work);
>>>> +
>>>> + /* what is the right sequence to avoid problems? */
>>>> + serdev_device_close(data->uart);
>>>> +
>>>> + minor = 0;
>>>> + tty_unregister_device(data->tty_drv, minor);
>>>> +
>>>> + tty_unregister_driver(data->tty_drv);
>>>> +}
>>>> +
>>>> +static int w2sg_suspend(struct device *dev)
>>>
>>> __maybe_unused, or if PM is disabled you will get a warning.
>>
>> Ok.
>>
>>>
>>>> +{
>>>> + struct w2sg_data *data = dev_get_drvdata(dev);
>>>> +
>>>> + data->suspended = true;
>>>> +
>>>> + cancel_delayed_work_sync(&data->work);
>>>> +
>>>> + w2sg_set_lna_power(data); /* shuts down if needed */
>>>> +
>>>> + if (data->state == W2SG_PULSE) {
>>>> + msleep(10);
>>>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>>>> + data->last_toggle = jiffies;
>>>> + data->is_on = !data->is_on;
>>>> + data->state = W2SG_NOPULSE;
>>>> + }
>>>> +
>>>> + if (data->state == W2SG_NOPULSE) {
>>>> + msleep(10);
>>>> + data->state = W2SG_IDLE;
>>>> + }
>>>> +
>>>> + if (data->is_on) {
>>>> + pr_info("GPS off for suspend %d %d %d\n", data->requested,
>>>> + data->is_on, data->lna_is_off);
>>>> +
>>>> + gpio_set_value_cansleep(data->on_off_gpio, 0);
>>>> + msleep(10);
>>>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>>>> + data->is_on = 0;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int w2sg_resume(struct device *dev)
>>>> +{
>>>> + struct w2sg_data *data = dev_get_drvdata(dev);
>>>> +
>>>> + data->suspended = false;
>>>> +
>>>> + pr_info("GPS resuming %d %d %d\n", data->requested,
>>>> + data->is_on, data->lna_is_off);
>>>> +
>>>> + schedule_delayed_work(&data->work, 0); /* enables LNA if needed */
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id w2sg0004_of_match[] = {
>>>> + { .compatible = "wi2wi,w2sg0004" },
>>>> + { .compatible = "wi2wi,w2sg0084" },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
>>>> +
>>>> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
>>>> +
>>>> +static struct serdev_device_driver w2sg_driver = {
>>>> + .probe = w2sg_probe,
>>>> + .remove = w2sg_remove,
>>>> + .driver = {
>>>> + .name = "w2sg0004",
>>>> + .owner = THIS_MODULE,
>>>> + .pm = &w2sg_pm_ops,
>>>> + .of_match_table = of_match_ptr(w2sg0004_of_match)
>>>> + },
>>>> +};
>>>> +
>>>> +module_serdev_device_driver(w2sg_driver);
>>>> +
>>>> +MODULE_ALIAS("w2sg0004");
>>>
>>> Is this needed?
>>
>> Seems to be redundant.
>> After removal the driver is still loaded automatically after boot:
>>
>> root at letux:~# modprobe -c | fgrep w2sg
>> alias of:N*T*Cwi2wi,w2sg0004 w2sg0004
>> alias of:N*T*Cwi2wi,w2sg0004C* w2sg0004
>> alias of:N*T*Cwi2wi,w2sg0084 w2sg0004
>> alias of:N*T*Cwi2wi,w2sg0084C* w2sg0004
>> root at letux:~# lsmod | fgrep w2sg
>> w2sg0004 16384 2
>> root at letux:~#
>>
>>>
>>>> +
>>>> +MODULE_AUTHOR("NeilBrown <neilb at suse.de>");
>>>
>>> Who really wrote this?
>>
>> Good question.
>>
>> The problem is that with cleaning up the code and rewriting history over such
>> a long time, it is no more visible.
>>
>> But I have searched through our older branches:
>>
>> Neil Brown had developed the first version for v3.7 in 2013:
>> http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=5b6fad7c7f15db8bb8e2c98ae9a50da52bda8b69
>>
>> This is where the MODULE_AUTHOR line comes from.
>>
>> Later, Marek Belisko worked on the UART driver, pinmux and interrupts.
>>
>> And I have
>> * added lna-regulator and rfkill ca. v3.12
>> * added device tree support ca. v3.15
>> * moved to drivers/misc
>> * ported the code to serdev API ca. v4.11
>> * submitted to LKML and worked in comments by reviewers ca. 4.15
>>
>> So this is the first version that is close to be acceptable.
>>
>> Neil had also submitted different versions to LKML but I am not
>> sure if he still is active anywhere.
>>
>> I have also checked a diff between the v3.7 version and the
>> current one and there are ca. 30-40% of lines original code by
>> Neil.
>>
>> So I'd say:
>>
>> * Neil is the original author and a significant amount of untouched code lines and comments are still there
>> * he designed the overall driver architecture
>> * Hence he is the copyright holder, did license under GPL v2 and we have just made a derivative work out of it
>> * Neil did submit his version of serdev ca. 2015 (quite different from this) but resigned after review feedback
>> * I did add some important features but not the core code and driver architecture
>> * I feel my role as maintainer and massaging everything for DT, serdev and get it upstream, but not "the author"
>>
>> How should we best reflect this in the AUTHOR macro?
>>
>
> Easy:
>
> MODULE_AUTHOR("NeilBrown <neilb at suse.de>");
> MODULE_AUTHOR("H. Nikolaus Schaller <hns at goldelico.com>");
>
> you can have more than one :)
Ah, that is much easier to solve than I had thought...
>
>>>
>>>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
[PATCH v5] now coming.
BR and thanks,
Nikolaus
More information about the linux-arm-kernel
mailing list