Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

From: Sebastian Reichel <sre@kernel.org>
To: "Pali Rohár" <pali.rohar@gmail.com>
Cc: "Marcel Holtmann" <marcel@holtmann.org>,
	"Tony Lindgren" <tony@atomide.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Jiri Slaby" <jslaby@suse.com>,
	"Ville Tervo" <ville.tervo@iki.fi>,
	"Filip Matijević" <filip.matijevic.pz@gmail.com>,
	"Aaro Koskinen" <aaro.koskinen@iki.fi>,
	"Pavel Machek" <pavel@ucw.cz>,
	ivo.g.dimitrov.75@gmail.com, linux-bluetooth@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-omap@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver
Date: Tue, 16 Aug 2016 11:25:48 +0200	[thread overview]
Message-ID: <20160816092548.swayvx4zu3vlg7as@earth> (raw)
In-Reply-To: <20160816075217.GK30047@pali>

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

Hi,

On Tue, Aug 16, 2016 at 09:52:17AM +0200, Pali Rohár wrote:
> > > +		case HCI_NOKIA_RADIO_PKT:
> > 
> > Are you sure you can ignore the RADIO_PKT commands. They are
> > used to set up the FM radio parts of the chip. They are standard
> > HCI commands (in the case of Broadcom at least). At minimum it
> > should be added a comment here that you are ignoring them on
> > purpose.
> > 
> > > +		case HCI_NOKIA_NEG_PKT:
> > > +		case HCI_NOKIA_ALIVE_PKT:
> > 
> > And here I would also a comment on why are we ignore these
> > commands and driving this all by ourselves.
> > 
> 
> Good question... In Pavel's version of bluetooth driver, which is
> working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
> step. It does not skip any packets, plus he added this comment:
> 
> /* Note that this is timing-critical. If sending packets takes
>  * too long, initialization will fail.
>  */
> 
> So really, can we skip those packets? And is not this reason why
> this bluetooth driver does not work on Nokia N900?

Let's have a look - here is Pavel's version:
https://lwn.net/Articles/627201/

In pseudocode:

while(true) {
    cmd = get_cmd_from_firmware();
    if (!cmd) break;
    __hci_cmd_sync(cmd);
}

This is not "whole firmware at one __hci_cmd_sync step", is it?
And obviously that wouldn't work. Next let's have a look at
"It does not skip any packets":

/* Skip first two packets */
if (++num <= 2)
    continue;

Which are HCI_NOKIA_NEG_PKT and HCI_NOKIA_ALIVE_PKT. Those are
open-coded. By using the packets from the firmware we could drop the
negotiation/alive functions from the driver and remove quite a few
lines of code. I think it should only be done after finding the N900
bug, though. I found it quite useful, that first communication does not
happen through the firmware file.

Surely the radio packet is not ignored, but that part is not used on
N950 and N900 fails at first packet, so no way to test the radio
packet handling. I probably should add a /* TODO: check how to
handle radio packets */ for the radio packet entry. Or implement it
the way Marcel suggested and hope that it just works once the other
bug is found.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]