Re: [PATCH 2/5] gnss: sirf: power on logic for devices without wakeup signal

On Wed, 5 Dec 2018 16:01:16 +0100
Johan Hovold [off-list ref] wrote:

On Sun, Nov 18, 2018 at 10:57:58PM +0100, Andreas Kemnade wrote:

quoted

Some Wi2Wi devices do not have a wakeup output, so device state can
only be indirectly detected by looking whether there is communitcation
over the serial lines.
Additionally it checks for the initial state of the device during
probing to ensure it is off.
Timeout values need to be increased, because the reaction on serial line
is slower and are in line  with previous patches by
Neil Brown [off-list ref] and  H. Nikolaus Schaller [off-list ref].

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/gnss/sirf.c | 97 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 32 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index b5efbb062316..6a0e5c0a2d62 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c

@@ -22,8 +22,8 @@
 
 #define SIRF_BOOT_DELAY			500
 #define SIRF_ON_OFF_PULSE_TIME		100
-#define SIRF_ACTIVATE_TIMEOUT		200
-#define SIRF_HIBERNATE_TIMEOUT		200
+#define SIRF_ACTIVATE_TIMEOUT		1000
+#define SIRF_HIBERNATE_TIMEOUT		1000  

We shouldn't increase the timeouts for the general case where we have
wakeup connected.

Well, in most times they are not hit in the general case, only once
if the internal state is not in sync.
But I can add a second pair of defines with more refined defines.

quoted

 struct sirf_data {
 	struct gnss_device *gdev;

@@ -45,26 +45,14 @@ static int sirf_open(struct gnss_device *gdev)
 	int ret;
 
 	data->opened = true;
-	ret = serdev_device_open(serdev);
-	if (ret)
-		return ret;
-
-	serdev_device_set_baudrate(serdev, data->speed);
-	serdev_device_set_flow_control(serdev, false);  

And also here, I think we shouldn't change the general case (wakeup
connected) unnecessarily. Currently user space can request the receiver
to remain powered, while not keeping the port open unnecessarily.

Yes, that usecase makes sense. There is even no need to keep that
device opened in the no-wakeup case. If I just open the serdev
during state change, code will probably be cleaner.

quoted

 
 	ret = pm_runtime_get_sync(&serdev->dev);
 	if (ret < 0) {
 		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
 		pm_runtime_put_noidle(&serdev->dev);
 		data->opened = false;
-		goto err_close;
 	}
 
-	return 0;
-
-err_close:
-	serdev_device_close(serdev);
-
 	return ret;
 }
 

@@ -73,8 +61,6 @@ static void sirf_close(struct gnss_device *gdev)
 	struct sirf_data *data = gnss_get_drvdata(gdev);
 	struct serdev_device *serdev = data->serdev;
 
-	serdev_device_close(serdev);
-
 	pm_runtime_put(&serdev->dev);
 	data->opened = false;
 }

@@ -109,6 +95,11 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 	struct sirf_data *data = serdev_device_get_drvdata(serdev);
 	struct gnss_device *gdev = data->gdev;
 
+	if ((!data->wakeup) && (!data->active)) {  

You have superfluous parenthesis like the above throughout the series.

OK, will reduce them.

quoted

+		data->active = 1;  

active is bool, so use "true".

quoted

+		wake_up_interruptible(&data->power_wait);
+	}
+
 	/*
 	 * we might come here everytime when runtime is resumed
 	 * and data is received. Two cases are possible

@@ -149,6 +140,25 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
 {
 	int ret;
 
+	/* no wakeup pin, success condition is that
+	 * no byte comes in in the period
+	 */  

Multiline comment style needs to be fixed throughout. Also use sentence
case and periods where appropriate.

OK. maybe I did believe too much in checkpatch.pl. It likes this patch.
I thought it would moan about such basic things.

quoted

+	if ((!data->wakeup) && (!active) && (data->active)) {
+		/* some bytes might come, so sleep a bit first */
+		msleep(timeout);  

This changes the semantics of the functions and effectively doubles the
requested timeout.

So maybe I should sort this block out into a properly-named function
with properly named constants?
The logic is to give the device some time first to calm down. And then
check for some time if it is really down.
 

quoted

+		data->active = false;
+		ret = wait_event_interruptible_timeout(data->power_wait,
+			data->active == true, msecs_to_jiffies(timeout));
+
+		if (ret < 0)
+			return ret;
+
+		/* we are still getting woken up -> timeout */
+		if (ret > 0)
+			return -ETIMEDOUT;
+		return 0;
+	}
+
 	ret = wait_event_interruptible_timeout(data->power_wait,
 			data->active == active, msecs_to_jiffies(timeout));
 	if (ret < 0)

@@ -203,21 +213,48 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 static int sirf_runtime_suspend(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret;
 
 	if (!data->on_off)
 		return regulator_disable(data->vcc);  

[..] minor  style issues ... will fix, still wondering
why checkpatch does not complain. Just saved the patch again  and
checked.

quoted

+
+	/* we should close it anyways, so the following receptions
+	 * will not run into the empty
+	 */  

Not sure what you mean here, please rephrase.

If the serdev is closed, nothing will be sent to a probably
not-existing-anymore gnss device.

quoted

+	serdev_device_close(data->serdev);
+	return 0;
 }
 

[...] more minor style issues

quoted

+	ret = sirf_set_active(data, true);
+
+	if (!ret)
+		return 0;  

Add an error label instead, and return 0 unconditionally in the success
path.

Ok, makes sense.

quoted

 
-	return sirf_set_active(data, true);
+	if (!data->on_off)
+		regulator_disable(data->vcc);
+err_close_serdev:
+	serdev_device_close(data->serdev);
+	return ret;
 }
 
 static int __maybe_unused sirf_suspend(struct device *dev)

@@ -311,18 +348,6 @@ static int sirf_probe(struct serdev_device *serdev)
 	if (data->on_off) {
 		data->wakeup = devm_gpiod_get_optional(dev, "sirf,wakeup",
 				GPIOD_IN);
-		if (IS_ERR(data->wakeup))
-			goto err_put_device;  

You still want to check for errors here.

Yes, I should only ignore NULL here..

Regards,
Andreas