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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Marcel and Ilya,

On 13/06/2015 16:10, Ilya Faenson wrote:
Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx]
Sent: Saturday, June 13, 2015 3:54 AM
To: Ilya Faenson
Cc: linux-bluetooth@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxx Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux