[PATCH v5 0/2] Add generic serial MIDI driver using serial bus API

[PATCH v5 0/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

Generic serial MIDI driver adding support for using serial devices
compatible with the serial bus as raw MIDI devices, allowing using
additional serial devices not compatible with the existing
serial-u16550 driver. Supports only setting standard serial baudrates on
the underlying serial device; however, the underlying serial device can
be configured so that a requested 38.4 kBaud is actually the standard MIDI
31.25 kBaud. Supports DeviceTree configuration.

Changes in v5:
- Reword and add to description in dt-binding
- Change 'speed' dt property to 'current-speed'
- Move MIDI output loop onto workqueue (since this could loop quite a while,
    if ALSA provides a continuous stream of bytes)
- Add tx_state bit flags to snd_serial_generic struct
- Safegard critical section in tx_work with atomic bit ops on tx_state
- Switch operations on `filemode` to use atomic bit ops

Changes in v4:
- Fix regressed typo - Correct 3.84 kBaud -> 38.4 kBaud in DT & Kconfig
  (sorry about spam - noticed after sending v3 and didn't want to let
  the error sit around for too long)

Changes in v3:
- Replace use of snd_printk() with dev_* alternatives
- Removed unnecessary initialization of err variables
- Replaced instances of `== SERIAL_MODE_NOT_OPENED` with zero check
- Loop on output_write to completely fill output buffer if data available
- Depend on CONFIG_OF in Kconfig
- Replace use of devm_kzalloc() with extra_size allocation in snd_devm_card_new()
- Use module_serdev_device_driver() instead of module_init() and module_exit(0)

Changes in v2:
- Fix 'snd_serial_generic_write_wakeup' missing static keyword 
- Correct 3.125 kBaud > 31.25 kBaud in documentation for MIDI         


The need for this driver arose from a project using a Raspberry Pi4 which
needed to receive and send raw MIDI with low latency. The pl011 UART
used is not compatible with the existing serial MIDI driver made for
u16550-style devices. Using a userspace program such as ttymidi to feed
input from the TTY device to a virtual ALSA MIDI device was functional,
but not ideal.

I am not sure if a MIDI driver needing the mentioned 'hack' to clock
38.4 kBaud down to the standard MIDI baud is permissible in the mainline
kernel, but am submitting nevertheless in case it is useful. To my knowledge,
it doesn't seem that there would be any way for this driver to manually
configure a serial port to 31.25 kBaud using the serial bus API (please 
correct me f I'm wrong). In my use case, I am actually configuring one port
to run at 115.2 kBaud for faster communication with a custom onboard MIDI controller.


Daniel Kaehn (2):
  dt-bindings: sound: Add generic serial MIDI device
  Add generic serial MIDI driver using serial bus API

 .../devicetree/bindings/sound/serialmidi.yaml |  46 +++
 sound/drivers/Kconfig                         |  18 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 377 ++++++++++++++++++
 4 files changed, 443 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: 3e71713c9e75c34fc03f55ea86b381856ca952ee
-- 
2.33.0

[PATCH v5 0/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

Generic serial MIDI driver adding support for using serial devices
compatible with the serial bus as raw MIDI devices, allowing using
additional serial devices not compatible with the existing
serial-u16550 driver. Supports only setting standard serial baudrates on
the underlying serial device; however, the underlying serial device can
be configured so that a requested 38.4 kBaud is actually the standard MIDI
31.25 kBaud. Supports DeviceTree configuration.

Changes in v5:
- Reword and add to description in dt-binding
- Change 'speed' dt property to 'current-speed'
- Move MIDI output loop onto workqueue (since this could loop quite a while,
    if ALSA provides a continuous stream of bytes)
- Add tx_state bit flags to snd_serial_generic struct
- Safegard critical section in tx_work with atomic bit ops on tx_state
- Switch operations on `filemode` to use atomic bit ops

Changes in v4:
- Fix regressed typo - Correct 3.84 kBaud -> 38.4 kBaud in DT & Kconfig
  (sorry about spam - noticed after sending v3 and didn't want to let
  the error sit around for too long)

Changes in v3:
- Replace use of snd_printk() with dev_* alternatives
- Removed unnecessary initialization of err variables
- Replaced instances of `== SERIAL_MODE_NOT_OPENED` with zero check
- Loop on output_write to completely fill output buffer if data available
- Depend on CONFIG_OF in Kconfig
- Replace use of devm_kzalloc() with extra_size allocation in snd_devm_card_new()
- Use module_serdev_device_driver() instead of module_init() and module_exit(0)

Changes in v2:
- Fix 'snd_serial_generic_write_wakeup' missing static keyword 
- Correct 3.125 kBaud > 31.25 kBaud in documentation for MIDI         


The need for this driver arose from a project using a Raspberry Pi4 which
needed to receive and send raw MIDI with low latency. The pl011 UART
used is not compatible with the existing serial MIDI driver made for
u16550-style devices. Using a userspace program such as ttymidi to feed
input from the TTY device to a virtual ALSA MIDI device was functional,
but not ideal.

I am not sure if a MIDI driver needing the mentioned 'hack' to clock
38.4 kBaud down to the standard MIDI baud is permissible in the mainline
kernel, but am submitting nevertheless in case it is useful. To my knowledge,
it doesn't seem that there would be any way for this driver to manually
configure a serial port to 31.25 kBaud using the serial bus API (please 
correct me f I'm wrong). In my use case, I am actually configuring one port
to run at 115.2 kBaud for faster communication with a custom onboard MIDI controller.


Daniel Kaehn (2):
  dt-bindings: sound: Add generic serial MIDI device
  Add generic serial MIDI driver using serial bus API

 .../devicetree/bindings/sound/serialmidi.yaml |  46 +++
 sound/drivers/Kconfig                         |  18 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 377 ++++++++++++++++++
 4 files changed, 443 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: 3e71713c9e75c34fc03f55ea86b381856ca952ee
-- 
2.33.0

[PATCH v5 1/2] dt-bindings: sound: Add generic serial MIDI device

[Daniel Kaehn]

Adds dt-binding for a Generic MIDI Interface using a serial device.

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 .../devicetree/bindings/sound/serialmidi.yaml | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml

diff --git a/Documentation/devicetree/bindings/sound/serialmidi.yaml b/Documentation/devicetree/bindings/sound/serialmidi.yaml
new file mode 100644
index 000000000000..06a894e1b91d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/serialmidi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic Serial MIDI Interface
+
+maintainers:
+  - Daniel Kaehn <kaehndan@gmail.com>
+
+description: 
+  Generic MIDI interface using a serial device. This denotes that a serial device is
+  dedicated to MIDI communication, either to an external MIDI device through a DIN5
+  or other connector, or to a known hardwired MIDI controller. This device must be a
+  child node of a serial node.
+
+  Can only be set to use standard baud rates corresponding to supported rates of the
+  parent serial device. If the standard MIDI baud of 31.25 kBaud is needed 
+  (as would be the case if interfacing with arbitrary external MIDI devices),
+  configure the clocks of the parent serial device so that a requested baud of 38.4 kBaud
+  resuts in the standard MIDI baud rate, and set the 'current-speed' property to 38400.
+
+properties:
+  compatible:
+    const: serialmidi
+
+  current-speed:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Baudrate to set the serial port to when this MIDI device is opened.
+      If not specified, the parent serial device is allowed to use its default baud.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+        midi {
+            compatible = "serialmidi";
+            current-speed = <38400>;
+        };
+    };
-- 
2.33.0

[PATCH v5 1/2] dt-bindings: sound: Add generic serial MIDI device

[Daniel Kaehn]

Adds dt-binding for a Generic MIDI Interface using a serial device.

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 .../devicetree/bindings/sound/serialmidi.yaml | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml

diff --git a/Documentation/devicetree/bindings/sound/serialmidi.yaml b/Documentation/devicetree/bindings/sound/serialmidi.yaml
new file mode 100644
index 000000000000..06a894e1b91d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/serialmidi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic Serial MIDI Interface
+
+maintainers:
+  - Daniel Kaehn <kaehndan@gmail.com>
+
+description: 
+  Generic MIDI interface using a serial device. This denotes that a serial device is
+  dedicated to MIDI communication, either to an external MIDI device through a DIN5
+  or other connector, or to a known hardwired MIDI controller. This device must be a
+  child node of a serial node.
+
+  Can only be set to use standard baud rates corresponding to supported rates of the
+  parent serial device. If the standard MIDI baud of 31.25 kBaud is needed 
+  (as would be the case if interfacing with arbitrary external MIDI devices),
+  configure the clocks of the parent serial device so that a requested baud of 38.4 kBaud
+  resuts in the standard MIDI baud rate, and set the 'current-speed' property to 38400.
+
+properties:
+  compatible:
+    const: serialmidi
+
+  current-speed:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Baudrate to set the serial port to when this MIDI device is opened.
+      If not specified, the parent serial device is allowed to use its default baud.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+        midi {
+            compatible = "serialmidi";
+            current-speed = <38400>;
+        };
+    };
-- 
2.33.0

[PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

Generic serial MIDI driver adding support for using serial devices
compatible with the serial bus as raw MIDI devices, allowing using
additional serial devices not compatible with the existing
serial-u16550 driver. Supports only setting standard serial baudrates on
the underlying serial device; however, the underlying serial device can
be configured so that a requested 38.4 kBaud is actually the standard MIDI
31.25 kBaud. Supports DeviceTree configuration.

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 sound/drivers/Kconfig          |  18 ++
 sound/drivers/Makefile         |   2 +
 sound/drivers/serial-generic.c | 377 +++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+)
 create mode 100644 sound/drivers/serial-generic.c

diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
index ca4cdf666f82..060e7d3f2439 100644
--- a/sound/drivers/Kconfig
+++ b/sound/drivers/Kconfig
@@ -165,6 +165,24 @@ config SND_SERIAL_U16550
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-serial-u16550.
 
+config SND_SERIAL_GENERIC
+	tristate "Generic serial MIDI driver"
+	depends on SERIAL_DEV_BUS
+	depends on OF
+	select SND_RAWMIDI
+	help
+	  To include support for mapping generic serial devices as raw
+	  ALSA MIDI devices, say Y here. The driver only supports setting
+	  the serial port to standard baudrates. To attain the standard MIDI
+	  baudrate of 31.25 kBaud, configure the clock of the underlying serial
+	  device so that a requested 38.4 kBaud will result in the standard speed.
+
+	  Use this devicetree binding to configure serial port mapping
+	  <file:Documentation/devicetree/bindings/sound/serialmidi.yaml>
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called snd-serial-generic.
+
 config SND_MPU401
 	tristate "Generic MPU-401 UART driver"
 	select SND_MPU401_UART
diff --git a/sound/drivers/Makefile b/sound/drivers/Makefile
index c0fe4eccdaef..b60303180a1b 100644
--- a/sound/drivers/Makefile
+++ b/sound/drivers/Makefile
@@ -10,6 +10,7 @@ snd-mtpav-objs := mtpav.o
 snd-mts64-objs := mts64.o
 snd-portman2x4-objs := portman2x4.o
 snd-serial-u16550-objs := serial-u16550.o
+snd-serial-generic-objs := serial-generic.o
 snd-virmidi-objs := virmidi.o
 
 # Toplevel Module Dependency
@@ -17,6 +18,7 @@ obj-$(CONFIG_SND_DUMMY) += snd-dummy.o
 obj-$(CONFIG_SND_ALOOP) += snd-aloop.o
 obj-$(CONFIG_SND_VIRMIDI) += snd-virmidi.o
 obj-$(CONFIG_SND_SERIAL_U16550) += snd-serial-u16550.o
+obj-$(CONFIG_SND_SERIAL_GENERIC) += snd-serial-generic.o
 obj-$(CONFIG_SND_MTPAV) += snd-mtpav.o
 obj-$(CONFIG_SND_MTS64) += snd-mts64.o
 obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
diff --git a/sound/drivers/serial-generic.c b/sound/drivers/serial-generic.c
new file mode 100644
index 000000000000..8168f8a71e15
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *   serial-generic.c
+ *   Copyright (c) by Daniel Kaehn <kaehndan@gmail.com
+ *   Based on serial-u16550.c by Jaroslav Kysela <perex@perex.cz>,
+ *		                 Isaku Yamahata <yamahata@private.email.ne.jp>,
+ *		                 George Hansper <ghansper@apana.org.au>,
+ *		                 Hannu Savolainen
+ *
+ * Generic serial MIDI driver using the serdev serial bus API for hardware interaction
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+#include <linux/serial_reg.h>
+#include <linux/slab.h>
+#include <linux/dev_printk.h>
+
+#include <sound/core.h>
+#include <sound/rawmidi.h>
+#include <sound/initval.h>
+
+MODULE_DESCRIPTION("Generic serial MIDI driver");
+MODULE_LICENSE("GPL");
+
+
+#define SERIAL_MODE_INPUT_OPEN		1
+#define SERIAL_MODE_OUTPUT_OPEN	2
+#define SERIAL_MODE_INPUT_TRIGGERED	3
+#define SERIAL_MODE_OUTPUT_TRIGGERED	4
+
+
+#define SERIAL_TX_STATE_ACTIVE	1
+#define SERIAL_TX_STATE_WAKEUP	2
+
+
+struct snd_serial_generic {
+	struct serdev_device *serdev;
+
+	struct snd_card *card;
+	struct snd_rawmidi *rmidi;
+	struct snd_rawmidi_substream *midi_output;
+	struct snd_rawmidi_substream *midi_input;
+
+	unsigned int baudrate;
+
+	unsigned long filemode;		/* open status of file */
+	struct work_struct tx_work;
+	unsigned long tx_state;
+
+};
+
+static void snd_serial_generic_tx_wakeup(struct snd_serial_generic *drvdata)
+{
+	if (test_and_set_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state))
+		set_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
+
+	schedule_work(&drvdata->tx_work);
+}
+
+#define INTERNAL_BUF_SIZE 256
+
+static void snd_serial_generic_tx_work(struct work_struct *work)
+{
+	static char buf[INTERNAL_BUF_SIZE];
+	int num_bytes;
+	struct snd_serial_generic *drvdata = container_of(work, struct snd_serial_generic,
+						   tx_work);
+	struct snd_rawmidi_substream *substream = drvdata->midi_output;
+
+	clear_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
+
+	while (!snd_rawmidi_transmit_empty(substream)) {
+		num_bytes = snd_rawmidi_transmit_peek(substream, buf, INTERNAL_BUF_SIZE);
+		num_bytes = serdev_device_write_buf(drvdata->serdev, buf, num_bytes);
+
+		if (!num_bytes)
+			break;
+
+		snd_rawmidi_transmit_ack(substream, num_bytes);
+
+		if (!test_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state))
+			break;
+	}
+
+	clear_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state);
+}
+
+static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
+{
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	snd_serial_generic_tx_wakeup(drvdata);
+}
+
+static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
+				const unsigned char *buf, size_t count)
+{
+	int ret;
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
+	return ret < 0 ? 0 : ret;
+}
+
+static const struct serdev_device_ops snd_serial_generic_serdev_device_ops = {
+	.receive_buf = snd_serial_generic_receive_buf,
+	.write_wakeup = snd_serial_generic_write_wakeup
+};
+
+
+static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
+{
+	int err;
+	unsigned int actual_baud;
+
+	if (!drvdata->filemode) {
+		dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
+			drvdata->card->shortname);
+		err = serdev_device_open(drvdata->serdev);
+		if (err < 0)
+			return err;
+		if (drvdata->baudrate) {
+			actual_baud = serdev_device_set_baudrate(drvdata->serdev,
+				drvdata->baudrate);
+			if (actual_baud != drvdata->baudrate) {
+				dev_warn(drvdata->card->dev, "requested %d baud for card %s but it was actually set to %d\n",
+					drvdata->baudrate, drvdata->card->shortname, actual_baud);
+			}
+		}
+	}
+	return 0;
+}
+
+static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
+{
+	int err;
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Opening input for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0)
+		return err;
+
+	set_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+	drvdata->midi_input = substream;
+	return 0;
+}
+
+static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+
+	drvdata->midi_input = NULL;
+
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+	return 0;
+}
+
+static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
+					int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	if (up)
+		set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
+	else
+		clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
+}
+
+static int snd_serial_generic_output_open(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+	int err;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Opening output for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0)
+		return err;
+
+	set_bit(SERIAL_MODE_OUTPUT_OPEN, &drvdata->filemode);
+
+	drvdata->midi_output = substream;
+	return 0;
+};
+
+static int snd_serial_generic_output_close(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - output close called\n");
+	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+
+	drvdata->midi_output = NULL;
+
+	return 0;
+};
+
+static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
+					 int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Output trigger called with %d\n", up);
+
+	if (up)
+		set_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
+	else
+		clear_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
+
+	if (up)
+		snd_serial_generic_tx_wakeup(drvdata);
+}
+
+
+static void snd_serial_generic_output_drain(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - output drain called\n");
+
+	/* Flush any pending characters */
+	serdev_device_write_flush(drvdata->serdev);
+	cancel_work_sync(&drvdata->tx_work);
+}
+
+
+static const struct snd_rawmidi_ops snd_serial_generic_output = {
+	.open =		snd_serial_generic_output_open,
+	.close =	snd_serial_generic_output_close,
+	.trigger =	snd_serial_generic_output_trigger,
+	.drain =	snd_serial_generic_output_drain,
+};
+
+static const struct snd_rawmidi_ops snd_serial_generic_input = {
+	.open =		snd_serial_generic_input_open,
+	.close =	snd_serial_generic_input_close,
+	.trigger =	snd_serial_generic_input_trigger,
+};
+
+
+static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
+				struct snd_serial_generic *drvdata)
+{
+	int err;
+
+	if (serdev->dev.of_node) {
+		err = of_property_read_u32(serdev->dev.of_node, "current-speed",
+			&drvdata->baudrate);
+		if (err < 0) {
+			dev_warn(drvdata->card->dev,
+				"MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
+				err);
+			drvdata->baudrate = 0;
+		}
+	} else {
+		dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
+			drvdata->card->shortname);
+		drvdata->baudrate = 0;
+	}
+}
+
+static void snd_serial_generic_substreams(struct snd_rawmidi_str *stream, int dev_num)
+{
+	struct snd_rawmidi_substream *substream;
+
+	list_for_each_entry(substream, &stream->substreams, list) {
+		sprintf(substream->name, "Serial MIDI %d-%d", dev_num, substream->number);
+	}
+}
+
+static int snd_serial_generic_rmidi(struct snd_serial_generic *drvdata,
+				int outs, int ins, struct snd_rawmidi **rmidi)
+{
+	struct snd_rawmidi *rrawmidi;
+	int err;
+
+	err = snd_rawmidi_new(drvdata->card, drvdata->card->driver, 0,
+				outs, ins, &rrawmidi);
+
+	if (err < 0)
+		return err;
+
+	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_INPUT,
+				&snd_serial_generic_input);
+	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
+				&snd_serial_generic_output);
+	strcpy(rrawmidi->name, drvdata->card->shortname);
+
+	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT],
+					drvdata->serdev->ctrl->nr);
+	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT],
+					drvdata->serdev->ctrl->nr);
+
+	rrawmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
+			       SNDRV_RAWMIDI_INFO_INPUT |
+			       SNDRV_RAWMIDI_INFO_DUPLEX;
+
+	if (rmidi)
+		*rmidi = rrawmidi;
+	return 0;
+}
+
+static int snd_serial_generic_probe(struct serdev_device *serdev)
+{
+	struct snd_card *card;
+	struct snd_serial_generic *drvdata;
+	int err;
+
+	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
+				SNDRV_DEFAULT_STR1, THIS_MODULE,
+				sizeof(struct snd_serial_generic), &card);
+
+	if (err < 0)
+		return err;
+
+	strcpy(card->driver, "SerialMIDI");
+	sprintf(card->shortname, "SerialMIDI-%d", serdev->ctrl->nr);
+	sprintf(card->longname, "Serial MIDI device at serial%d", serdev->ctrl->nr);
+
+	drvdata = card->private_data;
+
+	drvdata->serdev = serdev;
+	drvdata->card = card;
+
+	snd_serial_generic_parse_dt(serdev, drvdata);
+
+	INIT_WORK(&drvdata->tx_work, snd_serial_generic_tx_work);
+
+	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
+	if (err < 0)
+		return err;
+
+	err = snd_card_register(card);
+	if (err < 0)
+		return err;
+
+	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
+	serdev_device_set_drvdata(drvdata->serdev, drvdata);
+
+	return 0;
+}
+
+#define SND_SERIAL_GENERIC_DRIVER	"snd-serial-generic"
+
+static const struct of_device_id snd_serial_generic_dt_ids[] = {
+	{ .compatible = "serialmidi" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, snd_serial_generic_dt_ids);
+
+static struct serdev_device_driver snd_serial_generic_driver = {
+	.driver	= {
+		.name		= SND_SERIAL_GENERIC_DRIVER,
+		.of_match_table	= of_match_ptr(snd_serial_generic_dt_ids),
+	},
+	.probe	= snd_serial_generic_probe,
+};
+
+module_serdev_device_driver(snd_serial_generic_driver);
-- 
2.33.0

[PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

Generic serial MIDI driver adding support for using serial devices
compatible with the serial bus as raw MIDI devices, allowing using
additional serial devices not compatible with the existing
serial-u16550 driver. Supports only setting standard serial baudrates on
the underlying serial device; however, the underlying serial device can
be configured so that a requested 38.4 kBaud is actually the standard MIDI
31.25 kBaud. Supports DeviceTree configuration.

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 sound/drivers/Kconfig          |  18 ++
 sound/drivers/Makefile         |   2 +
 sound/drivers/serial-generic.c | 377 +++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+)
 create mode 100644 sound/drivers/serial-generic.c

diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
index ca4cdf666f82..060e7d3f2439 100644
--- a/sound/drivers/Kconfig
+++ b/sound/drivers/Kconfig
@@ -165,6 +165,24 @@ config SND_SERIAL_U16550
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-serial-u16550.
 
+config SND_SERIAL_GENERIC
+	tristate "Generic serial MIDI driver"
+	depends on SERIAL_DEV_BUS
+	depends on OF
+	select SND_RAWMIDI
+	help
+	  To include support for mapping generic serial devices as raw
+	  ALSA MIDI devices, say Y here. The driver only supports setting
+	  the serial port to standard baudrates. To attain the standard MIDI
+	  baudrate of 31.25 kBaud, configure the clock of the underlying serial
+	  device so that a requested 38.4 kBaud will result in the standard speed.
+
+	  Use this devicetree binding to configure serial port mapping
+	  <file:Documentation/devicetree/bindings/sound/serialmidi.yaml>
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called snd-serial-generic.
+
 config SND_MPU401
 	tristate "Generic MPU-401 UART driver"
 	select SND_MPU401_UART
diff --git a/sound/drivers/Makefile b/sound/drivers/Makefile
index c0fe4eccdaef..b60303180a1b 100644
--- a/sound/drivers/Makefile
+++ b/sound/drivers/Makefile
@@ -10,6 +10,7 @@ snd-mtpav-objs := mtpav.o
 snd-mts64-objs := mts64.o
 snd-portman2x4-objs := portman2x4.o
 snd-serial-u16550-objs := serial-u16550.o
+snd-serial-generic-objs := serial-generic.o
 snd-virmidi-objs := virmidi.o
 
 # Toplevel Module Dependency
@@ -17,6 +18,7 @@ obj-$(CONFIG_SND_DUMMY) += snd-dummy.o
 obj-$(CONFIG_SND_ALOOP) += snd-aloop.o
 obj-$(CONFIG_SND_VIRMIDI) += snd-virmidi.o
 obj-$(CONFIG_SND_SERIAL_U16550) += snd-serial-u16550.o
+obj-$(CONFIG_SND_SERIAL_GENERIC) += snd-serial-generic.o
 obj-$(CONFIG_SND_MTPAV) += snd-mtpav.o
 obj-$(CONFIG_SND_MTS64) += snd-mts64.o
 obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
diff --git a/sound/drivers/serial-generic.c b/sound/drivers/serial-generic.c
new file mode 100644
index 000000000000..8168f8a71e15
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *   serial-generic.c
+ *   Copyright (c) by Daniel Kaehn <kaehndan@gmail.com
+ *   Based on serial-u16550.c by Jaroslav Kysela <perex@perex.cz>,
+ *		                 Isaku Yamahata <yamahata@private.email.ne.jp>,
+ *		                 George Hansper <ghansper@apana.org.au>,
+ *		                 Hannu Savolainen
+ *
+ * Generic serial MIDI driver using the serdev serial bus API for hardware interaction
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+#include <linux/serial_reg.h>
+#include <linux/slab.h>
+#include <linux/dev_printk.h>
+
+#include <sound/core.h>
+#include <sound/rawmidi.h>
+#include <sound/initval.h>
+
+MODULE_DESCRIPTION("Generic serial MIDI driver");
+MODULE_LICENSE("GPL");
+
+
+#define SERIAL_MODE_INPUT_OPEN		1
+#define SERIAL_MODE_OUTPUT_OPEN	2
+#define SERIAL_MODE_INPUT_TRIGGERED	3
+#define SERIAL_MODE_OUTPUT_TRIGGERED	4
+
+
+#define SERIAL_TX_STATE_ACTIVE	1
+#define SERIAL_TX_STATE_WAKEUP	2
+
+
+struct snd_serial_generic {
+	struct serdev_device *serdev;
+
+	struct snd_card *card;
+	struct snd_rawmidi *rmidi;
+	struct snd_rawmidi_substream *midi_output;
+	struct snd_rawmidi_substream *midi_input;
+
+	unsigned int baudrate;
+
+	unsigned long filemode;		/* open status of file */
+	struct work_struct tx_work;
+	unsigned long tx_state;
+
+};
+
+static void snd_serial_generic_tx_wakeup(struct snd_serial_generic *drvdata)
+{
+	if (test_and_set_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state))
+		set_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
+
+	schedule_work(&drvdata->tx_work);
+}
+
+#define INTERNAL_BUF_SIZE 256
+
+static void snd_serial_generic_tx_work(struct work_struct *work)
+{
+	static char buf[INTERNAL_BUF_SIZE];
+	int num_bytes;
+	struct snd_serial_generic *drvdata = container_of(work, struct snd_serial_generic,
+						   tx_work);
+	struct snd_rawmidi_substream *substream = drvdata->midi_output;
+
+	clear_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
+
+	while (!snd_rawmidi_transmit_empty(substream)) {
+		num_bytes = snd_rawmidi_transmit_peek(substream, buf, INTERNAL_BUF_SIZE);
+		num_bytes = serdev_device_write_buf(drvdata->serdev, buf, num_bytes);
+
+		if (!num_bytes)
+			break;
+
+		snd_rawmidi_transmit_ack(substream, num_bytes);
+
+		if (!test_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state))
+			break;
+	}
+
+	clear_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state);
+}
+
+static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
+{
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	snd_serial_generic_tx_wakeup(drvdata);
+}
+
+static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
+				const unsigned char *buf, size_t count)
+{
+	int ret;
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
+	return ret < 0 ? 0 : ret;
+}
+
+static const struct serdev_device_ops snd_serial_generic_serdev_device_ops = {
+	.receive_buf = snd_serial_generic_receive_buf,
+	.write_wakeup = snd_serial_generic_write_wakeup
+};
+
+
+static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
+{
+	int err;
+	unsigned int actual_baud;
+
+	if (!drvdata->filemode) {
+		dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
+			drvdata->card->shortname);
+		err = serdev_device_open(drvdata->serdev);
+		if (err < 0)
+			return err;
+		if (drvdata->baudrate) {
+			actual_baud = serdev_device_set_baudrate(drvdata->serdev,
+				drvdata->baudrate);
+			if (actual_baud != drvdata->baudrate) {
+				dev_warn(drvdata->card->dev, "requested %d baud for card %s but it was actually set to %d\n",
+					drvdata->baudrate, drvdata->card->shortname, actual_baud);
+			}
+		}
+	}
+	return 0;
+}
+
+static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
+{
+	int err;
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Opening input for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0)
+		return err;
+
+	set_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+	drvdata->midi_input = substream;
+	return 0;
+}
+
+static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+
+	drvdata->midi_input = NULL;
+
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+	return 0;
+}
+
+static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
+					int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	if (up)
+		set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
+	else
+		clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
+}
+
+static int snd_serial_generic_output_open(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+	int err;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Opening output for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0)
+		return err;
+
+	set_bit(SERIAL_MODE_OUTPUT_OPEN, &drvdata->filemode);
+
+	drvdata->midi_output = substream;
+	return 0;
+};
+
+static int snd_serial_generic_output_close(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - output close called\n");
+	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
+
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+
+	drvdata->midi_output = NULL;
+
+	return 0;
+};
+
+static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
+					 int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - Output trigger called with %d\n", up);
+
+	if (up)
+		set_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
+	else
+		clear_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
+
+	if (up)
+		snd_serial_generic_tx_wakeup(drvdata);
+}
+
+
+static void snd_serial_generic_output_drain(struct snd_rawmidi_substream *substream)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	dev_dbg(drvdata->card->dev, "DEBUG - output drain called\n");
+
+	/* Flush any pending characters */
+	serdev_device_write_flush(drvdata->serdev);
+	cancel_work_sync(&drvdata->tx_work);
+}
+
+
+static const struct snd_rawmidi_ops snd_serial_generic_output = {
+	.open =		snd_serial_generic_output_open,
+	.close =	snd_serial_generic_output_close,
+	.trigger =	snd_serial_generic_output_trigger,
+	.drain =	snd_serial_generic_output_drain,
+};
+
+static const struct snd_rawmidi_ops snd_serial_generic_input = {
+	.open =		snd_serial_generic_input_open,
+	.close =	snd_serial_generic_input_close,
+	.trigger =	snd_serial_generic_input_trigger,
+};
+
+
+static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
+				struct snd_serial_generic *drvdata)
+{
+	int err;
+
+	if (serdev->dev.of_node) {
+		err = of_property_read_u32(serdev->dev.of_node, "current-speed",
+			&drvdata->baudrate);
+		if (err < 0) {
+			dev_warn(drvdata->card->dev,
+				"MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
+				err);
+			drvdata->baudrate = 0;
+		}
+	} else {
+		dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
+			drvdata->card->shortname);
+		drvdata->baudrate = 0;
+	}
+}
+
+static void snd_serial_generic_substreams(struct snd_rawmidi_str *stream, int dev_num)
+{
+	struct snd_rawmidi_substream *substream;
+
+	list_for_each_entry(substream, &stream->substreams, list) {
+		sprintf(substream->name, "Serial MIDI %d-%d", dev_num, substream->number);
+	}
+}
+
+static int snd_serial_generic_rmidi(struct snd_serial_generic *drvdata,
+				int outs, int ins, struct snd_rawmidi **rmidi)
+{
+	struct snd_rawmidi *rrawmidi;
+	int err;
+
+	err = snd_rawmidi_new(drvdata->card, drvdata->card->driver, 0,
+				outs, ins, &rrawmidi);
+
+	if (err < 0)
+		return err;
+
+	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_INPUT,
+				&snd_serial_generic_input);
+	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
+				&snd_serial_generic_output);
+	strcpy(rrawmidi->name, drvdata->card->shortname);
+
+	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT],
+					drvdata->serdev->ctrl->nr);
+	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT],
+					drvdata->serdev->ctrl->nr);
+
+	rrawmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
+			       SNDRV_RAWMIDI_INFO_INPUT |
+			       SNDRV_RAWMIDI_INFO_DUPLEX;
+
+	if (rmidi)
+		*rmidi = rrawmidi;
+	return 0;
+}
+
+static int snd_serial_generic_probe(struct serdev_device *serdev)
+{
+	struct snd_card *card;
+	struct snd_serial_generic *drvdata;
+	int err;
+
+	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
+				SNDRV_DEFAULT_STR1, THIS_MODULE,
+				sizeof(struct snd_serial_generic), &card);
+
+	if (err < 0)
+		return err;
+
+	strcpy(card->driver, "SerialMIDI");
+	sprintf(card->shortname, "SerialMIDI-%d", serdev->ctrl->nr);
+	sprintf(card->longname, "Serial MIDI device at serial%d", serdev->ctrl->nr);
+
+	drvdata = card->private_data;
+
+	drvdata->serdev = serdev;
+	drvdata->card = card;
+
+	snd_serial_generic_parse_dt(serdev, drvdata);
+
+	INIT_WORK(&drvdata->tx_work, snd_serial_generic_tx_work);
+
+	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
+	if (err < 0)
+		return err;
+
+	err = snd_card_register(card);
+	if (err < 0)
+		return err;
+
+	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
+	serdev_device_set_drvdata(drvdata->serdev, drvdata);
+
+	return 0;
+}
+
+#define SND_SERIAL_GENERIC_DRIVER	"snd-serial-generic"
+
+static const struct of_device_id snd_serial_generic_dt_ids[] = {
+	{ .compatible = "serialmidi" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, snd_serial_generic_dt_ids);
+
+static struct serdev_device_driver snd_serial_generic_driver = {
+	.driver	= {
+		.name		= SND_SERIAL_GENERIC_DRIVER,
+		.of_match_table	= of_match_ptr(snd_serial_generic_dt_ids),
+	},
+	.probe	= snd_serial_generic_probe,
+};
+
+module_serdev_device_driver(snd_serial_generic_driver);
-- 
2.33.0

Re: [PATCH v5 1/2] dt-bindings: sound: Add generic serial MIDI device

[Rob Herring]

On Mon, May 02, 2022 at 10:04:03AM -0500, Daniel Kaehn wrote:
> Adds dt-binding for a Generic MIDI Interface using a serial device.
> 
> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  .../devicetree/bindings/sound/serialmidi.yaml | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/serialmidi.yaml b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> new file mode 100644
> index 000000000000..06a894e1b91d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/serialmidi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic Serial MIDI Interface
> +
> +maintainers:
> +  - Daniel Kaehn <kaehndan@gmail.com>
> +
> +description: 
> +  Generic MIDI interface using a serial device. This denotes that a serial device is
> +  dedicated to MIDI communication, either to an external MIDI device through a DIN5
> +  or other connector, or to a known hardwired MIDI controller. This device must be a
> +  child node of a serial node.
> +
> +  Can only be set to use standard baud rates corresponding to supported rates of the
> +  parent serial device. If the standard MIDI baud of 31.25 kBaud is needed 
> +  (as would be the case if interfacing with arbitrary external MIDI devices),
> +  configure the clocks of the parent serial device so that a requested baud of 38.4 kBaud
> +  resuts in the standard MIDI baud rate, and set the 'current-speed' property to 38400.

s/resuts/results/

> +
> +properties:
> +  compatible:
> +    const: serialmidi

serial-midi would be a bit more readable. (And then align the filename 
with that.)

> +
> +  current-speed:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Already has a type applied by serial.yaml, so you can drop.

> +    description: Baudrate to set the serial port to when this MIDI device is opened.
> +      If not specified, the parent serial device is allowed to use its default baud.
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serial {
> +        midi {
> +            compatible = "serialmidi";
> +            current-speed = <38400>;
> +        };
> +    };
> -- 
> 2.33.0
> 
> 

Re: [PATCH v5 1/2] dt-bindings: sound: Add generic serial MIDI device

[Rob Herring]

On Mon, May 02, 2022 at 10:04:03AM -0500, Daniel Kaehn wrote:
> Adds dt-binding for a Generic MIDI Interface using a serial device.
> 
> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  .../devicetree/bindings/sound/serialmidi.yaml | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/serialmidi.yaml b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> new file mode 100644
> index 000000000000..06a894e1b91d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/serialmidi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic Serial MIDI Interface
> +
> +maintainers:
> +  - Daniel Kaehn <kaehndan@gmail.com>
> +
> +description: 
> +  Generic MIDI interface using a serial device. This denotes that a serial device is
> +  dedicated to MIDI communication, either to an external MIDI device through a DIN5
> +  or other connector, or to a known hardwired MIDI controller. This device must be a
> +  child node of a serial node.
> +
> +  Can only be set to use standard baud rates corresponding to supported rates of the
> +  parent serial device. If the standard MIDI baud of 31.25 kBaud is needed 
> +  (as would be the case if interfacing with arbitrary external MIDI devices),
> +  configure the clocks of the parent serial device so that a requested baud of 38.4 kBaud
> +  resuts in the standard MIDI baud rate, and set the 'current-speed' property to 38400.

s/resuts/results/

> +
> +properties:
> +  compatible:
> +    const: serialmidi

serial-midi would be a bit more readable. (And then align the filename 
with that.)

> +
> +  current-speed:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Already has a type applied by serial.yaml, so you can drop.

> +    description: Baudrate to set the serial port to when this MIDI device is opened.
> +      If not specified, the parent serial device is allowed to use its default baud.
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serial {
> +        midi {
> +            compatible = "serialmidi";
> +            current-speed = <38400>;
> +        };
> +    };
> -- 
> 2.33.0
> 
> 

Re: [PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Rob Herring]

On Mon, May 02, 2022 at 10:04:04AM -0500, Daniel Kaehn wrote:
> Generic serial MIDI driver adding support for using serial devices
> compatible with the serial bus as raw MIDI devices, allowing using
> additional serial devices not compatible with the existing
> serial-u16550 driver. Supports only setting standard serial baudrates on
> the underlying serial device; however, the underlying serial device can
> be configured so that a requested 38.4 kBaud is actually the standard MIDI
> 31.25 kBaud. Supports DeviceTree configuration.

Curious, what would it take to remove serial-u16550? I suppose some way 
to use it without DT.

> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  sound/drivers/Kconfig          |  18 ++
>  sound/drivers/Makefile         |   2 +
>  sound/drivers/serial-generic.c | 377 +++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+)
>  create mode 100644 sound/drivers/serial-generic.c
> 
> diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
> index ca4cdf666f82..060e7d3f2439 100644
> --- a/sound/drivers/Kconfig
> +++ b/sound/drivers/Kconfig
> @@ -165,6 +165,24 @@ config SND_SERIAL_U16550
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called snd-serial-u16550.
>  
> +config SND_SERIAL_GENERIC
> +	tristate "Generic serial MIDI driver"
> +	depends on SERIAL_DEV_BUS
> +	depends on OF
> +	select SND_RAWMIDI
> +	help
> +	  To include support for mapping generic serial devices as raw
> +	  ALSA MIDI devices, say Y here. The driver only supports setting
> +	  the serial port to standard baudrates. To attain the standard MIDI
> +	  baudrate of 31.25 kBaud, configure the clock of the underlying serial
> +	  device so that a requested 38.4 kBaud will result in the standard speed.
> +
> +	  Use this devicetree binding to configure serial port mapping
> +	  <file:Documentation/devicetree/bindings/sound/serialmidi.yaml>
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called snd-serial-generic.
> +
>  config SND_MPU401
>  	tristate "Generic MPU-401 UART driver"
>  	select SND_MPU401_UART
> diff --git a/sound/drivers/Makefile b/sound/drivers/Makefile
> index c0fe4eccdaef..b60303180a1b 100644
> --- a/sound/drivers/Makefile
> +++ b/sound/drivers/Makefile
> @@ -10,6 +10,7 @@ snd-mtpav-objs := mtpav.o
>  snd-mts64-objs := mts64.o
>  snd-portman2x4-objs := portman2x4.o
>  snd-serial-u16550-objs := serial-u16550.o
> +snd-serial-generic-objs := serial-generic.o
>  snd-virmidi-objs := virmidi.o
>  
>  # Toplevel Module Dependency
> @@ -17,6 +18,7 @@ obj-$(CONFIG_SND_DUMMY) += snd-dummy.o
>  obj-$(CONFIG_SND_ALOOP) += snd-aloop.o
>  obj-$(CONFIG_SND_VIRMIDI) += snd-virmidi.o
>  obj-$(CONFIG_SND_SERIAL_U16550) += snd-serial-u16550.o
> +obj-$(CONFIG_SND_SERIAL_GENERIC) += snd-serial-generic.o
>  obj-$(CONFIG_SND_MTPAV) += snd-mtpav.o
>  obj-$(CONFIG_SND_MTS64) += snd-mts64.o
>  obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
> diff --git a/sound/drivers/serial-generic.c b/sound/drivers/serial-generic.c
> new file mode 100644
> index 000000000000..8168f8a71e15
> --- /dev/null
> +++ b/sound/drivers/serial-generic.c
> @@ -0,0 +1,377 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *   serial-generic.c
> + *   Copyright (c) by Daniel Kaehn <kaehndan@gmail.com
> + *   Based on serial-u16550.c by Jaroslav Kysela <perex@perex.cz>,
> + *		                 Isaku Yamahata <yamahata@private.email.ne.jp>,
> + *		                 George Hansper <ghansper@apana.org.au>,
> + *		                 Hannu Savolainen
> + *
> + * Generic serial MIDI driver using the serdev serial bus API for hardware interaction
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/dev_printk.h>
> +
> +#include <sound/core.h>
> +#include <sound/rawmidi.h>
> +#include <sound/initval.h>
> +
> +MODULE_DESCRIPTION("Generic serial MIDI driver");
> +MODULE_LICENSE("GPL");
> +
> +

1 blank line. Here and elsewhere.

> +#define SERIAL_MODE_INPUT_OPEN		1
> +#define SERIAL_MODE_OUTPUT_OPEN	2
> +#define SERIAL_MODE_INPUT_TRIGGERED	3
> +#define SERIAL_MODE_OUTPUT_TRIGGERED	4
> +
> +
> +#define SERIAL_TX_STATE_ACTIVE	1
> +#define SERIAL_TX_STATE_WAKEUP	2
> +
> +
> +struct snd_serial_generic {
> +	struct serdev_device *serdev;
> +
> +	struct snd_card *card;
> +	struct snd_rawmidi *rmidi;
> +	struct snd_rawmidi_substream *midi_output;
> +	struct snd_rawmidi_substream *midi_input;
> +
> +	unsigned int baudrate;
> +
> +	unsigned long filemode;		/* open status of file */
> +	struct work_struct tx_work;
> +	unsigned long tx_state;
> +
> +};
> +
> +static void snd_serial_generic_tx_wakeup(struct snd_serial_generic *drvdata)
> +{
> +	if (test_and_set_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state))
> +		set_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
> +
> +	schedule_work(&drvdata->tx_work);
> +}
> +
> +#define INTERNAL_BUF_SIZE 256
> +
> +static void snd_serial_generic_tx_work(struct work_struct *work)
> +{
> +	static char buf[INTERNAL_BUF_SIZE];
> +	int num_bytes;
> +	struct snd_serial_generic *drvdata = container_of(work, struct snd_serial_generic,
> +						   tx_work);
> +	struct snd_rawmidi_substream *substream = drvdata->midi_output;
> +
> +	clear_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
> +
> +	while (!snd_rawmidi_transmit_empty(substream)) {
> +		num_bytes = snd_rawmidi_transmit_peek(substream, buf, INTERNAL_BUF_SIZE);
> +		num_bytes = serdev_device_write_buf(drvdata->serdev, buf, num_bytes);
> +
> +		if (!num_bytes)
> +			break;
> +
> +		snd_rawmidi_transmit_ack(substream, num_bytes);
> +
> +		if (!test_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state))
> +			break;
> +	}
> +
> +	clear_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state);
> +}
> +
> +static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
> +{
> +	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
> +
> +	snd_serial_generic_tx_wakeup(drvdata);
> +}
> +
> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *buf, size_t count)
> +{
> +	int ret;
> +	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
> +
> +	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
> +	return ret < 0 ? 0 : ret;
> +}
> +
> +static const struct serdev_device_ops snd_serial_generic_serdev_device_ops = {
> +	.receive_buf = snd_serial_generic_receive_buf,
> +	.write_wakeup = snd_serial_generic_write_wakeup
> +};
> +
> +
> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> +{
> +	int err;
> +	unsigned int actual_baud;
> +
> +	if (!drvdata->filemode) {

if (drvdata->filemode)
	return 0;

And save some indentation. Though, can multiple opens happen or does the 
upper layer prevent that?

> +		dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
> +			drvdata->card->shortname);
> +		err = serdev_device_open(drvdata->serdev);
> +		if (err < 0)
> +			return err;
> +		if (drvdata->baudrate) {

Supporting the default, random baudrates of the underlying UARTs doesn't 
seem that useful to me. Perhaps 38400 should be the default? If so, the 
binding should define that. 

> +			actual_baud = serdev_device_set_baudrate(drvdata->serdev,
> +				drvdata->baudrate);
> +			if (actual_baud != drvdata->baudrate) {
> +				dev_warn(drvdata->card->dev, "requested %d baud for card %s but it was actually set to %d\n",
> +					drvdata->baudrate, drvdata->card->shortname, actual_baud);
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
> +{
> +	int err;
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Opening input for card %s\n",
> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0)
> +		return err;
> +
> +	set_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +	drvdata->midi_input = substream;
> +	return 0;
> +}
> +
> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +
> +	drvdata->midi_input = NULL;
> +
> +	if (!drvdata->filemode)
> +		serdev_device_close(drvdata->serdev);
> +	return 0;
> +}
> +
> +static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
> +					int up)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	if (up)
> +		set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
> +	else
> +		clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);

This bit is never read, only filemode as a whole. I'd assume this won't 
run unless the input is opened first and this can be dropped? If the 
upper layer doesn't control the ordering, this looks racy to me. If the 
above bit is set and snd_serial_generic_input_close() is called, then 
you've left the serdev open forever.

> +}
> +
> +static int snd_serial_generic_output_open(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +	int err;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Opening output for card %s\n",

'DEBUG' seems redundant given the level encodes that.

> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0)
> +		return err;
> +
> +	set_bit(SERIAL_MODE_OUTPUT_OPEN, &drvdata->filemode);
> +
> +	drvdata->midi_output = substream;
> +	return 0;
> +};
> +
> +static int snd_serial_generic_output_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - output close called\n");
> +	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +
> +	if (!drvdata->filemode)
> +		serdev_device_close(drvdata->serdev);
> +
> +	drvdata->midi_output = NULL;
> +
> +	return 0;
> +};
> +
> +static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
> +					 int up)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Output trigger called with %d\n", up);
> +
> +	if (up)
> +		set_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
> +	else
> +		clear_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
> +
> +	if (up)
> +		snd_serial_generic_tx_wakeup(drvdata);
> +}
> +
> +
> +static void snd_serial_generic_output_drain(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - output drain called\n");
> +
> +	/* Flush any pending characters */
> +	serdev_device_write_flush(drvdata->serdev);
> +	cancel_work_sync(&drvdata->tx_work);
> +}
> +
> +
> +static const struct snd_rawmidi_ops snd_serial_generic_output = {
> +	.open =		snd_serial_generic_output_open,
> +	.close =	snd_serial_generic_output_close,
> +	.trigger =	snd_serial_generic_output_trigger,
> +	.drain =	snd_serial_generic_output_drain,
> +};
> +
> +static const struct snd_rawmidi_ops snd_serial_generic_input = {
> +	.open =		snd_serial_generic_input_open,
> +	.close =	snd_serial_generic_input_close,
> +	.trigger =	snd_serial_generic_input_trigger,
> +};
> +
> +
> +static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
> +				struct snd_serial_generic *drvdata)
> +{
> +	int err;
> +
> +	if (serdev->dev.of_node) {

Always true.

> +		err = of_property_read_u32(serdev->dev.of_node, "current-speed",
> +			&drvdata->baudrate);
> +		if (err < 0) {
> +			dev_warn(drvdata->card->dev,
> +				"MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
> +				err);
> +			drvdata->baudrate = 0;
> +		}
> +	} else {
> +		dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
> +			drvdata->card->shortname);

That message is somewhat misleading as the node would be missing, but I 
don't think you can get here.

> +		drvdata->baudrate = 0;
> +	}
> +}
> +
> +static void snd_serial_generic_substreams(struct snd_rawmidi_str *stream, int dev_num)
> +{
> +	struct snd_rawmidi_substream *substream;
> +
> +	list_for_each_entry(substream, &stream->substreams, list) {
> +		sprintf(substream->name, "Serial MIDI %d-%d", dev_num, substream->number);
> +	}
> +}
> +
> +static int snd_serial_generic_rmidi(struct snd_serial_generic *drvdata,
> +				int outs, int ins, struct snd_rawmidi **rmidi)
> +{
> +	struct snd_rawmidi *rrawmidi;
> +	int err;
> +
> +	err = snd_rawmidi_new(drvdata->card, drvdata->card->driver, 0,
> +				outs, ins, &rrawmidi);
> +
> +	if (err < 0)
> +		return err;
> +
> +	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_INPUT,
> +				&snd_serial_generic_input);
> +	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
> +				&snd_serial_generic_output);
> +	strcpy(rrawmidi->name, drvdata->card->shortname);
> +
> +	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT],
> +					drvdata->serdev->ctrl->nr);
> +	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT],
> +					drvdata->serdev->ctrl->nr);
> +
> +	rrawmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
> +			       SNDRV_RAWMIDI_INFO_INPUT |
> +			       SNDRV_RAWMIDI_INFO_DUPLEX;
> +
> +	if (rmidi)
> +		*rmidi = rrawmidi;
> +	return 0;
> +}
> +
> +static int snd_serial_generic_probe(struct serdev_device *serdev)
> +{
> +	struct snd_card *card;
> +	struct snd_serial_generic *drvdata;
> +	int err;
> +
> +	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
> +				SNDRV_DEFAULT_STR1, THIS_MODULE,
> +				sizeof(struct snd_serial_generic), &card);
> +
> +	if (err < 0)
> +		return err;
> +
> +	strcpy(card->driver, "SerialMIDI");
> +	sprintf(card->shortname, "SerialMIDI-%d", serdev->ctrl->nr);
> +	sprintf(card->longname, "Serial MIDI device at serial%d", serdev->ctrl->nr);
> +
> +	drvdata = card->private_data;
> +
> +	drvdata->serdev = serdev;
> +	drvdata->card = card;
> +
> +	snd_serial_generic_parse_dt(serdev, drvdata);
> +
> +	INIT_WORK(&drvdata->tx_work, snd_serial_generic_tx_work);
> +
> +	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
> +	if (err < 0)
> +		return err;
> +
> +	err = snd_card_register(card);
> +	if (err < 0)
> +		return err;
> +
> +	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
> +	serdev_device_set_drvdata(drvdata->serdev, drvdata);

Don't these need to be called before snd_card_register()? What 
guarantees open or any of the API is not called between 
snd_card_register and these calls?

> +
> +	return 0;
> +}
> +
> +#define SND_SERIAL_GENERIC_DRIVER	"snd-serial-generic"

I'd drop the define used only once.

> +
> +static const struct of_device_id snd_serial_generic_dt_ids[] = {
> +	{ .compatible = "serialmidi" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, snd_serial_generic_dt_ids);
> +
> +static struct serdev_device_driver snd_serial_generic_driver = {
> +	.driver	= {
> +		.name		= SND_SERIAL_GENERIC_DRIVER,
> +		.of_match_table	= of_match_ptr(snd_serial_generic_dt_ids),
> +	},
> +	.probe	= snd_serial_generic_probe,
> +};
> +
> +module_serdev_device_driver(snd_serial_generic_driver);
> -- 
> 2.33.0
> 
> 

Re: [PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Rob Herring]

On Mon, May 02, 2022 at 10:04:04AM -0500, Daniel Kaehn wrote:
> Generic serial MIDI driver adding support for using serial devices
> compatible with the serial bus as raw MIDI devices, allowing using
> additional serial devices not compatible with the existing
> serial-u16550 driver. Supports only setting standard serial baudrates on
> the underlying serial device; however, the underlying serial device can
> be configured so that a requested 38.4 kBaud is actually the standard MIDI
> 31.25 kBaud. Supports DeviceTree configuration.

Curious, what would it take to remove serial-u16550? I suppose some way 
to use it without DT.

> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  sound/drivers/Kconfig          |  18 ++
>  sound/drivers/Makefile         |   2 +
>  sound/drivers/serial-generic.c | 377 +++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+)
>  create mode 100644 sound/drivers/serial-generic.c
> 
> diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
> index ca4cdf666f82..060e7d3f2439 100644
> --- a/sound/drivers/Kconfig
> +++ b/sound/drivers/Kconfig
> @@ -165,6 +165,24 @@ config SND_SERIAL_U16550
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called snd-serial-u16550.
>  
> +config SND_SERIAL_GENERIC
> +	tristate "Generic serial MIDI driver"
> +	depends on SERIAL_DEV_BUS
> +	depends on OF
> +	select SND_RAWMIDI
> +	help
> +	  To include support for mapping generic serial devices as raw
> +	  ALSA MIDI devices, say Y here. The driver only supports setting
> +	  the serial port to standard baudrates. To attain the standard MIDI
> +	  baudrate of 31.25 kBaud, configure the clock of the underlying serial
> +	  device so that a requested 38.4 kBaud will result in the standard speed.
> +
> +	  Use this devicetree binding to configure serial port mapping
> +	  <file:Documentation/devicetree/bindings/sound/serialmidi.yaml>
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called snd-serial-generic.
> +
>  config SND_MPU401
>  	tristate "Generic MPU-401 UART driver"
>  	select SND_MPU401_UART
> diff --git a/sound/drivers/Makefile b/sound/drivers/Makefile
> index c0fe4eccdaef..b60303180a1b 100644
> --- a/sound/drivers/Makefile
> +++ b/sound/drivers/Makefile
> @@ -10,6 +10,7 @@ snd-mtpav-objs := mtpav.o
>  snd-mts64-objs := mts64.o
>  snd-portman2x4-objs := portman2x4.o
>  snd-serial-u16550-objs := serial-u16550.o
> +snd-serial-generic-objs := serial-generic.o
>  snd-virmidi-objs := virmidi.o
>  
>  # Toplevel Module Dependency
> @@ -17,6 +18,7 @@ obj-$(CONFIG_SND_DUMMY) += snd-dummy.o
>  obj-$(CONFIG_SND_ALOOP) += snd-aloop.o
>  obj-$(CONFIG_SND_VIRMIDI) += snd-virmidi.o
>  obj-$(CONFIG_SND_SERIAL_U16550) += snd-serial-u16550.o
> +obj-$(CONFIG_SND_SERIAL_GENERIC) += snd-serial-generic.o
>  obj-$(CONFIG_SND_MTPAV) += snd-mtpav.o
>  obj-$(CONFIG_SND_MTS64) += snd-mts64.o
>  obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
> diff --git a/sound/drivers/serial-generic.c b/sound/drivers/serial-generic.c
> new file mode 100644
> index 000000000000..8168f8a71e15
> --- /dev/null
> +++ b/sound/drivers/serial-generic.c
> @@ -0,0 +1,377 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *   serial-generic.c
> + *   Copyright (c) by Daniel Kaehn <kaehndan@gmail.com
> + *   Based on serial-u16550.c by Jaroslav Kysela <perex@perex.cz>,
> + *		                 Isaku Yamahata <yamahata@private.email.ne.jp>,
> + *		                 George Hansper <ghansper@apana.org.au>,
> + *		                 Hannu Savolainen
> + *
> + * Generic serial MIDI driver using the serdev serial bus API for hardware interaction
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>
> +#include <linux/serial_reg.h>
> +#include <linux/slab.h>
> +#include <linux/dev_printk.h>
> +
> +#include <sound/core.h>
> +#include <sound/rawmidi.h>
> +#include <sound/initval.h>
> +
> +MODULE_DESCRIPTION("Generic serial MIDI driver");
> +MODULE_LICENSE("GPL");
> +
> +

1 blank line. Here and elsewhere.

> +#define SERIAL_MODE_INPUT_OPEN		1
> +#define SERIAL_MODE_OUTPUT_OPEN	2
> +#define SERIAL_MODE_INPUT_TRIGGERED	3
> +#define SERIAL_MODE_OUTPUT_TRIGGERED	4
> +
> +
> +#define SERIAL_TX_STATE_ACTIVE	1
> +#define SERIAL_TX_STATE_WAKEUP	2
> +
> +
> +struct snd_serial_generic {
> +	struct serdev_device *serdev;
> +
> +	struct snd_card *card;
> +	struct snd_rawmidi *rmidi;
> +	struct snd_rawmidi_substream *midi_output;
> +	struct snd_rawmidi_substream *midi_input;
> +
> +	unsigned int baudrate;
> +
> +	unsigned long filemode;		/* open status of file */
> +	struct work_struct tx_work;
> +	unsigned long tx_state;
> +
> +};
> +
> +static void snd_serial_generic_tx_wakeup(struct snd_serial_generic *drvdata)
> +{
> +	if (test_and_set_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state))
> +		set_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
> +
> +	schedule_work(&drvdata->tx_work);
> +}
> +
> +#define INTERNAL_BUF_SIZE 256
> +
> +static void snd_serial_generic_tx_work(struct work_struct *work)
> +{
> +	static char buf[INTERNAL_BUF_SIZE];
> +	int num_bytes;
> +	struct snd_serial_generic *drvdata = container_of(work, struct snd_serial_generic,
> +						   tx_work);
> +	struct snd_rawmidi_substream *substream = drvdata->midi_output;
> +
> +	clear_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state);
> +
> +	while (!snd_rawmidi_transmit_empty(substream)) {
> +		num_bytes = snd_rawmidi_transmit_peek(substream, buf, INTERNAL_BUF_SIZE);
> +		num_bytes = serdev_device_write_buf(drvdata->serdev, buf, num_bytes);
> +
> +		if (!num_bytes)
> +			break;
> +
> +		snd_rawmidi_transmit_ack(substream, num_bytes);
> +
> +		if (!test_bit(SERIAL_TX_STATE_WAKEUP, &drvdata->tx_state))
> +			break;
> +	}
> +
> +	clear_bit(SERIAL_TX_STATE_ACTIVE, &drvdata->tx_state);
> +}
> +
> +static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
> +{
> +	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
> +
> +	snd_serial_generic_tx_wakeup(drvdata);
> +}
> +
> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *buf, size_t count)
> +{
> +	int ret;
> +	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
> +
> +	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
> +	return ret < 0 ? 0 : ret;
> +}
> +
> +static const struct serdev_device_ops snd_serial_generic_serdev_device_ops = {
> +	.receive_buf = snd_serial_generic_receive_buf,
> +	.write_wakeup = snd_serial_generic_write_wakeup
> +};
> +
> +
> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> +{
> +	int err;
> +	unsigned int actual_baud;
> +
> +	if (!drvdata->filemode) {

if (drvdata->filemode)
	return 0;

And save some indentation. Though, can multiple opens happen or does the 
upper layer prevent that?

> +		dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
> +			drvdata->card->shortname);
> +		err = serdev_device_open(drvdata->serdev);
> +		if (err < 0)
> +			return err;
> +		if (drvdata->baudrate) {

Supporting the default, random baudrates of the underlying UARTs doesn't 
seem that useful to me. Perhaps 38400 should be the default? If so, the 
binding should define that. 

> +			actual_baud = serdev_device_set_baudrate(drvdata->serdev,
> +				drvdata->baudrate);
> +			if (actual_baud != drvdata->baudrate) {
> +				dev_warn(drvdata->card->dev, "requested %d baud for card %s but it was actually set to %d\n",
> +					drvdata->baudrate, drvdata->card->shortname, actual_baud);
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
> +{
> +	int err;
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Opening input for card %s\n",
> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0)
> +		return err;
> +
> +	set_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +	drvdata->midi_input = substream;
> +	return 0;
> +}
> +
> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +
> +	drvdata->midi_input = NULL;
> +
> +	if (!drvdata->filemode)
> +		serdev_device_close(drvdata->serdev);
> +	return 0;
> +}
> +
> +static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
> +					int up)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	if (up)
> +		set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
> +	else
> +		clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);

This bit is never read, only filemode as a whole. I'd assume this won't 
run unless the input is opened first and this can be dropped? If the 
upper layer doesn't control the ordering, this looks racy to me. If the 
above bit is set and snd_serial_generic_input_close() is called, then 
you've left the serdev open forever.

> +}
> +
> +static int snd_serial_generic_output_open(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +	int err;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Opening output for card %s\n",

'DEBUG' seems redundant given the level encodes that.

> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0)
> +		return err;
> +
> +	set_bit(SERIAL_MODE_OUTPUT_OPEN, &drvdata->filemode);
> +
> +	drvdata->midi_output = substream;
> +	return 0;
> +};
> +
> +static int snd_serial_generic_output_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - output close called\n");
> +	clear_bit(SERIAL_MODE_INPUT_OPEN, &drvdata->filemode);
> +
> +	if (!drvdata->filemode)
> +		serdev_device_close(drvdata->serdev);
> +
> +	drvdata->midi_output = NULL;
> +
> +	return 0;
> +};
> +
> +static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
> +					 int up)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - Output trigger called with %d\n", up);
> +
> +	if (up)
> +		set_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
> +	else
> +		clear_bit(SERIAL_MODE_OUTPUT_TRIGGERED, &drvdata->filemode);
> +
> +	if (up)
> +		snd_serial_generic_tx_wakeup(drvdata);
> +}
> +
> +
> +static void snd_serial_generic_output_drain(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> +
> +	dev_dbg(drvdata->card->dev, "DEBUG - output drain called\n");
> +
> +	/* Flush any pending characters */
> +	serdev_device_write_flush(drvdata->serdev);
> +	cancel_work_sync(&drvdata->tx_work);
> +}
> +
> +
> +static const struct snd_rawmidi_ops snd_serial_generic_output = {
> +	.open =		snd_serial_generic_output_open,
> +	.close =	snd_serial_generic_output_close,
> +	.trigger =	snd_serial_generic_output_trigger,
> +	.drain =	snd_serial_generic_output_drain,
> +};
> +
> +static const struct snd_rawmidi_ops snd_serial_generic_input = {
> +	.open =		snd_serial_generic_input_open,
> +	.close =	snd_serial_generic_input_close,
> +	.trigger =	snd_serial_generic_input_trigger,
> +};
> +
> +
> +static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
> +				struct snd_serial_generic *drvdata)
> +{
> +	int err;
> +
> +	if (serdev->dev.of_node) {

Always true.

> +		err = of_property_read_u32(serdev->dev.of_node, "current-speed",
> +			&drvdata->baudrate);
> +		if (err < 0) {
> +			dev_warn(drvdata->card->dev,
> +				"MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
> +				err);
> +			drvdata->baudrate = 0;
> +		}
> +	} else {
> +		dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
> +			drvdata->card->shortname);

That message is somewhat misleading as the node would be missing, but I 
don't think you can get here.

> +		drvdata->baudrate = 0;
> +	}
> +}
> +
> +static void snd_serial_generic_substreams(struct snd_rawmidi_str *stream, int dev_num)
> +{
> +	struct snd_rawmidi_substream *substream;
> +
> +	list_for_each_entry(substream, &stream->substreams, list) {
> +		sprintf(substream->name, "Serial MIDI %d-%d", dev_num, substream->number);
> +	}
> +}
> +
> +static int snd_serial_generic_rmidi(struct snd_serial_generic *drvdata,
> +				int outs, int ins, struct snd_rawmidi **rmidi)
> +{
> +	struct snd_rawmidi *rrawmidi;
> +	int err;
> +
> +	err = snd_rawmidi_new(drvdata->card, drvdata->card->driver, 0,
> +				outs, ins, &rrawmidi);
> +
> +	if (err < 0)
> +		return err;
> +
> +	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_INPUT,
> +				&snd_serial_generic_input);
> +	snd_rawmidi_set_ops(rrawmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
> +				&snd_serial_generic_output);
> +	strcpy(rrawmidi->name, drvdata->card->shortname);
> +
> +	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT],
> +					drvdata->serdev->ctrl->nr);
> +	snd_serial_generic_substreams(&rrawmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT],
> +					drvdata->serdev->ctrl->nr);
> +
> +	rrawmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
> +			       SNDRV_RAWMIDI_INFO_INPUT |
> +			       SNDRV_RAWMIDI_INFO_DUPLEX;
> +
> +	if (rmidi)
> +		*rmidi = rrawmidi;
> +	return 0;
> +}
> +
> +static int snd_serial_generic_probe(struct serdev_device *serdev)
> +{
> +	struct snd_card *card;
> +	struct snd_serial_generic *drvdata;
> +	int err;
> +
> +	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
> +				SNDRV_DEFAULT_STR1, THIS_MODULE,
> +				sizeof(struct snd_serial_generic), &card);
> +
> +	if (err < 0)
> +		return err;
> +
> +	strcpy(card->driver, "SerialMIDI");
> +	sprintf(card->shortname, "SerialMIDI-%d", serdev->ctrl->nr);
> +	sprintf(card->longname, "Serial MIDI device at serial%d", serdev->ctrl->nr);
> +
> +	drvdata = card->private_data;
> +
> +	drvdata->serdev = serdev;
> +	drvdata->card = card;
> +
> +	snd_serial_generic_parse_dt(serdev, drvdata);
> +
> +	INIT_WORK(&drvdata->tx_work, snd_serial_generic_tx_work);
> +
> +	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
> +	if (err < 0)
> +		return err;
> +
> +	err = snd_card_register(card);
> +	if (err < 0)
> +		return err;
> +
> +	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
> +	serdev_device_set_drvdata(drvdata->serdev, drvdata);

Don't these need to be called before snd_card_register()? What 
guarantees open or any of the API is not called between 
snd_card_register and these calls?

> +
> +	return 0;
> +}
> +
> +#define SND_SERIAL_GENERIC_DRIVER	"snd-serial-generic"

I'd drop the define used only once.

> +
> +static const struct of_device_id snd_serial_generic_dt_ids[] = {
> +	{ .compatible = "serialmidi" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, snd_serial_generic_dt_ids);
> +
> +static struct serdev_device_driver snd_serial_generic_driver = {
> +	.driver	= {
> +		.name		= SND_SERIAL_GENERIC_DRIVER,
> +		.of_match_table	= of_match_ptr(snd_serial_generic_dt_ids),
> +	},
> +	.probe	= snd_serial_generic_probe,
> +};
> +
> +module_serdev_device_driver(snd_serial_generic_driver);
> -- 
> 2.33.0
> 
> 

Re: [PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

On Tue, May 3, 2022 at 2:10 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 02, 2022 at 10:04:04AM -0500, Daniel Kaehn wrote:
> > Generic serial MIDI driver adding support for using serial devices
> > compatible with the serial bus as raw MIDI devices, allowing using
> > additional serial devices not compatible with the existing
> > serial-u16550 driver. Supports only setting standard serial baudrates on
> > the underlying serial device; however, the underlying serial device can
> > be configured so that a requested 38.4 kBaud is actually the standard MIDI
> > 31.25 kBaud. Supports DeviceTree configuration.
>
> Curious, what would it take to remove serial-u16550? I suppose some way
> to use it without DT.
>

Take my thoughts with a grain of salt - but I think using without DT
is one of the main things.

The serial-u16550 driver uses module params for pretty much
everything, including mapping of devices - but I must admit I don't
fully understand how exactly the device mapping part works. It appears
to have some register-level device detection logic as well.

Additionally, that driver does support a few of those "oddball" cases
that we briefly discussed earlier. That driver is written specifically
with a PC serial port in mind, to be connected to a MIDI interface
device, rather than to directly connect to a raw MIDI channel. It
supports a few devices that have multiple output MIDI ports, and
multiplexes them in the driver with a special MIDI message. It even
supplies power to the device over the RTS and DTR pins of the port,
which I don't think could be done from the serdev abstraction layer.
Again, that's not really a part of the MIDI standard, it's just how
those specific older MIDI interfaces worked, which happened to connect
to a PC over a serial port and use "MIDI" to communicate between the
PC and the interface.

Overall, I think this probably couldn't replace that driver unless it
were to violate the serdev abstraction for special cases.


> > +
> > +
>
> 1 blank line. Here and elsewhere.
>

Will remove. Thought this was an acceptable way of dividing "sections"
of the driver.


> > +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> > +{
> > +     int err;
> > +     unsigned int actual_baud;
> > +
> > +     if (!drvdata->filemode) {
> if (drvdata->filemode)
>         return 0;
>
> And save some indentation. Though, can multiple opens happen or does the
> upper layer prevent that?
>

Good call. This function is called from both the input_open() and
output_open() -- if one is called while the other is already open, the
serial device will be open already.

> > +             dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
> > +                     drvdata->card->shortname);
> > +             err = serdev_device_open(drvdata->serdev);
> > +             if (err < 0)
> > +                     return err;
> > +             if (drvdata->baudrate) {
>
> Supporting the default, random baudrates of the underlying UARTs doesn't
> seem that useful to me. Perhaps 38400 should be the default? If so, the
> binding should define that.
>

That seems reasonable. I was thinking there might be a scenario where
'current-speed' might be defined on the dt-node of the serial device
itself, and that would save needing to call
`serdev_device_set_baudrate` each time the MIDI device is opened.

> > +static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
> > +                                     int up)
> > +{
> > +     struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> > +
> > +     if (up)
> > +             set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
> > +     else
> > +             clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
>
> This bit is never read, only filemode as a whole. I'd assume this won't
> run unless the input is opened first and this can be dropped?

My misplaced propensity to trust code already in the kernel over
documentation is certainly showing here... Technically a call to the
input or output `trigger` function with a zero `up` parameter should
cause the driver to stop actually receiving or transmitting data
(respectively). This construct seems a bit odd, as it could result in
dropping input or output data... But it seems to be intended to put
the module in a "warm" standby mode. I'll update the driver to behave
this way, I think that would address this part of the comment. I was
initially reluctant to do so, since the serial-u16550 driver behaves
how this driver currently does.

> If the
> upper layer doesn't control the ordering, this looks racy to me. If the
> above bit is set and snd_serial_generic_input_close() is called, then
> you've left the serdev open forever.
>

As for the ordering of calls.. I've observed drain() -> trigger()->
close() always being the ordering (sometimes with repeated drain() and
trigger() calls), but looking through the code, I agree that it
doesn't seem like this is enforced. I'll update the _close() functions
to clear the corresponding _TRIGGERED bit to make sure that is
covered.

> > +
> > +static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
> > +                             struct snd_serial_generic *drvdata)
> > +{
> > +     int err;
> > +
> > +     if (serdev->dev.of_node) {
>
> Always true.
>

I think this was from before the module depended on CONFIG_OF - but it
doesn't really seem possible to use the driver as-is without DT unless
some other way of specifying which serial devices are connected to
MIDI is implemented. Will remove.

> > +             err = of_property_read_u32(serdev->dev.of_node, "current-speed",
> > +                     &drvdata->baudrate);
> > +             if (err < 0) {
> > +                     dev_warn(drvdata->card->dev,
> > +                             "MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
> > +                             err);
> > +                     drvdata->baudrate = 0;
> > +             }
> > +     } else {
> > +             dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
> > +                     drvdata->card->shortname);
>
> That message is somewhat misleading as the node would be missing, but I
> don't think you can get here.

Agreed, will remove.

> > +     err = snd_card_register(card);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
> > +     serdev_device_set_drvdata(drvdata->serdev, drvdata);
>
> Don't these need to be called before snd_card_register()? What
> guarantees open or any of the API is not called between
> snd_card_register and these calls?
>

True, my logic is definitely wrong here. Will correct.

> > +
> > +     return 0;
> > +}
> > +
> > +#define SND_SERIAL_GENERIC_DRIVER    "snd-serial-generic"
>
> I'd drop the define used only once.
>

Will do.

Thanks,
Daniel

Re: [PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn]

On Tue, May 3, 2022 at 2:10 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 02, 2022 at 10:04:04AM -0500, Daniel Kaehn wrote:
> > Generic serial MIDI driver adding support for using serial devices
> > compatible with the serial bus as raw MIDI devices, allowing using
> > additional serial devices not compatible with the existing
> > serial-u16550 driver. Supports only setting standard serial baudrates on
> > the underlying serial device; however, the underlying serial device can
> > be configured so that a requested 38.4 kBaud is actually the standard MIDI
> > 31.25 kBaud. Supports DeviceTree configuration.
>
> Curious, what would it take to remove serial-u16550? I suppose some way
> to use it without DT.
>

Take my thoughts with a grain of salt - but I think using without DT
is one of the main things.

The serial-u16550 driver uses module params for pretty much
everything, including mapping of devices - but I must admit I don't
fully understand how exactly the device mapping part works. It appears
to have some register-level device detection logic as well.

Additionally, that driver does support a few of those "oddball" cases
that we briefly discussed earlier. That driver is written specifically
with a PC serial port in mind, to be connected to a MIDI interface
device, rather than to directly connect to a raw MIDI channel. It
supports a few devices that have multiple output MIDI ports, and
multiplexes them in the driver with a special MIDI message. It even
supplies power to the device over the RTS and DTR pins of the port,
which I don't think could be done from the serdev abstraction layer.
Again, that's not really a part of the MIDI standard, it's just how
those specific older MIDI interfaces worked, which happened to connect
to a PC over a serial port and use "MIDI" to communicate between the
PC and the interface.

Overall, I think this probably couldn't replace that driver unless it
were to violate the serdev abstraction for special cases.


> > +
> > +
>
> 1 blank line. Here and elsewhere.
>

Will remove. Thought this was an acceptable way of dividing "sections"
of the driver.


> > +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> > +{
> > +     int err;
> > +     unsigned int actual_baud;
> > +
> > +     if (!drvdata->filemode) {
> if (drvdata->filemode)
>         return 0;
>
> And save some indentation. Though, can multiple opens happen or does the
> upper layer prevent that?
>

Good call. This function is called from both the input_open() and
output_open() -- if one is called while the other is already open, the
serial device will be open already.

> > +             dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",
> > +                     drvdata->card->shortname);
> > +             err = serdev_device_open(drvdata->serdev);
> > +             if (err < 0)
> > +                     return err;
> > +             if (drvdata->baudrate) {
>
> Supporting the default, random baudrates of the underlying UARTs doesn't
> seem that useful to me. Perhaps 38400 should be the default? If so, the
> binding should define that.
>

That seems reasonable. I was thinking there might be a scenario where
'current-speed' might be defined on the dt-node of the serial device
itself, and that would save needing to call
`serdev_device_set_baudrate` each time the MIDI device is opened.

> > +static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
> > +                                     int up)
> > +{
> > +     struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
> > +
> > +     if (up)
> > +             set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
> > +     else
> > +             clear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);
>
> This bit is never read, only filemode as a whole. I'd assume this won't
> run unless the input is opened first and this can be dropped?

My misplaced propensity to trust code already in the kernel over
documentation is certainly showing here... Technically a call to the
input or output `trigger` function with a zero `up` parameter should
cause the driver to stop actually receiving or transmitting data
(respectively). This construct seems a bit odd, as it could result in
dropping input or output data... But it seems to be intended to put
the module in a "warm" standby mode. I'll update the driver to behave
this way, I think that would address this part of the comment. I was
initially reluctant to do so, since the serial-u16550 driver behaves
how this driver currently does.

> If the
> upper layer doesn't control the ordering, this looks racy to me. If the
> above bit is set and snd_serial_generic_input_close() is called, then
> you've left the serdev open forever.
>

As for the ordering of calls.. I've observed drain() -> trigger()->
close() always being the ordering (sometimes with repeated drain() and
trigger() calls), but looking through the code, I agree that it
doesn't seem like this is enforced. I'll update the _close() functions
to clear the corresponding _TRIGGERED bit to make sure that is
covered.

> > +
> > +static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
> > +                             struct snd_serial_generic *drvdata)
> > +{
> > +     int err;
> > +
> > +     if (serdev->dev.of_node) {
>
> Always true.
>

I think this was from before the module depended on CONFIG_OF - but it
doesn't really seem possible to use the driver as-is without DT unless
some other way of specifying which serial devices are connected to
MIDI is implemented. Will remove.

> > +             err = of_property_read_u32(serdev->dev.of_node, "current-speed",
> > +                     &drvdata->baudrate);
> > +             if (err < 0) {
> > +                     dev_warn(drvdata->card->dev,
> > +                             "MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",
> > +                             err);
> > +                     drvdata->baudrate = 0;
> > +             }
> > +     } else {
> > +             dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",
> > +                     drvdata->card->shortname);
>
> That message is somewhat misleading as the node would be missing, but I
> don't think you can get here.

Agreed, will remove.

> > +     err = snd_card_register(card);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
> > +     serdev_device_set_drvdata(drvdata->serdev, drvdata);
>
> Don't these need to be called before snd_card_register()? What
> guarantees open or any of the API is not called between
> snd_card_register and these calls?
>

True, my logic is definitely wrong here. Will correct.

> > +
> > +     return 0;
> > +}
> > +
> > +#define SND_SERIAL_GENERIC_DRIVER    "snd-serial-generic"
>
> I'd drop the define used only once.
>

Will do.

Thanks,
Daniel