Re: [PATCH v2 09/16] Bluetooth: Prepare for full support discovery procedures

Hi Andre,

<snip>

quoted

quoted

diff --git a/include/net/bluetooth/hci_core.h b/include/net/ 

bluetooth/hci_core.h
index 1ff59f2..0d2e703 100644

--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h

@@ -597,6 +597,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);

#define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
#define lmp_ssp_capable(dev)       ((dev)->features[6] &  
LMP_SIMPLE_PAIR)
#define lmp_no_flush_capable(dev)  ((dev)->features[6] &  
LMP_NO_FLUSH)
+#define lmp_bredr_capable(dev)     (!((dev)->features[4] &  
LMP_NO_BREDR))

I don't think this is a good idea. You keep forgetting if you actually
have LE switched on or not.

I think we should keep it like this and just keep a global hci_dev  
state
which discovery procedure to use. Depending on if the device is just
really LE-Only, it is dual-stack, but LE got switched off (we will  
need
this eventually for testing) or it is just only BR/EDR.

I agree. What really matters for the discovery procedure is the
operation mode not the device type. The term "device type" was
misused here.

About the global hci_dev state, IMO we may not need it. By looking
at the controller's LMP features we are able to infer what is the
controller's operation mode.

you need to look at the extended features page 1, but yes, if you have
that available, the it might be as simple as have the global state.

It really depends on how often you have to check. Maybe it is worth to
just have a simple operation mode variable compared to always have to
access multiple bits in the features array. Depends on how often you
need to use it.

quoted

quoted

#define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)

/* ----- Extended LMP capabilities ----- */

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index bbb0daa..dcfb466 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c

@@ -32,6 +32,15 @@

#define MGMT_VERSION	0
#define MGMT_REVISION	1

+enum bt_device_type {
+	BREDR_ONLY,
+	LE_ONLY,
+	BREDR_LE,
+	UNKNOWN,
+};

What is this for? We essentially have a local device capabilities  
and an
operation mode. They are both different. We do not have a device type.

I'll change this. I may call this bt_operation_mode or something.

Call it bt_opermode or similar. Something shorter than some long crazy
name that takes a lot of space at least ;)

Also remove the UNKNOWN thing. That is useless. The devices is either
BREDR or LE_ONLY anyway. And worst case it is really BREDR. That has
been the default for a long time now.

And while at it just call it BREDR. And prefix this with
BT_OPERMODE_BREDR or so.

Regards

Marcel