PATCH hid2hci for CSR 8510 A10

PATCH hid2hci for CSR 8510 A10

[Gordon]

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

Hi,

ich had a problem with the Sitecom CNT-524 as described by someone else
here:
http://blog.ruecker.fi/2013/10/06/adventures-in-bluetooth-4-0-part-i/

so i tried to fix that by introducing --mode csr2 for hid2hci

I neither know how to switch back to hid nor how to detect the EALREADY
state, otherwise the patch is very small.

I works on my machine in ubuntu 13.04 with bluez 4.101-0ubuntu8b1 and
said sitecom usb dongle. the patch is against bluez commit
fd00064e0bb2c81e53e9d0b7d22ce919b41dbe60

Could someone please review.

Cheers,

Gordon



[-- Attachment #2: bluez_hid2hci.patch --]
[-- Type: text/x-patch, Size: 2471 bytes --]

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index 95b4abf..514accc 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -118,6 +118,41 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err = 0;
+	struct usbfs_disconnect disconnect;
+	char report[] = { 0x1 , 0x5 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 };
+	switch (mode) {
+	case HCI:
+		//send report as is
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+						strerror(errno), errno);
+			return -1;
+		}
+
+		err = control_message(fd, USB_ENDPOINT_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+					USB_REQ_SET_CONFIGURATION, //Set_Report Request
+					0x01 | (0x03 << 8), //report id: 0x01, report type: feature (0x03)
+					0, //interface
+					report, sizeof(report), 5000);
+		//unable to detect EALREADY
+		break;
+	case HID:
+		// currently unknown what to do here
+		fprintf(stderr, "csr2: Switching to hid mode is not implemented\n");
+		return -1;
+		break;
+	}
+
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -257,7 +292,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -310,6 +345,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {

Re: PATCH hid2hci for CSR 8510 A10

[Gordon]

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

Hi,

some time ago I sent a patch attached to following mail.

On Mo, 2013-12-30 at 22:59 +0100, Gordon wrote:
> Hi,
> 
> ich had a problem with the Sitecom CNT-524 as described by someone else
> here:
> http://blog.ruecker.fi/2013/10/06/adventures-in-bluetooth-4-0-part-i/
> 
> so i tried to fix that by introducing --mode csr2 for hid2hci
> 
> I neither know how to switch back to hid nor how to detect the EALREADY
> state, otherwise the patch is very small.
> 
> I works on my machine in ubuntu 13.04 with bluez 4.101-0ubuntu8b1 and
> said sitecom usb dongle. the patch is against bluez commit
> fd00064e0bb2c81e53e9d0b7d22ce919b41dbe60
> 
> Could someone please review.

I didn't get a reply yet. Since it has been my first mail to this list I
wonder whether there is anything wrong with this request, or just nobody
is interested ?


Cheers,

Gordon


[-- Attachment #2: bluez_hid2hci.patch --]
[-- Type: text/x-patch, Size: 2471 bytes --]

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index 95b4abf..514accc 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -118,6 +118,41 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err = 0;
+	struct usbfs_disconnect disconnect;
+	char report[] = { 0x1 , 0x5 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 };
+	switch (mode) {
+	case HCI:
+		//send report as is
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+						strerror(errno), errno);
+			return -1;
+		}
+
+		err = control_message(fd, USB_ENDPOINT_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+					USB_REQ_SET_CONFIGURATION, //Set_Report Request
+					0x01 | (0x03 << 8), //report id: 0x01, report type: feature (0x03)
+					0, //interface
+					report, sizeof(report), 5000);
+		//unable to detect EALREADY
+		break;
+	case HID:
+		// currently unknown what to do here
+		fprintf(stderr, "csr2: Switching to hid mode is not implemented\n");
+		return -1;
+		break;
+	}
+
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -257,7 +292,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -310,6 +345,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {

Re: PATCH hid2hci for CSR 8510 A10

[Johan Hedberg]

Hi Gordon,

On Fri, Mar 28, 2014, Gordon wrote:
> some time ago I sent a patch attached to following mail.
> 
> On Mo, 2013-12-30 at 22:59 +0100, Gordon wrote:
> > Hi,
> > 
> > ich had a problem with the Sitecom CNT-524 as described by someone else
> > here:
> > http://blog.ruecker.fi/2013/10/06/adventures-in-bluetooth-4-0-part-i/
> > 
> > so i tried to fix that by introducing --mode csr2 for hid2hci
> > 
> > I neither know how to switch back to hid nor how to detect the EALREADY
> > state, otherwise the patch is very small.
> > 
> > I works on my machine in ubuntu 13.04 with bluez 4.101-0ubuntu8b1 and
> > said sitecom usb dongle. the patch is against bluez commit
> > fd00064e0bb2c81e53e9d0b7d22ce919b41dbe60
> > 
> > Could someone please review.
> 
> I didn't get a reply yet. Since it has been my first mail to this list I
> wonder whether there is anything wrong with this request, or just nobody
> is interested ?

A couple of things regarding the presentation of the patch should at
least be fixed:

 - Send it as a proper git patch, i.e. using git commit, format-patch
   and send-email
 - Try to follow the bluez.git coding style which is roughly the same as
   the Linux kernel coding style. I realize the rest of hid2hci.c is
   probably the worst reference for good BlueZ coding style (being one
   of the oldest pieces of code with very little maintenance over the
   years).

A couple of things that could be improved coding style-wise:

> +static int usb_switch_csr2(int fd, enum mode mode)
> +{
> +	int err = 0;

Does this really need to be initialized to 0 upon declaration? Doing
this prevents the compiler from pointing out places where we should be
setting the variable to something else but dot (i.e.. in the case of
errors).

> +	struct usbfs_disconnect disconnect;
> +	char report[] = { 0x1 , 0x5 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 , 0x0 };

This should be an unsigned integer, uint8_t is probably best choice.
Spell out the individual values with two digits to make clear they are
bytes and not 4-bit values.


> +	switch (mode) {
> +	case HCI:
> +		//send report as is
> +		disconnect.interface = 0;
> +		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
> +		strcpy(disconnect.driver, "usbfs");
> +
> +		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
> +			fprintf(stderr, "Can't claim interface: %s (%d)\n",
> +						strerror(errno), errno);
> +			return -1;
> +		}
> +
> +		err = control_message(fd, USB_ENDPOINT_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +					USB_REQ_SET_CONFIGURATION, //Set_Report Request
> +					0x01 | (0x03 << 8), //report id: 0x01, report type: feature (0x03)
> +					0, //interface
> +					report, sizeof(report), 5000);

Looks like you're hitting the 80-character line limits here. I know
other places in hid2hci.c violate against it but it'd be nice to at
least follow the rule for new code.

> +		//unable to detect EALREADY

We don't generally use C++ style comments in the tree. Prefer /* .. */
where possible. In this case I'd elaborate the comment a bit since I
don't quite understand what it means.

> +	case HID:
> +		// currently unknown what to do here

Same here.

Johan

Re: PATCH hid2hci for CSR 8510 A10

[Gordon]

Hi Johan,

On Fr, 2014-03-28 at 21:49 +0200, Johan Hedberg wrote:
> Hi Gordon,
> 
> On Fri, Mar 28, 2014, Gordon wrote:
> > some time ago I sent a patch attached to following mail.
> > 
> > On Mo, 2013-12-30 at 22:59 +0100, Gordon wrote:
> > > Hi,
> > > 
> > > Could someone please review.
> > 
> > I didn't get a reply yet. Since it has been my first mail to this list I
> > wonder whether there is anything wrong with this request, or just nobody
> > is interested ?
> 
> A couple of things regarding the presentation of the patch should at
> least be fixed:
> 
>  - Send it as a proper git patch, i.e. using git commit, format-patch
>    and send-email

ok, will try.

>  - Try to follow the bluez.git coding style which is roughly the same as
>    the Linux kernel coding style. I realize the rest of hid2hci.c is
>    probably the worst reference for good BlueZ coding style (being one
>    of the oldest pieces of code with very little maintenance over the
>    years).
> 

thanks for the detailed remarks, although removed from the reply, I will
address them.

> > +		//unable to detect EALREADY
> 
> We don't generally use C++ style comments in the tree. Prefer /* .. */
> where possible. In this case I'd elaborate the comment a bit since I
> don't quite understand what it means.

I am not sure, other functions in hid2hci presumably return this when
the device is already in the mode that should be switched to. 

> 
> > +	case HID:
> > +		// currently unknown what to do here
> 

the method for switching back to hid mode is unknown, should I remove
the cases or just be more verbose in the comments ?

Thanks,

Gordon

Re: PATCH hid2hci for CSR 8510 A10

[Gordon]

second try fo this patch, with remarks from Johan Hedberg addressed.
code style checked with linux's scripts/Lindent.


Gordon

[PATCH] hid2hci add CSR 8510 A10 support

[Gordon]

add a new method "csr2" to hid2hci that sends a control message
to the usb device which switches the Cambridge Silicon Radio 8510 A10
as used in Sitecom CNT-524

fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
---
 tools/hid2hci.1 |  2 +-
 tools/hid2hci.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index 2dbfca7..1f64b41 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -119,6 +119,49 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err;
+	struct usbfs_disconnect disconnect;
+	uint8_t report[] = {
+		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+	};
+	switch (mode) {
+	case HCI:
+		/* send report as is */
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+				strerror(errno), errno);
+			return -1;
+		}
+		/* Set_report request with 
+		 * report id: 0x01, report type: feature (0x03) 
+		 * on interface 0
+		 */
+		err = control_message(fd,
+				      USB_ENDPOINT_OUT | USB_TYPE_CLASS |
+				      USB_RECIP_INTERFACE,
+				      USB_REQ_SET_CONFIGURATION,
+				      0x01 | (0x03 << 8),
+				      0, report, sizeof(report), 5000);
+		/* unable to detect whether the previous state 
+		 * already was HCI (EALREADY)  
+		 */
+		break;
+	case HID:
+		/* currently unknown how to switch to HID */
+		fprintf(stderr,
+			"csr2: Switching to hid mode is not implemented\n");
+		return -1;
+		break;
+	}
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -258,7 +301,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -311,6 +354,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {
-- 
1.8.3.2

Re: [PATCH] hid2hci add CSR 8510 A10 support

[Anderson Lizardo]

Hi Gordon,

A few more coding style suggestions below.

On Sat, Mar 29, 2014 at 7:37 AM, Gordon <kernel@gkr.i24.cc> wrote:
> diff --git a/tools/hid2hci.c b/tools/hid2hci.c
> index 2dbfca7..1f64b41 100644
> --- a/tools/hid2hci.c
> +++ b/tools/hid2hci.c
> @@ -119,6 +119,49 @@ static int usb_switch_csr(int fd, enum mode mode)
>         return err;
>  }
>
> +static int usb_switch_csr2(int fd, enum mode mode)
> +{
> +       int err;
> +       struct usbfs_disconnect disconnect;
> +       uint8_t report[] = {
> +               0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +       };

Add an empty line here to separate variable declarations.

> +       switch (mode) {
> +       case HCI:
> +               /* send report as is */
> +               disconnect.interface = 0;
> +               disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
> +               strcpy(disconnect.driver, "usbfs");
> +
> +               if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
> +                       fprintf(stderr, "Can't claim interface: %s (%d)\n",
> +                               strerror(errno), errno);
> +                       return -1;
> +               }

Add an empty line here.

> +               /* Set_report request with
> +                * report id: 0x01, report type: feature (0x03)
> +                * on interface 0
> +                */
> +               err = control_message(fd,
> +                                     USB_ENDPOINT_OUT | USB_TYPE_CLASS |
> +                                     USB_RECIP_INTERFACE,
> +                                     USB_REQ_SET_CONFIGURATION,
> +                                     0x01 | (0x03 << 8),
> +                                     0, report, sizeof(report), 5000);
> +               /* unable to detect whether the previous state
> +                * already was HCI (EALREADY)
> +                */
> +               break;
> +       case HID:
> +               /* currently unknown how to switch to HID */
> +               fprintf(stderr,
> +                       "csr2: Switching to hid mode is not implemented\n");
> +               return -1;
> +               break;

Instead of "return -1; break;"  you can do:

err = -1;
break;

> +       }

Add an empty line here to separate the "return err" from the rest of the code.

> +       return err;
> +}
> +
>  static int hid_logitech_send_report(int fd, const char *buf, size_t size)
>  {
>         struct hiddev_report_info rinfo;

Best Regards,
-- 
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil

[PATCH v3] hid2hci add CSR 8510 A10 support

[Gordon]

add a new method "csr2" to hid2hci that sends a control message
to the usb device which switches the Cambridge Silicon Radio 8510 A10
as used in Sitecom CNT-524

fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
---

Notes:
    3rd version, coding style corrections from Johan Hedberg and Anderson Lizardo

 tools/hid2hci.1 |  2 +-
 tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index 2dbfca7..a83bace 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -119,6 +119,52 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err;
+	struct usbfs_disconnect disconnect;
+	uint8_t report[] = {
+		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+	};
+
+	switch (mode) {
+	case HCI:
+		/* send report as is */
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+				strerror(errno), errno);
+			return -1;
+		}
+
+		/* Set_report request with 
+		 * report id: 0x01, report type: feature (0x03) 
+		 * on interface 0
+		 */
+		err = control_message(fd,
+				      USB_ENDPOINT_OUT | USB_TYPE_CLASS |
+				      USB_RECIP_INTERFACE,
+				      USB_REQ_SET_CONFIGURATION,
+				      0x01 | (0x03 << 8),
+				      0, report, sizeof(report), 5000);
+		/* unable to detect whether the previous state 
+		 * already was HCI (EALREADY)  
+		 */
+		break;
+	case HID:
+		/* currently unknown how to switch to HID */
+		fprintf(stderr,
+			"csr2: Switching to hid mode is not implemented\n");
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -258,7 +304,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -311,6 +357,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {
-- 
1.8.3.2

Re: [PATCH v3] hid2hci add CSR 8510 A10 support

[Gordon]

Hi,

is there a reason this is not yet in bluez ?

http://article.gmane.org/gmane.linux.bluez.kernel/46985

Gordon

Re: [PATCH v3] hid2hci add CSR 8510 A10 support

[Johan Hedberg]

Hi Gordon,

On Wed, Jan 07, 2015, Gordon wrote:
> is there a reason this is not yet in bluez ?
> 
> http://article.gmane.org/gmane.linux.bluez.kernel/46985

Probably because it's simply been forgotten about (feel free to send a
reminder email if a patch hasn't been responded to in a week or so).
However, before applying this I'd appreciate if you could re-send it
with your full name in the author information. We generally prefer not
to include anonymous commits in the tree.

Johan

[PATCH v4] hid2hci add CSR 8510 A10 support

[kernel]

From: Gordon Kramer <kernel@gkr.i24.cc>

add a new method "csr2" to hid2hci that sends a control message
to the usb device which switches the Cambridge Silicon Radio 8510 A10
as used in Sitecom CNT-524

fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
---

Notes:
    4th version, coding style corrections from Johan Hedberg and Anderson Lizardo, resend

 tools/hid2hci.1 |  2 +-
 tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index a183bfa..49dcada 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -119,6 +119,52 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err;
+	struct usbfs_disconnect disconnect;
+	uint8_t report[] = {
+		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+	};
+
+	switch (mode) {
+	case HCI:
+		/* send report as is */
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+				strerror(errno), errno);
+			return -1;
+		}
+
+		/* Set_report request with 
+		 * report id: 0x01, report type: feature (0x03) 
+		 * on interface 0
+		 */
+		err = control_message(fd,
+				      USB_ENDPOINT_OUT | USB_TYPE_CLASS |
+				      USB_RECIP_INTERFACE,
+				      USB_REQ_SET_CONFIGURATION,
+				      0x01 | (0x03 << 8),
+				      0, report, sizeof(report), 5000);
+		/* unable to detect whether the previous state 
+		 * already was HCI (EALREADY)  
+		 */
+		break;
+	case HID:
+		/* currently unknown how to switch to HID */
+		fprintf(stderr,
+			"csr2: Switching to hid mode is not implemented\n");
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -258,7 +304,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -311,6 +357,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {
-- 
1.9.1

Re: [PATCH v4] hid2hci add CSR 8510 A10 support

[Johan Hedberg]

Hi Gordon,

On Wed, Jan 07, 2015, kernel@gkr.i24.cc wrote:
> add a new method "csr2" to hid2hci that sends a control message
> to the usb device which switches the Cambridge Silicon Radio 8510 A10
> as used in Sitecom CNT-524
> 
> fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
> ---
> 
> Notes:
>     4th version, coding style corrections from Johan Hedberg and Anderson Lizardo, resend
> 
>  tools/hid2hci.1 |  2 +-
>  tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 2 deletions(-)

I was about to apply this but it doesn't compile:

make --no-print-directory all-am
  CC       tools/hid2hci.o
tools/hid2hci.c: In function ‘usb_switch_csr2’:
tools/hid2hci.c:152:14: error: pointer targets in passing argument 6 of ‘control_message’ differ in signedness [-Werror=pointer-sign]
           0, report, sizeof(report), 5000);
              ^
tools/hid2hci.c:78:12: note: expected ‘char *’ but argument is of type ‘uint8_t *’
 static int control_message(int fd, int requesttype, int request,
            ^
tools/hid2hci.c:124:6: error: ‘err’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  int err;
      ^
cc1: all warnings being treated as errors

Please use ./configure --enable-maintainer-mode to verify that the build
is clean (or ./bootstrap-configure && make).

> +	uint8_t report[] = {
> +		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +	};

Minor improvement: you might want to declare this as const.

Johan

Re: [PATCH v4] hid2hci add CSR 8510 A10 support

[Gordon]

Am Mittwoch, 7. Januar 2015, 22:31:59 schrieb Johan Hedberg:
> Hi Gordon,
> 
> On Wed, Jan 07, 2015, kernel@gkr.i24.cc wrote:
> > add a new method "csr2" to hid2hci that sends a control message
> > to the usb device which switches the Cambridge Silicon Radio 8510 A10
> > as used in Sitecom CNT-524
> > 
> > fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
> > ---
> > 
> > Notes:
> >     4th version, coding style corrections from Johan Hedberg and Anderson
> >     Lizardo, resend>  
> >  tools/hid2hci.1 |  2 +-
> >  tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> I was about to apply this but it doesn't compile:
> 

sorry, i didn't test after rebasing, last year the compiler wasn't 
complaining, and now i even need a new glib. is there a recommended developer 
platform/OS distribution ?

Thanks,

Gordon

Re: [PATCH v4] hid2hci add CSR 8510 A10 support

[Gordon]

Hi Johan,

forget my earlier mail, this was due to using a different system.

Am Mittwoch, 7. Januar 2015, 22:31:59 schrieb Johan Hedberg:
> Hi Gordon,
> 
> On Wed, Jan 07, 2015, kernel@gkr.i24.cc wrote:
> > add a new method "csr2" to hid2hci that sends a control message
> > to the usb device which switches the Cambridge Silicon Radio 8510 A10
> > as used in Sitecom CNT-524
> > 
> > fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
> > ---
> > 
> > Notes:
> >     4th version, coding style corrections from Johan Hedberg and Anderson
> >     Lizardo, resend>  
> >  tools/hid2hci.1 |  2 +-
> >  tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> I was about to apply this but it doesn't compile:
> 
> make --no-print-directory all-am
>   CC       tools/hid2hci.o
> tools/hid2hci.c: In function ‘usb_switch_csr2’:
> tools/hid2hci.c:152:14: error: pointer targets in passing argument 6 of
> ‘control_message’ differ in signedness [-Werror=pointer-sign] 0, report,
> sizeof(report), 5000);
>               ^
> tools/hid2hci.c:78:12: note: expected ‘char *’ but argument is of type
> ‘uint8_t *’ static int control_message(int fd, int requesttype, int
> request,
>             ^

what do you prefer? char * or uint_8 * in the function declaration ? when 
switching to unit_8 there will be changes in other functions as well.


> tools/hid2hci.c:124:6: error: ‘err’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized] int err;
>       ^
> cc1: all warnings being treated as errors

ok easy

> 
> Please use ./configure --enable-maintainer-mode to verify that the build
> is clean (or ./bootstrap-configure && make).
> 
> > +	uint8_t report[] = {
> > +		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> > +	};
> 
> Minor improvement: you might want to declare this as const.

const also means a change to the method declaration or an extra cast. What do 
you prefer ?

Thanks,

Gordon

[PATCH v5 0/2] hid2hci add CSR 8510 A10 support

[kernel]

From: Gordon Kramer <kernel@gkr.i24.cc>

Hi Johan,

fixed the compilation bug. using following config
configure --enable-maintainer-mode --disable-systemd --disable-cups  --disable-monitor

i also only compile with
make tools/hid2hci

added a second patch that implements your suggestions with const uint_8 * 

Thanks!

Gordon Kramer (2):
  hid2hci add CSR 8510 A10 support
  use const uint_8* for pointer in control_message

 tools/hid2hci.1 |  2 +-
 tools/hid2hci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 54 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH v5 1/2] hid2hci add CSR 8510 A10 support

[kernel]

From: Gordon Kramer <kernel@gkr.i24.cc>

add a new method "csr2" to hid2hci that sends a control message
to the usb device which switches the Cambridge Silicon Radio 8510 A10
as used in Sitecom CNT-524

fixes: https://bugzilla.kernel.org/show_bug.cgi?id=69181
---

Notes:
    5th version, coding style corrections from Johan Hedberg and Anderson Lizardo, compilation fix

 tools/hid2hci.1 |  2 +-
 tools/hid2hci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/tools/hid2hci.1 b/tools/hid2hci.1
index 8c5d520..c6876a3 100644
--- a/tools/hid2hci.1
+++ b/tools/hid2hci.1
@@ -32,7 +32,7 @@ mode and back.
 .B --mode= [hid, hci]
 Sets the mode to switch the device into
 .TP
-.B --method= [csr, logitech-hid, dell]
+.B --method= [csr, csr2, logitech-hid, dell]
 Which vendor method to use for switching the device.
 .TP
 .B --devpath=
diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index a183bfa..3eedf4e 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -119,6 +119,52 @@ static int usb_switch_csr(int fd, enum mode mode)
 	return err;
 }
 
+static int usb_switch_csr2(int fd, enum mode mode)
+{
+	int err = 0;
+	struct usbfs_disconnect disconnect;
+	char report[] = {
+		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+	};
+
+	switch (mode) {
+	case HCI:
+		/* send report as is */
+		disconnect.interface = 0;
+		disconnect.flags = USBFS_DISCONNECT_EXCEPT_DRIVER;
+		strcpy(disconnect.driver, "usbfs");
+
+		if (ioctl(fd, USBFS_IOCTL_DISCONNECT, &disconnect) < 0) {
+			fprintf(stderr, "Can't claim interface: %s (%d)\n",
+				strerror(errno), errno);
+			return -1;
+		}
+
+		/* Set_report request with 
+		 * report id: 0x01, report type: feature (0x03) 
+		 * on interface 0
+		 */
+		err = control_message(fd,
+				      USB_ENDPOINT_OUT | USB_TYPE_CLASS |
+				      USB_RECIP_INTERFACE,
+				      USB_REQ_SET_CONFIGURATION,
+				      0x01 | (0x03 << 8),
+				      0, report, sizeof(report), 5000);
+		/* unable to detect whether the previous state 
+		 * already was HCI (EALREADY)  
+		 */
+		break;
+	case HID:
+		/* currently unknown how to switch to HID */
+		fprintf(stderr,
+			"csr2: Switching to hid mode is not implemented\n");
+		err = -1;
+		break;
+	}
+
+	return err;
+}
+
 static int hid_logitech_send_report(int fd, const char *buf, size_t size)
 {
 	struct hiddev_report_info rinfo;
@@ -258,7 +304,7 @@ static void usage(const char *error)
 	printf("Usage: hid2hci [options]\n"
 		"  --mode=       mode to switch to [hid|hci] (default hci)\n"
 		"  --devpath=    sys device path\n"
-		"  --method=     method to use to switch [csr|logitech-hid|dell]\n"
+		"  --method=     method to use to switch [csr|csr2|logitech-hid|dell]\n"
 		"  --help\n\n");
 }
 
@@ -311,6 +357,9 @@ int main(int argc, char *argv[])
 			if (!strcmp(optarg, "csr")) {
 				method = METHOD_CSR;
 				usb_switch = usb_switch_csr;
+			} else if (!strcmp(optarg, "csr2")) {
+				method = METHOD_CSR;
+				usb_switch = usb_switch_csr2;
 			} else if (!strcmp(optarg, "logitech-hid")) {
 				method = METHOD_LOGITECH_HID;
 			} else if (!strcmp(optarg, "dell")) {
-- 
1.9.1

[PATCH v5 2/2] use const uint_8* for pointer in control_message

[kernel]

From: Gordon Kramer <kernel@gkr.i24.cc>

change control_message signature
requires const void* usbfs_ctrltransfer for ioctl
use uint8 * in usb_switch_dell
---
 tools/hid2hci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/hid2hci.c b/tools/hid2hci.c
index 3eedf4e..033fcfd 100644
--- a/tools/hid2hci.c
+++ b/tools/hid2hci.c
@@ -59,7 +59,7 @@ struct usbfs_ctrltransfer {
 	uint16_t wIndex;
 	uint16_t wLength;
 	uint32_t timeout;	/* in milliseconds */
-	void *data;		/* pointer to data */
+	const void *data;	/* pointer to data */
 };
 
 
@@ -77,7 +77,7 @@ struct usbfs_disconnect{
 
 static int control_message(int fd, int requesttype, int request,
 					int value, int index,
-					char *bytes, int size, int timeout)
+					const uint8_t *bytes, int size, int timeout)
 {
 	struct usbfs_ctrltransfer transfer;
 
@@ -123,7 +123,7 @@ static int usb_switch_csr2(int fd, enum mode mode)
 {
 	int err = 0;
 	struct usbfs_disconnect disconnect;
-	char report[] = {
+	const uint8_t report[] = {
 		0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 	};
 
@@ -226,7 +226,7 @@ out:
 
 static int usb_switch_dell(int fd, enum mode mode)
 {
-	char report[] = { 0x7f, 0x00, 0x00, 0x00 };
+	uint8_t report[] = { 0x7f, 0x00, 0x00, 0x00 };
 	struct usbfs_disconnect disconnect;
 	int err;
 
-- 
1.9.1

Re: [PATCH v5 0/2] hid2hci add CSR 8510 A10 support

[Johan Hedberg]

Hi Gordon,

On Wed, Jan 07, 2015, kernel@gkr.i24.cc wrote:
> fixed the compilation bug. using following config
> configure --enable-maintainer-mode --disable-systemd --disable-cups  --disable-monitor
> 
> i also only compile with
> make tools/hid2hci
> 
> added a second patch that implements your suggestions with const uint_8 * 
> 
> Thanks!
> 
> Gordon Kramer (2):
>   hid2hci add CSR 8510 A10 support
>   use const uint_8* for pointer in control_message
> 
>  tools/hid2hci.1 |  2 +-
>  tools/hid2hci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 54 insertions(+), 5 deletions(-)

I applied both of these patches now, however the first one still wasn't
completely clean and needed auto-fixing from git:

Applying: hid2hci add CSR 8510 A10 support
/home/jh/src/bluez/.git/rebase-apply/patch:52: trailing whitespace.
		/* Set_report request with 
/home/jh/src/bluez/.git/rebase-apply/patch:53: trailing whitespace.
		 * report id: 0x01, report type: feature (0x03) 
/home/jh/src/bluez/.git/rebase-apply/patch:62: trailing whitespace.
		/* unable to detect whether the previous state 
/home/jh/src/bluez/.git/rebase-apply/patch:63: trailing whitespace.
		 * already was HCI (EALREADY)  
warning: 4 lines applied after fixing whitespace errors.

Johan