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:
pgp7gDG1uKJh1.pgp
Description: OpenPGP digital signature
