Re: [PATCH v4 2/5] gnss: sirf: add support for configurations without wakeup signal

[Johan Hovold <johan@kernel.org>]

On Thu, Jan 24, 2019 at 07:34:36AM +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 communication
> over the serial lines.
> This approach requires a report cycle set to a value less than 2 seconds
> to be reliable.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v4:
>  - was 3/6 earlier
>  - more cleanup
>  - separate no wakeup version for sirf_wait_for_power_state
>  - cleaned up optimisation for initial power off
> 
> Changes in v3:
>  - was 2/5 earlier
>  - changed commit headline
>  - more style cleanup
>  - split out initial power off as 2/6
>  - introduced SIRF_REPORT_CYCLE constant
>  - added documentation about limitations
>  - ignore first data after power state on so no
>    shutdown meassages are treated as power on success
>  - clearer logic in sirf_wait_for_power_state
> 
> Changes in v2:
>  - style cleanup
>  - do not keep serdev open just because runtime is active,
>    only when needed (gnss device is opened or state is changed)
>  - clearer timeout semantics
> 
>  drivers/gnss/sirf.c | 124 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 19 deletions(-)

>  static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
>  					unsigned long timeout)
>  {
>  	int ret;
>  
> +	if (!data->wakeup)
> +		return sirf_wait_for_power_state_nowakeup(data, active,
> +							  timeout);
> +
>  	ret = wait_event_interruptible_timeout(data->power_wait,
>  			data->active == active, msecs_to_jiffies(timeout));
>  	if (ret < 0)
> @@ -195,6 +267,12 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  	else
>  		timeout = SIRF_HIBERNATE_TIMEOUT;
>  
> +	if (!data->wakeup) {
> +		ret = sirf_serdev_open(data);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	do {
>  		sirf_pulse_on_off(data);
>  		ret = sirf_wait_for_power_state(data, active, timeout);
> @@ -202,12 +280,17 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  			if (ret == -ETIMEDOUT)
>  				continue;
>  
> -			return ret;
>  		}
> -
>  		break;
> +
>  	} while (retries--);

I noticed there were some odd white-space changes here after you
reverted the modified error handling from v3 which initially looked a
little out of place to me.

I decided to add those changed back in instead after looking at the end
result and noticing that the (retries < 0) check below also becomes
superfluous.

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

Series now applied, good job!

Johan