Hi, "Felipe F. Tonello" <eu@xxxxxxxxxxxxxxxxx> writes: > [ text/plain ] > This refactor results in a cleaner state machine code and promotes > consistency, readability, and maintanability of this driver. > > Signed-off-by: Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx> while working on this driver ... > --- > 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) ... could you get rid of the userspace types (as a separate patch, of course) ? > { > - uint8_t p0 = port->cable << 4; > + uint8_t p[4] = { port->cable << 4, 0, 0, 0 }; > + uint8_t next_state = STATE_INITIAL; and here too. I know you're going for consistency, but maybe it makes sense to clean up the types before you cleanup the state machine. [...] > + /* 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; okay, so this function will be called from ->complete() which is called without controller lock held but with IRQs disabled. I wonder if you're racing port->state here with this change. Could race on SMP, no ? -- balbi
Attachment:
signature.asc
Description: PGP signature