Re: [PATCH v4 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver
From: Andrew F. Davis
Date: Wed Nov 29 2017 - 09:50:00 EST
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@xxxxxx>:
>>
>> 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@xxxxxxx>
>>> but simplified and adapted to use the new serdev API introduced in 4.11.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>>> ---
>>> 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.
>>> 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.
>>
>>> +/*
>>> + * 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@xxxxxxx>");
>>> +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.
>>
>>> + * 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.
>>
>>> +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.
> 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.
>>
>>> +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@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@letux:~# lsmod | fgrep w2sg
> w2sg0004 16384 2
> root@letux:~#
>
>>
>>> +
>>> +MODULE_AUTHOR("NeilBrown <neilb@xxxxxxx>");
>>
>> 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@xxxxxxx>");
MODULE_AUTHOR("H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>");
you can have more than one :)
>>
>>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>
>
> BR and thanks,
> Nikolaus Schaller
>
> --
> 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
>