Hi,

On Thu, Dec 11, 2014 at 11:13:07PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > h4p changes uart speed again after load of the firmware, but I guess
> > > that's ok.
> 
> >  if you can do it the other way around it would result in a faster
> > init. Depending on how many patches are actually required to be
> > loaded.
> 
> IIRC driver does two speed changes, so it looks to me like someone
> already tried that (and it did not work).

maybe. maybe not. Should be easy to test it (again) and add a
comment, shouldn't it?

> > >> What needs to be done is the bring up of the device including the proper 
> > >> UART settings and speed and then just run the firmware downloads. All 
> > >> firmware files on the Nokia devices where just HCI commands with vendor 
> > >> specific details. Some from CSR, some from Broadcom and some from TI. 
> > >> You can actually decode them if you really want to. Shouldn't be that 
> > >> hard.
> > >> 
> > > 
> > > Speed changes at the end of firmware load, but I guess that's detail?
> > > Anyway, patch would look like this.
> > 
> > You should really look into providing hdev->setup() callback. That is 
> > normally the callback where you want to load the firmware.
> > 
> 
> I can provide setup() callback and load firmware from there.
> 
> I have created provisional device tree binding, and the driver still
> works.

I don't have time to look at the code now, but I have some comments
regarding the binding.

> Some time ago you mentioned that with the "big" issues fixed, you'd be
> willing to take it into the tree. What way forward do you see? Would
> it make sense to re-enable the driver in staging, so that "big"
> changes could be applied, followed by renames?
> 
> Thanks,
>                                                               Pavel
> 
> diff --git a/arch/arm/boot/dts/omap3-n900.dts 
> b/arch/arm/boot/dts/omap3-n900.dts
> index 9e0e5a2..201f21b 100644
> --- a/arch/arm/boot/dts/omap3-n900.dts
> +++ b/arch/arm/boot/dts/omap3-n900.dts
> @@ -790,9 +776,21 @@
>  };
>  
>  &uart2 {
> +        compatible = "brcm,uart,bcm2048";

This does not look correct. The uart should not be overwritten. The
current h4p driver indeed implements a driver for the serial port,
but that's a) linux specific and does not belong in the DT and b)
should probably be changed in the mainline kernel.

>       interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>;
>       pinctrl-names = "default";
>       pinctrl-0 = <&uart2_pins>;
> +     device {
> +               compatible = "brcm,bcm2048";
> +               uart = <&uart2>;

You don't need a phandle to the parent device.

> +               reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */
> +               host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 
> */

The host-wakeup should be mapped as irq, gpio2irq conversion
will happen ;)

> +               bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* want 
> 37 */

To be consistent with the n900 DTS file you should probably drop
"want " from the comments.

> +               chip-type = <3>;

This should be set in the driver based on the compatible
value and not via DT data.

> +               clocks = <&uart2_fck>, <&uart2_ick>;
> +               clock-names = "fck", "ick";

These clocks you defined belong to the uart device and not to the
uart slave (bluetooth) device.

> +               bt-sysclk = <2>;

I think this should be mapped cleanly in DT by adding a new clock
to the DTS file:

vctcxo_clock: clock  {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <38400000>;
};

Then the bluetooth device can reference its clock device:

clocks = <&vctcxo_clock>;

The same clock reference should be added to the wl1251 DT node :)

> ... [code] ...

-- Sebastian

Attachment: signature.asc
Description: Digital signature

Reply via email to