On Wed, Jan 16, 2019 at 10:18:09PM +0100, Andreas Kemnade wrote:

Some Wi2Wi devices do not have a wakeup output, so device state can
only be indirectly detected by looking whether there is communitcation

communication

over the serial lines.
This approach requires a report cycle set to a low value to be reliable.

How low? Better to spell out your current assumptions so people can
configure accordingly.

"we assume that the chip is off"?

+ * REVISIT: The report cycle is configurable and can be several minutes long,
+ * so this will only work reliably if the report cycle is set to a reasonable
+ * low value. Also power saving settings (like send data only on movement)
+ * might things work even worse.
+ * Workaround might be to parse shutdown or bootup messages.
+ * This problem mainly makes error checking uncertain.

I'd drop the last sentence. You could also fail to detect the state and
waste power indefinitely, right?

Lockdep is not a problem; a possible deadlock is. Just drop this comment.

+	 */
+	struct mutex serdev_mutex;
+	int serdev_count;
+
 	wait_queue_head_t power_wait;
 };
 
+static int sirf_serdev_open(struct sirf_data *data)
+{
+	int ret = 0;
+
+	mutex_lock(&data->serdev_mutex);
+	if (++data->serdev_count == 1) {
+		ret = serdev_device_open(data->serdev);
+		if (ret) {
+			data->serdev_count--;
+			mutex_unlock(&data->serdev_mutex);

Use a common "out_unlock:" path below.

+			return ret;
+		}
+
+		serdev_device_set_baudrate(data->serdev, data->speed);
+		serdev_device_set_flow_control(data->serdev, false);
+	}
+	mutex_unlock(&data->serdev_mutex);
+
+	return ret;
+}

You currently don't share any code with the wakeup-case; better to keep
this in a separate helper if so.

+		/* Wait for boot or shutdown messages */

	/* Wait for state change (including any shutdown messages). */

+		msleep(timeout);
+		data->active = false;
+		/* Now check if it is really off. */

You now also use this code to check for activate state:

	/* Wait for data reception or timeout. */

+		ret = wait_event_interruptible_timeout(data->power_wait,
+					data->active,
+					msecs_to_jiffies(SIRF_REPORT_CYCLE));
+		if (ret < 0)
+			return ret;
+
+		if ((ret > 0 && !active) ||
+		    (ret == 0 && active))

Split these cases in two add add comments (failed suspend and failed
resume) for readability.

Special casing like this only hurts readability. This is better handled
as a I did for the wakeup case in the series I just posted, that is, by
making sure the receiver is hibernated in probe. You just need to add a
helper to determine the state for no-wakeup.

+
 		ret = sirf_wait_for_power_state(data, active, timeout);
-		if (ret < 0) {
-			if (ret == -ETIMEDOUT)
-				continue;
+	} while (ret == -ETIMEDOUT && retries--);

Why change this?

 
-			return ret;

A break should do right?

-		}
+	if (!data->wakeup)
+		sirf_serdev_close(data);
 
-		break;
-	} while (retries--);
+	if (ret)
+		return ret;
 
 	if (retries < 0)
 		return -ETIMEDOUT;

Johan