Hi, "Felipe F. Tonello" <e...@felipetonello.com> 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 <e...@felipetonello.com>
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
signature.asc
Description: PGP signature
