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

[Andreas Kemnade]

On Wed, 5 Dec 2018 16:01:16 +0100
Johan Hovold <johan@xxxxxxxxxx> wrote:

> On Sun, Nov 18, 2018 at 10:57:58PM +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
> > 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 <neilb@xxxxxxx> and  H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> > ---
> >  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.

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

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

> > +		data->active = 1;  
> 
> active is bool, so use "true".
> 
> > +		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.

> > +	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.
 
> > +		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.
> > +
> > +	/* 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.
> > +	serdev_device_close(data->serdev);
> > +	return 0;
> >  }
> >  

[...] more minor style issues
> 
> > +	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.

> >  
> > -	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

Attachment: pgpoTB2nIxjcg.pgp
Description: OpenPGP digital signature