Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1348422 > unrolled thread

[PATCH 0/5] MIDI USB Gadget improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] MIDI USB Gadget improvements "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
    [PATCH 5/5] usb: gadget: f_midi: updated copyright "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
      Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100
    [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:40 +0100
      Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100
    [PATCH 1/5] usb: gadget: f_midi: refactor state machine "Felipe F. Tonello" <eu@felipetonello.com> - 2016-03-02 20:50 +0100
      Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-02 22:20 +0100
        Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-03 10:00 +0100
          Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Clemens Ladisch <clemens@ladisch.de> - 2016-03-03 12:40 +0100
            Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine Felipe Ferreri Tonello <eu@felipetonello.com> - 2016-03-03 17:30 +0100
    Re: [PATCH 0/5] MIDI USB Gadget improvements Felipe Balbi <balbi@kernel.org> - 2016-03-04 08:20 +0100

#1348422 — [PATCH 0/5] MIDI USB Gadget improvements

Patches are pretty much self-described.

Patch 1 is revised from comments.
Patch 2 is a bug fix.

Felipe F. Tonello (5):
  usb: gadget: f_midi: refactor state machine
  usb: gadget: f_midi: added spinlock on transmit function
  usb: gadget: gmidi: remove bus powered requirement on bmAttributes
  usb: gadget: f_midi: cleanups and typos fixes
  usb: gadget: f_midi: updated copyright

 drivers/usb/gadget/function/f_midi.c | 293 ++++++++++++++++++++++-------------
 drivers/usb/gadget/legacy/gmidi.c    |  14 +-
 2 files changed, 193 insertions(+), 114 deletions(-)

-- 
2.7.2

[toc] | [next] | [standalone]

#1348425 — [PATCH 5/5] usb: gadget: f_midi: updated copyright

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 9a9e6112e224..5c7f5c780fda 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -5,6 +5,9 @@
  * Developed for Thumtronics by Grey Innovation
  * Ben Williamson <ben.williamson@greyinnovation.com>
  *
+ * Copyright (C) 2015,2016 ROLI Ltd.
+ * Felipe F. Tonello <felipe.tonello@roli.com>
+ *
  * Rewritten for the composite framework
  *   Copyright (C) 2011 Daniel Mack <zonque@gmail.com>
  *
-- 
2.7.2

[toc] | [prev] | [next] | [standalone]

#1349908 — Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright

[Multipart message — attachments visible in raw view] — view raw

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

no commit log == no commit

> ---
>  drivers/usb/gadget/function/f_midi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 9a9e6112e224..5c7f5c780fda 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -5,6 +5,9 @@
>   * Developed for Thumtronics by Grey Innovation
>   * Ben Williamson <ben.williamson@greyinnovation.com>
>   *
> + * Copyright (C) 2015,2016 ROLI Ltd.
> + * Felipe F. Tonello <felipe.tonello@roli.com>

Did you check with your company's lawyer that your changes are enough to
justify a copyright ?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1348426 — [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 77 +++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 8475e3dc82d4..9a9e6112e224 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1,5 +1,5 @@
 /*
- * f_midi.c -- USB MIDI class function driver
+ * f_midi.c -- USB-MIDI class function driver
  *
  * Copyright (C) 2006 Thumtronics Pty Ltd.
  * Developed for Thumtronics by Grey Innovation
@@ -16,7 +16,7 @@
  *   Copyright (C) 2006 Thumtronics Pty Ltd.
  *   Ben Williamson <ben.williamson@greyinnovation.com>
  *
- * Licensed under the GPL-2 or later.
+ * Licensed under the GPLv2.
  */
 
 #include <linux/kernel.h>
@@ -41,8 +41,8 @@
 MODULE_AUTHOR("Ben Williamson");
 MODULE_LICENSE("GPL v2");
 
-static const char f_midi_shortname[] = "f_midi";
-static const char f_midi_longname[] = "MIDI Gadget";
+static const char f_midi_shortname[] =	"f_midi";
+static const char f_midi_longname[] =	"MIDI Gadget";
 
 /*
  * We can only handle 16 cables on one single endpoint, as cable numbers are
@@ -78,28 +78,31 @@ struct gmidi_in_port {
 };
 
 struct f_midi {
-	struct usb_function	func;
-	struct usb_gadget	*gadget;
-	struct usb_ep		*in_ep, *out_ep;
-	struct snd_card		*card;
-	struct snd_rawmidi	*rmidi;
-	u8			ms_id;
-
-	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
-
-	unsigned long		out_triggered;
-	struct tasklet_struct	tasklet;
+	struct usb_function func;
+	struct usb_gadget *gadget;
+	struct usb_ep *in_ep, *out_ep;
+	u8 ms_id;
+	unsigned long out_triggered;
 	unsigned int in_ports;
 	unsigned int out_ports;
-	int index;
-	char *id;
-	unsigned int buflen, qlen;
+	unsigned int buflen;
+	unsigned int qlen;
+	unsigned int len;
+
 	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
 	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
 	spinlock_t transmit_lock;
+
+	/* ALSA stuff */
+	struct snd_card *card;
+	struct snd_rawmidi *rmidi;
+	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
+	struct tasklet_struct tasklet;
 	unsigned int in_last_port;
+	int index;
+	char *id;
 
-	struct gmidi_in_port	in_ports_array[/* in_ports */];
+	struct gmidi_in_port in_ports_array[/* in_ports */];
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16 ms_in_desc = {
 
 /* string IDs are assigned dynamically */
 
-#define STRING_FUNC_IDX			0
+#define STRING_FUNC_IDX 0
 
 static struct usb_string midi_string_defs[] = {
 	[STRING_FUNC_IDX].s = "MIDI function",
@@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
 };
 
 static struct usb_gadget_strings midi_stringtab = {
-	.language	= 0x0409,	/* en-us */
+	.language	= 0x0409, /* en-us */
 	.strings	= midi_string_defs,
 };
 
@@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device *device)
 }
 
 /*
- * Converts MIDI commands to USB MIDI packets.
+ * Converts MIDI commands to USB-MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 				 struct gmidi_in_port *port, uint8_t b)
@@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
 		in_emb->iJack			= 0;
 		midi_function[i++] = (struct usb_descriptor_header *) in_emb;
 
-		out_ext->bLength =		USB_DT_MIDI_OUT_SIZE(1);
-		out_ext->bDescriptorType =	USB_DT_CS_INTERFACE;
-		out_ext->bDescriptorSubtype =	USB_MS_MIDI_OUT_JACK;
-		out_ext->bJackType =		USB_MS_EXTERNAL;
-		out_ext->bJackID =		jack++;
-		out_ext->bNrInputPins =		1;
-		out_ext->iJack =		0;
-		out_ext->pins[0].baSourceID =	in_emb->bJackID;
-		out_ext->pins[0].baSourcePin =	1;
+		out_ext->bLength		= USB_DT_MIDI_OUT_SIZE(1);
+		out_ext->bDescriptorType	= USB_DT_CS_INTERFACE;
+		out_ext->bDescriptorSubtype	= USB_MS_MIDI_OUT_JACK;
+		out_ext->bJackType		= USB_MS_EXTERNAL;
+		out_ext->bJackID		= jack++;
+		out_ext->bNrInputPins		= 1;
+		out_ext->iJack			= 0;
+		out_ext->pins[0].baSourceID	= in_emb->bJackID;
+		out_ext->pins[0].baSourcePin	= 1;
 		midi_function[i++] = (struct usb_descriptor_header *) out_ext;
 
 		/* link it to the endpoint */
@@ -1251,12 +1254,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 		status = -ENOMEM;
 		goto setup_fail;
 	}
-	midi->in_ports = opts->in_ports;
-	midi->out_ports = opts->out_ports;
-	midi->index = opts->index;
-	midi->buflen = opts->buflen;
-	midi->qlen = opts->qlen;
-	midi->in_last_port = 0;
+	midi->in_ports		= opts->in_ports;
+	midi->out_ports		= opts->out_ports;
+	midi->index		= opts->index;
+	midi->buflen		= opts->buflen;
+	midi->qlen		= opts->qlen;
+	midi->in_last_port	= 0;
 
 	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
 	if (status)
-- 
2.7.2

[toc] | [prev] | [next] | [standalone]

#1349906 — Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Multipart message — attachments visible in raw view] — view raw

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

no commit log == no commit

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1348431 — [PATCH 1/5] usb: gadget: f_midi: refactor state machine

This refactor results in a cleaner state machine code and as a result fixed a
bug when packaging a USB-MIDI packet right after a non-conformant MIDI byte stream.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 204 ++++++++++++++++++++++-------------
 1 file changed, 129 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 84c0ee5ebd1e..3cdb0741f3f8 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+	STATE_INITIAL = 0,	/* pseudo state */
+	STATE_1PARAM,
+	STATE_2PARAM_1,
+	STATE_2PARAM_2,
+	STATE_SYSEX_0,
+	STATE_SYSEX_1,
+	STATE_SYSEX_2,
+	STATE_REAL_TIME,
+	STATE_FINISHED,		/* pseudo state */
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +73,6 @@ struct gmidi_in_port {
 	int active;
 	uint8_t cable;
 	uint8_t state;
-#define STATE_UNKNOWN	0
-#define STATE_1PARAM	1
-#define STATE_2PARAM_1	2
-#define STATE_2PARAM_2	3
-#define STATE_SYSEX_0	4
-#define STATE_SYSEX_1	5
-#define STATE_SYSEX_2	6
 	uint8_t data[2];
 };
 
@@ -400,118 +406,166 @@ static int f_midi_snd_free(struct snd_device *device)
 	return 0;
 }
 
-static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
-					uint8_t p1, uint8_t p2, uint8_t p3)
-{
-	unsigned length = req->length;
-	u8 *buf = (u8 *)req->buf + length;
-
-	buf[0] = p0;
-	buf[1] = p1;
-	buf[2] = p2;
-	buf[3] = p3;
-	req->length = length + 4;
-}
-
 /*
  * Converts MIDI commands to USB MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 				 struct gmidi_in_port *port, uint8_t b)
 {
-	uint8_t p0 = port->cable << 4;
+	uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+	uint8_t next_state = STATE_INITIAL;
+
+	switch (b) {
+	case 0xf8 ... 0xff:
+		/* System Real-Time Messages */
+		p[0] |= 0x0f;
+		p[1] = b;
+		next_state = port->state;
+		port->state = STATE_REAL_TIME;
+		break;
+
+	case 0xf7:
+		/* End of SysEx */
+		switch (port->state) {
+		case STATE_SYSEX_0:
+			p[0] |= 0x05;
+			p[1] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		case STATE_SYSEX_1:
+			p[0] |= 0x06;
+			p[1] = port->data[0];
+			p[2] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		case STATE_SYSEX_2:
+			p[0] |= 0x07;
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = 0xf7;
+			next_state = STATE_FINISHED;
+			break;
+		default:
+			/* Ignore byte */
+			next_state = port->state;
+			port->state = STATE_INITIAL;
+		}
+		break;
 
-	if (b >= 0xf8) {
-		f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-	} else if (b >= 0xf0) {
+	case 0xf0 ... 0xf6:
+		/* System Common Messages */
+		port->data[0] = port->data[1] = 0;
+		port->state = STATE_INITIAL;
 		switch (b) {
 		case 0xf0:
 			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
+			port->data[1] = 0;
+			next_state = STATE_SYSEX_1;
 			break;
 		case 0xf1:
 		case 0xf3:
 			port->data[0] = b;
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 			break;
 		case 0xf2:
 			port->data[0] = b;
-			port->state = STATE_2PARAM_1;
+			next_state = STATE_2PARAM_1;
 			break;
 		case 0xf4:
 		case 0xf5:
-			port->state = STATE_UNKNOWN;
+			next_state = STATE_INITIAL;
 			break;
 		case 0xf6:
-			f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-			port->state = STATE_UNKNOWN;
-			break;
-		case 0xf7:
-			switch (port->state) {
-			case STATE_SYSEX_0:
-				f_midi_transmit_packet(req,
-					p0 | 0x05, 0xf7, 0, 0);
-				break;
-			case STATE_SYSEX_1:
-				f_midi_transmit_packet(req,
-					p0 | 0x06, port->data[0], 0xf7, 0);
-				break;
-			case STATE_SYSEX_2:
-				f_midi_transmit_packet(req,
-					p0 | 0x07, port->data[0],
-					port->data[1], 0xf7);
-				break;
-			}
-			port->state = STATE_UNKNOWN;
+			p[0] |= 0x05;
+			p[1] = 0xf6;
+			next_state = STATE_FINISHED;
 			break;
 		}
-	} else if (b >= 0x80) {
+		break;
+
+	case 0x80 ... 0xef:
+		/*
+		 * Channel Voice Messages, Channel Mode Messages
+		 * and Control Change Messages.
+		 */
 		port->data[0] = b;
+		port->data[1] = 0;
+		port->state = STATE_INITIAL;
 		if (b >= 0xc0 && b <= 0xdf)
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 		else
-			port->state = STATE_2PARAM_1;
-	} else { /* b < 0x80 */
+			next_state = STATE_2PARAM_1;
+		break;
+
+	case 0x00 ... 0x7f:
+		/* Message parameters */
 		switch (port->state) {
 		case STATE_1PARAM:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-			} else {
-				p0 |= 0x02;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req, p0, port->data[0], b, 0);
+			if (port->data[0] < 0xf0)
+				p[0] |= port->data[0] >> 4;
+			else
+				p[0] |= 0x02;
+
+			p[1] = port->data[0];
+			p[2] = b;
+			/* This is to allow Running State Messages */
+			next_state = STATE_1PARAM;
 			break;
 		case STATE_2PARAM_1:
 			port->data[1] = b;
-			port->state = STATE_2PARAM_2;
+			next_state = STATE_2PARAM_2;
 			break;
 		case STATE_2PARAM_2:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-				port->state = STATE_2PARAM_1;
-			} else {
-				p0 |= 0x03;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req,
-				p0, port->data[0], port->data[1], b);
+			if (port->data[0] < 0xf0)
+				p[0] |= port->data[0] >> 4;
+			else
+				p[0] |= 0x03;
+
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = b;
+			/* This is to allow Running State Messages */
+			next_state = STATE_2PARAM_1;
 			break;
 		case STATE_SYSEX_0:
 			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
+			next_state = STATE_SYSEX_1;
 			break;
 		case STATE_SYSEX_1:
 			port->data[1] = b;
-			port->state = STATE_SYSEX_2;
+			next_state = STATE_SYSEX_2;
 			break;
 		case STATE_SYSEX_2:
-			f_midi_transmit_packet(req,
-				p0 | 0x04, port->data[0], port->data[1], b);
-			port->state = STATE_SYSEX_0;
+			p[0] |= 0x04;
+			p[1] = port->data[0];
+			p[2] = port->data[1];
+			p[3] = b;
+			next_state = STATE_SYSEX_0;
 			break;
 		}
+		break;
+	}
+
+	/* States where we have to write into the USB request */
+	if (next_state == STATE_FINISHED ||
+	    port->state == STATE_SYSEX_2 ||
+	    port->state == STATE_1PARAM ||
+	    port->state == STATE_2PARAM_2 ||
+	    port->state == STATE_REAL_TIME) {
+
+		unsigned length = req->length;
+		u8 *buf = (u8 *)req->buf + length;
+
+		memcpy(buf, p, sizeof(p));
+		req->length = length + sizeof(p);
+
+		if (next_state == STATE_FINISHED) {
+			next_state = STATE_INITIAL;
+			port->data[0] = port->data[1] = 0;
+		}
 	}
+
+	port->state = next_state;
 }
 
 static void f_midi_drop_out_substreams(struct f_midi *midi)
@@ -631,7 +685,7 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 	VDBG(midi, "%s()\n", __func__);
 	port = midi->in_ports_array + substream->number;
 	port->substream = substream;
-	port->state = STATE_UNKNOWN;
+	port->state = STATE_INITIAL;
 	return 0;
 }
 
-- 
2.7.2

[toc] | [prev] | [next] | [standalone]

#1348477 — Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code

It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.

> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.

I have been unable to determine where exactly the new code behaves
differently.  Can you show an example?


Regards,
Clemens

[toc] | [prev] | [next] | [standalone]

#1348918 — Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Multipart message — attachments visible in raw view] — view raw

Hi Clemens,

On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
> 
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.

Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.

I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.

> 
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
> 
> I have been unable to determine where exactly the new code behaves
> differently.  Can you show an example?

Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.

-- 
Felipe

[toc] | [prev] | [next] | [standalone]

#1349007 — Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.

I know, "clean" is subjective.  But in what way was the old state
machine not "proper"?

And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.


Regards,
Clemens

[toc] | [prev] | [next] | [standalone]

#1349337 — Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Multipart message — attachments visible in raw view] — view raw

Hi Clemens,

On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
> 
> I know, "clean" is subjective.

Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.

> But in what way was the old state
> machine not "proper"?

Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.

> 
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.

next_state is a transitional state, thus the temporal nature.

This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.

I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.

-- 
Felipe

[toc] | [prev] | [next] | [standalone]

#1349910

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Patches are pretty much self-described.
>
> Patch 1 is revised from comments.

you really need to describe what you changed. This also should have v2
on subject line.

I guess it's too late to get this in v4.6 merge window as I'm already
applying the last few patches and plan to send a pull request in a few
minutes.

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web