Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Frederic Danis <frederic.danis@linux.intel.com>]

Hello Marcel and Ilya,

On 13/06/2015 16:10, Ilya Faenson wrote:
> Hi Marcel,
>
> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Saturday, June 13, 2015 3:54 AM
> To: Ilya Faenson
> Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
> Subject: Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol
>
> Hi Ilya,
>
> >> Merge Broadcom protocol with the existing implementation, providing
> >> for UART setup, firmware download and power management.
> >>
> >> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> >> ---
> >> drivers/bluetooth/hci_bcm.c | 400 
> ++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 384 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> >> index e4d66b6..5ff35b7 100644
> >> --- a/drivers/bluetooth/hci_bcm.c
> >> +++ b/drivers/bluetooth/hci_bcm.c
> >> @@ -3,6 +3,7 @@
> >> *  Bluetooth HCI UART driver for Broadcom devices
> >> *
> >> *  Copyright (C) 2015  Intel Corporation
> >> + *  Copyright (C) 2015  Broadcom Corporation
> >> *
> >> *
> >> *  This program is free software; you can redistribute it and/or modify
> >> @@ -24,6 +25,7 @@
> >> #include <linux/kernel.h>
> >> #include <linux/errno.h>
> >> #include <linux/skbuff.h>
> >> +#include <linux/tty.h>
> >> #include <linux/firmware.h>
> >>
> >> #include <net/bluetooth/bluetooth.h>
> >> @@ -31,12 +33,170 @@
> >>
> >> #include "btbcm.h"
> >> #include "hci_uart.h"
> >> +#include "btbcm_uart.h"
> >>
> >> struct bcm_data {
> >>     struct sk_buff *rx_skb;
> >>     struct sk_buff_head txq;
> >> +    struct hci_uart *hu;
> >> +
> >> +    bool is_suspended; /* suspend/resume flag */
> >> +
> >> +    struct timer_list timer; /* idle timer */
> >
> > I prefer not to use timers if we can use delayed work instead.
> >
> > IF: Could you clarify how you want me to use the delayed work for 
> the frequently refreshed 5 second timer? It would essentially be an 
> extra thread running most of the time with us having to synchronize it 
> with the device stops etc. Clear invite for race conditions in my opinion.
>
> refreshed means re-armed? I wonder how delayed work is different then? 
> Maybe I need to understand a bit more than first.
>
> IF: Every time a new packet comes from either direction, the idle 
> timer needs to be re-armed to fire in 5 seconds regardless of the 
> current state of this timer. Hope that clarifies the logic.
>
> >
> >> +
> >> +    struct btbcm_uart_parameters pars; /* device parameters */
> >
> > btbcm_uart_params and params please. No pars and not point in 
> spelling out parameters in a struct name.
> >
> > IF: Okay.
> >
> >> +    void *device_context; /* ACPI/DT device context */
> >> };
> >>
> >> +/* Suspend/resume synchronization mutex */
> >> +static DEFINE_MUTEX(plock);
> >
> > Please use a more descriptive name than plock. That is too generic 
> for a global variable.
> >
> > IF: Okay.
> >
> >> +
> >> +/* Forward reference */
> >> +static struct hci_uart_proto bcm_proto;
> >
> > Why do we need this? I am generally not in favor of having forward 
> reference until really really needed.
> >
> > IF: That is needed to set the oper-speed in this structure once the 
> device parameters are available. Intel patch has hard-coded it to a 
> single value for all Broadcom devices which is not acceptable.
>
> So for Broadcom devices, I think in the static table, no oper-speed 
> should be set. That means it has to come by other means. Either ACPI 
> or DT. But as I mentioned in the other part, that table has to stay const.
>
> IF: The oper_speed comes from DT in my current code already. The BlueZ 
> line discipline currently acts on the oper_speed for all 
> manufacturers. Do you want an exception for Broadcom there? If not, 
> the oper-speed changes should apply to everybody.

<snip>
> >> -static const struct hci_uart_proto bcm_proto = {
> >> +/* This structure may not be const as speed may change at runtime */
> >
> > No. This struct is const. These are the value that a protocol driver 
> sets.
> >
> > If values are dynamic and change based on DT, then left them zero 
> here and provide a method for overwriting them internally in hci_uart. 
> Please do not hack this in. There are simple UART drivers that we want 
> to enable in a really simple way.
> >
> > IF: As I said above, I've inherited the need to change the speed(s) 
> from the recent Intel patches. Will try fixing that.
>
> I get where you are coming from. And for Broadcom the table will not 
> contain an oper-speed. That will come from ACPI or DT. So you action 
> should be to remove it here and introduce a variable in bcm_data that 
> has it and that can be used instead. I have not figured out all the 
> details, but that is the general idea. I leave the details up to you.
>
> IF: As I said above, this is slightly more complicated as the 
> oper_speed is used by the BlueZ line discipline for all manufacturers. 
> And any manufacturer with the oper-speed changeable among the devices 
> will be affected. Since the oper-speed used is largely defined by the 
> UART rather than BT device capabilities, I'm afraid it should be 
> configurable and not reside in a const structure for all.

FYI, the oper_speed is not available in ACPI table of T100.
Depending on device, even the init_speed may not be available.

So, I agree with Ilya these values should be configurable.

Regards

Fred

-- 
Frederic Danis Open Source Technology Center
frederic.danis@intel.com Intel Corporation