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

[Andreas Kemnade]

On Mon, 14 Jan 2019 11:51:29 +0100
Johan Hovold <johan@xxxxxxxxxx> wrote:

> On Thu, Jan 10, 2019 at 11:02:23PM +0100, Andreas Kemnade wrote:
> > Hi Johan,
> > 
> > On Thu, 10 Jan 2019 13:10:38 +0100
> > Johan Hovold <johan@xxxxxxxxxx> wrote:  
> 
> > > On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:  
> 
> > > > Additionally it checks for the initial state of the device during
> > > > probing to ensure it is off.    
> > > 
> > > Is this really needed? If so, it should probably go in a separate patch.
> > >   
> > Well, it is the main motivation for the new try of upstreaming this again.
> > You know the loooong history...
> > It has several times messed up my power consumption statistics. As I try
> > to test patches on top of mainline, this has often led to false alarms
> > regarding pm bugs in other drivers.
> > 
> > We could also come from another kernel here via kexec having the
> > device in another state. 
> > 
> > And why we do not want to check for uncommon things here? We e.g. do
> > multiple tries for changing power state.   
> 
> You still need to argue why it is needed (e.g. the kexec case) and that
> needs to go in the commit message of a separate patch adding something
> like that as it is orthogonal to supporting configurations without
> wakeup.
> 
yes, totally agreed, there is already a separate patch with an extensive
commit message.

> This may also be better handled by a shutdown() callback which is where
> such kexec concerns are supposed to be handled, and that would also take
> care of the reboot case. This way, not everyone has to pay a penalty on
> every boot for the arguable rare use case of kexec.
> 
there are more possibilities than kexec.

> > GPS chips will have usually some boot messages. So it is not the
> > "send nmea data set every X seconds" for the wakeup case, it is
> > another situation deserving IMHO another name.  
> 
> Ok, but unless all supported (sirf-star-based) chips have boot messages,
> we'd still need to wait that long for wakeup.
> 
I have never seen one without. But that could also be attached
to the dtb compatible name or a separate property.

> Are these messages you refer to output also on wake from hibernate, and
> not just on boot?
> 
also wake from hibernate.
I mean
$PSRF150,1*3E


> > > In pseudo code we have:
> > > 
> > >   activate:
> > >    - toggle on-off
> > >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> > >      - reception: success   
> > 
> > Note: we can also get the goodbye/shutdown message from the chip here
> > so there are chances of a false success, but since we initially power down,
> > we will rule out wrong state here.  
> 
> Good point. Unless we know the current state, we'd need to sleep for
> HIBERNATE_TIMEOUT before waiting for data reception.
> 
> > >      - timeout: failure
> > > 
> > >   hibernate:
> > >    - toggle on-off
> > >    - sleep(HIBERNATE_TIMEOUT)  
> > we could also additionally check here for 
> >    if (last_bytes_received == GOODBYE_MSG)  
> 
> Caching and parsing the stream for this could get messy. And is the
> expected message clearly defined somewhere, or would it be device (and
> firmware) dependent?
> 
I think so but I must check.
$PSRF150.0

But as said, these ideas are be for a possibly later patchset.

Regards,
Andreas

Attachment: pgpcKJZE5ICjj.pgp
Description: OpenPGP digital signature