Date:   Wed, 29 Nov 2017 08:47:31 -0600
From:   "Andrew F. Davis" <afd@...com>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
CC:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Benoît Cousson <bcousson@...libre.com>,
        Tony Lindgren <tony@...mide.com>,
        Russell King <linux@...linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kevin Hilman <khilman@...libre.com>,
        Andreas Färber <afaerber@...e.de>,
        Thierry Reding <treding@...dia.com>,
        Jonathan Cameron <jic23@...nel.org>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-omap <linux-omap@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, <kernel@...a-handheld.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Marek Belisko <marek@...delico.com>, Neil Brown <neilb@...e.de>
Subject: Re: [PATCH v4 3/5] misc serdev: Add w2sg0004 (gps receiver) power
 control driver

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@...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@...e.de>
>>> but simplified and adapted to use the new serdev API introduced in 4.11.
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.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.

>>> 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@...e.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.

>>
>>> + * 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@...ux:~# 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@...ux:~# lsmod | fgrep w2sg
> w2sg0004               16384  2 
> root@...ux:~# 
> 
>>
>>> +
>>> +MODULE_AUTHOR("NeilBrown <neilb@...e.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@...e.de>");
MODULE_AUTHOR("H. Nikolaus Schaller <hns@...delico.com>");

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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists