On Wed, 5 Dec 2018 16:01:16 +0100 Johan Hovold 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 and H. Nikolaus Schaller . > > > > Signed-off-by: Andreas Kemnade > > --- > > 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