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

[PATCH v4 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 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 |  41 +++
 sound/drivers/Kconfig                         |  18 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 319 ++++++++++++++++++
 4 files changed, 380 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: d615b5416f8a1afeb82d13b238f8152c572d59c0
-- 
2.33.0

[PATCH v4 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 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 |  41 +++
 sound/drivers/Kconfig                         |  18 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 319 ++++++++++++++++++
 4 files changed, 380 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: d615b5416f8a1afeb82d13b238f8152c572d59c0
-- 
2.33.0

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

[Daniel Kaehn]

Adds dt-binding for snd-serial-generic serial MIDI driver

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
 1 file changed, 41 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..38ef49a0c2f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
@@ -0,0 +1,41 @@
+# 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 Device
+
+maintainers:
+  - Daniel Kaehn <kaehndan@gmail.com>
+
+description: |
+  Generic MIDI interface using a serial device. Can only be set to use standard speeds
+  corresponding to supported baud rates of the underlying serial device. If standard MIDI
+  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
+  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
+
+properties:
+  compatible:
+    const: serialmidi
+
+  speed:
+    maxItems: 1
+    description: |
+      Speed to set the serial port to when the MIDI device is opened.
+      If not specified, the underlying serial device is allowed to use its configured default speed.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+        midi {
+            compatible = "serialmidi";
+            speed = <38400>;
+        };
+    };
-- 
2.33.0

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

[Daniel Kaehn]

Adds dt-binding for snd-serial-generic serial MIDI driver

Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
---
 .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
 1 file changed, 41 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..38ef49a0c2f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
@@ -0,0 +1,41 @@
+# 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 Device
+
+maintainers:
+  - Daniel Kaehn <kaehndan@gmail.com>
+
+description: |
+  Generic MIDI interface using a serial device. Can only be set to use standard speeds
+  corresponding to supported baud rates of the underlying serial device. If standard MIDI
+  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
+  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
+
+properties:
+  compatible:
+    const: serialmidi
+
+  speed:
+    maxItems: 1
+    description: |
+      Speed to set the serial port to when the MIDI device is opened.
+      If not specified, the underlying serial device is allowed to use its configured default speed.
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+        midi {
+            compatible = "serialmidi";
+            speed = <38400>;
+        };
+    };
-- 
2.33.0

[PATCH v4 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 | 319 +++++++++++++++++++++++++++++++++
 3 files changed, 339 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..b25daf89cf52
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,319 @@
+// 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 << 0)
+#define SERIAL_MODE_OUTPUT_OPEN	(1 << 1)
+#define SERIAL_MODE_INPUT_TRIGGERED	(1 << 2)
+#define SERIAL_MODE_OUTPUT_TRIGGERED	(1 << 3)
+
+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;
+
+	int filemode;		/* open status of file */
+	unsigned int baudrate;
+};
+
+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;
+
+	drvdata->filemode |= SERIAL_MODE_INPUT_OPEN;
+	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;
+
+	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
+	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)
+		drvdata->filemode |= SERIAL_MODE_INPUT_TRIGGERED;
+	else
+		drvdata->filemode &= ~SERIAL_MODE_INPUT_TRIGGERED;
+}
+
+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;
+
+	drvdata->filemode |= SERIAL_MODE_OUTPUT_OPEN;
+	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;
+
+	drvdata->filemode &= ~SERIAL_MODE_OUTPUT_OPEN;
+	drvdata->midi_output = NULL;
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+	return 0;
+};
+
+#define INTERNAL_BUF_SIZE 256
+
+static void snd_serial_generic_output_write(struct snd_rawmidi_substream *substream)
+{
+	static char buf[INTERNAL_BUF_SIZE];
+	int num_bytes;
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	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);
+		snd_rawmidi_transmit_ack(substream, num_bytes);
+	}
+}
+
+static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
+					 int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	if (up)
+		drvdata->filemode |= SERIAL_MODE_OUTPUT_TRIGGERED;
+	else
+		drvdata->filemode &= ~SERIAL_MODE_OUTPUT_TRIGGERED;
+	if (up)
+		snd_serial_generic_output_write(substream);
+}
+
+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,
+};
+
+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 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);
+	dev_dbg(drvdata->card->dev, "DEBUG - Receive buf called for card %s\n",
+		drvdata->card->shortname);
+	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
+	return ret < 0 ? 0 : ret;
+}
+
+static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
+{
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	if (!snd_rawmidi_transmit_empty(drvdata->midi_output))
+		snd_serial_generic_output_write(drvdata->midi_output);
+}
+
+
+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 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, "speed", &drvdata->baudrate);
+		if (err < 0) {
+			dev_warn(drvdata->card->dev,
+				"MIDI device reading of 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 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);
+
+	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 v4 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 | 319 +++++++++++++++++++++++++++++++++
 3 files changed, 339 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..b25daf89cf52
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,319 @@
+// 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 << 0)
+#define SERIAL_MODE_OUTPUT_OPEN	(1 << 1)
+#define SERIAL_MODE_INPUT_TRIGGERED	(1 << 2)
+#define SERIAL_MODE_OUTPUT_TRIGGERED	(1 << 3)
+
+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;
+
+	int filemode;		/* open status of file */
+	unsigned int baudrate;
+};
+
+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;
+
+	drvdata->filemode |= SERIAL_MODE_INPUT_OPEN;
+	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;
+
+	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
+	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)
+		drvdata->filemode |= SERIAL_MODE_INPUT_TRIGGERED;
+	else
+		drvdata->filemode &= ~SERIAL_MODE_INPUT_TRIGGERED;
+}
+
+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;
+
+	drvdata->filemode |= SERIAL_MODE_OUTPUT_OPEN;
+	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;
+
+	drvdata->filemode &= ~SERIAL_MODE_OUTPUT_OPEN;
+	drvdata->midi_output = NULL;
+	if (!drvdata->filemode)
+		serdev_device_close(drvdata->serdev);
+	return 0;
+};
+
+#define INTERNAL_BUF_SIZE 256
+
+static void snd_serial_generic_output_write(struct snd_rawmidi_substream *substream)
+{
+	static char buf[INTERNAL_BUF_SIZE];
+	int num_bytes;
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	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);
+		snd_rawmidi_transmit_ack(substream, num_bytes);
+	}
+}
+
+static void snd_serial_generic_output_trigger(struct snd_rawmidi_substream *substream,
+					 int up)
+{
+	struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;
+
+	if (up)
+		drvdata->filemode |= SERIAL_MODE_OUTPUT_TRIGGERED;
+	else
+		drvdata->filemode &= ~SERIAL_MODE_OUTPUT_TRIGGERED;
+	if (up)
+		snd_serial_generic_output_write(substream);
+}
+
+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,
+};
+
+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 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);
+	dev_dbg(drvdata->card->dev, "DEBUG - Receive buf called for card %s\n",
+		drvdata->card->shortname);
+	ret = snd_rawmidi_receive(drvdata->midi_input, buf, count);
+	return ret < 0 ? 0 : ret;
+}
+
+static void snd_serial_generic_write_wakeup(struct serdev_device *serdev)
+{
+	struct snd_serial_generic *drvdata = serdev_device_get_drvdata(serdev);
+
+	if (!snd_rawmidi_transmit_empty(drvdata->midi_output))
+		snd_serial_generic_output_write(drvdata->midi_output);
+}
+
+
+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 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, "speed", &drvdata->baudrate);
+		if (err < 0) {
+			dev_warn(drvdata->card->dev,
+				"MIDI device reading of 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 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);
+
+	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 v4 1/2] dt-bindings: sound: Add generic serial MIDI device

[Rob Herring]

On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> Adds dt-binding for snd-serial-generic serial MIDI driver

Bindings are for h/w and there's no such thing as generic h/w. There are 
some exceptions but you'll have to justify why this is special.


> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
>  1 file changed, 41 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..38ef49a0c2f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> @@ -0,0 +1,41 @@
> +# 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 Device
> +
> +maintainers:
> +  - Daniel Kaehn <kaehndan@gmail.com>
> +
> +description: |

Don't need '|' unless there is formatting to preserve.

> +  Generic MIDI interface using a serial device. Can only be set to use standard speeds
> +  corresponding to supported baud rates of the underlying serial device. If standard MIDI
> +  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
> +  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
> +
> +properties:
> +  compatible:
> +    const: serialmidi
> +
> +  speed:

Not a standard property and we already have 2 of them concerning baud 
rate.

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

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

[Rob Herring]

On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> Adds dt-binding for snd-serial-generic serial MIDI driver

Bindings are for h/w and there's no such thing as generic h/w. There are 
some exceptions but you'll have to justify why this is special.


> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
>  .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
>  1 file changed, 41 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..38ef49a0c2f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> @@ -0,0 +1,41 @@
> +# 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 Device
> +
> +maintainers:
> +  - Daniel Kaehn <kaehndan@gmail.com>
> +
> +description: |

Don't need '|' unless there is formatting to preserve.

> +  Generic MIDI interface using a serial device. Can only be set to use standard speeds
> +  corresponding to supported baud rates of the underlying serial device. If standard MIDI
> +  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
> +  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
> +
> +properties:
> +  compatible:
> +    const: serialmidi
> +
> +  speed:

Not a standard property and we already have 2 of them concerning baud 
rate.

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

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

[Dan K]

On Mon, Apr 25, 2022 at 5:16 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> > Adds dt-binding for snd-serial-generic serial MIDI driver
>
> Bindings are for h/w and there's no such thing as generic h/w. There are
> some exceptions but you'll have to justify why this is special.
>

Thanks for taking the time to look at this!

Not entirely sure if you mean that I'll need to justify the existence
/ need for this binding,
or the use of the term 'generic' -- just in case, I'll make sure to
respond to both. Note that
I'm justifying my reasoning for submitting the binding - but I'm
uncertain myself if my reasoning
is valid, as someone new to kernel development.

The intent of this binding is to signify that a serial port (namely a
UART) is connected
in hardware to a MIDI decoupling circuit, which then connects to some
(any) sort of MIDI device,
either directly to an on-board device, or via a jack/connector. In a
sense, the MIDI device that this
connects to is 'generic', as the identity of the device does not need
to be known to interface with it
over MIDI for most use cases.

I see how this is a bit of an oddball, since it's not specifically
describing a particular hardware
device attached to a UART (like some of the bluetooth modules are),
but thought this sort of
binding might be permissible because of things like the
gpio-matrix-keypad binding, which doesn't
describe specific switches, just how thoise switches are wired, and
what GPIO they use, which is all
that is needed to interface with them. Some MIDI devices implement
extra low-level features like device
multiplexing, but these aren't (to my knowledge) common, and are
beyond what this supports.


The reason that the corresponding driver written has the name
'generic' is for an entirely
different reason. A "serial MIDI" driver already exists in the kernel,
however, it  interfaces only with
u16550-compatible UARTs. This driver uses the serial bus, making it
work with 'generic' serial devices.


If this comment was directed toward the use of 'generic' in the commit
message and binding
descriptions: I can reword them to be more specific and to avoid the term.


>
> > Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> > ---
> >  .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
> >  1 file changed, 41 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..38ef49a0c2f9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> > @@ -0,0 +1,41 @@
> > +# 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 Device
> > +
> > +maintainers:
> > +  - Daniel Kaehn <kaehndan@gmail.com>
> > +
> > +description: |
>
> Don't need '|' unless there is formatting to preserve.
>

Will fix.

> > +  Generic MIDI interface using a serial device. Can only be set to use standard speeds
> > +  corresponding to supported baud rates of the underlying serial device. If standard MIDI
> > +  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
> > +  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
> > +
> > +properties:
> > +  compatible:
> > +    const: serialmidi
> > +
> > +  speed:
>
> Not a standard property and we already have 2 of them concerning baud
> rate.
>

Thanks for the correction - I'll switch this to the existing "baud" property.

I somehow missed that there was a fixed list of standard properties to be used,
and just chose 'speed' as opposed to 'current-speed' which I saw on
serial bindings,
since this speed is fixed and can't (and shouldn't need to be)
changed. "baud" describes
this well enough.

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

Thanks,

Daneil Kaehn

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

[Dan K]

On Mon, Apr 25, 2022 at 5:16 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> > Adds dt-binding for snd-serial-generic serial MIDI driver
>
> Bindings are for h/w and there's no such thing as generic h/w. There are
> some exceptions but you'll have to justify why this is special.
>

Thanks for taking the time to look at this!

Not entirely sure if you mean that I'll need to justify the existence
/ need for this binding,
or the use of the term 'generic' -- just in case, I'll make sure to
respond to both. Note that
I'm justifying my reasoning for submitting the binding - but I'm
uncertain myself if my reasoning
is valid, as someone new to kernel development.

The intent of this binding is to signify that a serial port (namely a
UART) is connected
in hardware to a MIDI decoupling circuit, which then connects to some
(any) sort of MIDI device,
either directly to an on-board device, or via a jack/connector. In a
sense, the MIDI device that this
connects to is 'generic', as the identity of the device does not need
to be known to interface with it
over MIDI for most use cases.

I see how this is a bit of an oddball, since it's not specifically
describing a particular hardware
device attached to a UART (like some of the bluetooth modules are),
but thought this sort of
binding might be permissible because of things like the
gpio-matrix-keypad binding, which doesn't
describe specific switches, just how thoise switches are wired, and
what GPIO they use, which is all
that is needed to interface with them. Some MIDI devices implement
extra low-level features like device
multiplexing, but these aren't (to my knowledge) common, and are
beyond what this supports.


The reason that the corresponding driver written has the name
'generic' is for an entirely
different reason. A "serial MIDI" driver already exists in the kernel,
however, it  interfaces only with
u16550-compatible UARTs. This driver uses the serial bus, making it
work with 'generic' serial devices.


If this comment was directed toward the use of 'generic' in the commit
message and binding
descriptions: I can reword them to be more specific and to avoid the term.


>
> > Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> > ---
> >  .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
> >  1 file changed, 41 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..38ef49a0c2f9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> > @@ -0,0 +1,41 @@
> > +# 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 Device
> > +
> > +maintainers:
> > +  - Daniel Kaehn <kaehndan@gmail.com>
> > +
> > +description: |
>
> Don't need '|' unless there is formatting to preserve.
>

Will fix.

> > +  Generic MIDI interface using a serial device. Can only be set to use standard speeds
> > +  corresponding to supported baud rates of the underlying serial device. If standard MIDI
> > +  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
> > +  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
> > +
> > +properties:
> > +  compatible:
> > +    const: serialmidi
> > +
> > +  speed:
>
> Not a standard property and we already have 2 of them concerning baud
> rate.
>

Thanks for the correction - I'll switch this to the existing "baud" property.

I somehow missed that there was a fixed list of standard properties to be used,
and just chose 'speed' as opposed to 'current-speed' which I saw on
serial bindings,
since this speed is fixed and can't (and shouldn't need to be)
changed. "baud" describes
this well enough.

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

Thanks,

Daneil Kaehn

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

[Rob Herring]

On Mon, Apr 25, 2022 at 07:49:49PM -0500, Dan K wrote:
> On Mon, Apr 25, 2022 at 5:16 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> > > Adds dt-binding for snd-serial-generic serial MIDI driver
> >
> > Bindings are for h/w and there's no such thing as generic h/w. There are
> > some exceptions but you'll have to justify why this is special.
> >
> 
> Thanks for taking the time to look at this!
> 
> Not entirely sure if you mean that I'll need to justify the existence
> / need for this binding,
> or the use of the term 'generic' -- just in case, I'll make sure to
> respond to both. Note that
> I'm justifying my reasoning for submitting the binding - but I'm
> uncertain myself if my reasoning
> is valid, as someone new to kernel development.

'Generic' is really just a red flag.

We've had generic or simple bindings before. The result tends to be a 
never ending stream of new properties to deal with every new variation 
in devices. These can be quirks for device behavior, timing for power 
control, etc.

> The intent of this binding is to signify that a serial port (namely a
> UART) is connected
> in hardware to a MIDI decoupling circuit, which then connects to some
> (any) sort of MIDI device,
> either directly to an on-board device, or via a jack/connector. In a
> sense, the MIDI device that this
> connects to is 'generic', as the identity of the device does not need
> to be known to interface with it
> over MIDI for most use cases.

Okay, maybe it is appropriate. The key part is 'most use cases'. What 
about ones that don't fit into 'most'? It's possible to do both (generic 
binding and device specific bindings), but we need to define when 
generic bindings are appropriate or not.

Do devices ever need power controls or other sideband interfaces? 
Regulators, resets, clocks? If so, you need to describe the specific 
device.

Is a jack/connector in any way standard and have signals other than UART 
(or whatever is the other side of the MIDI decoupling circuit)? We have 
bindings for standard connectors.

I don't really know anything about what this h/w looks like, so any 
pointers or examples would help. 

> I see how this is a bit of an oddball, since it's not specifically
> describing a particular hardware
> device attached to a UART (like some of the bluetooth modules are),

To follow that comparison, all/most BT modules use a standard/generic 
protocol over the serial port. But we don't have compatibles aligned to 
the protocol because the devices are more than just a serial protocol. 
They have GPIOs, regulators, clocks, etc. Furthermore, the serial 
protocols themselves can have extensions and/or quirks.


> but thought this sort of
> binding might be permissible because of things like the
> gpio-matrix-keypad binding, which doesn't
> describe specific switches, just how thoise switches are wired, and
> what GPIO they use, which is all
> that is needed to interface with them. Some MIDI devices implement
> extra low-level features like device
> multiplexing, but these aren't (to my knowledge) common, and are
> beyond what this supports.

At some point devices become simple enough to model generically.

> The reason that the corresponding driver written has the name
> 'generic' is for an entirely
> different reason. A "serial MIDI" driver already exists in the kernel,
> however, it  interfaces only with
> u16550-compatible UARTs. This driver uses the serial bus, making it
> work with 'generic' serial devices.

Bindings are separate from the kernel (though they live in the same 
repository for convenience). A 'generic' binding often appears with a 
'generic' driver. You can have specific bindings with a generic driver. 
The difference with doing that is the OS can evolve without changing the 
binding (an ABI). Maybe initially you use a generic driver until there's 
extra/custom features of the device you want to support with a custom 
driver.

Rob

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

[Rob Herring]

On Mon, Apr 25, 2022 at 07:49:49PM -0500, Dan K wrote:
> On Mon, Apr 25, 2022 at 5:16 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> > > Adds dt-binding for snd-serial-generic serial MIDI driver
> >
> > Bindings are for h/w and there's no such thing as generic h/w. There are
> > some exceptions but you'll have to justify why this is special.
> >
> 
> Thanks for taking the time to look at this!
> 
> Not entirely sure if you mean that I'll need to justify the existence
> / need for this binding,
> or the use of the term 'generic' -- just in case, I'll make sure to
> respond to both. Note that
> I'm justifying my reasoning for submitting the binding - but I'm
> uncertain myself if my reasoning
> is valid, as someone new to kernel development.

'Generic' is really just a red flag.

We've had generic or simple bindings before. The result tends to be a 
never ending stream of new properties to deal with every new variation 
in devices. These can be quirks for device behavior, timing for power 
control, etc.

> The intent of this binding is to signify that a serial port (namely a
> UART) is connected
> in hardware to a MIDI decoupling circuit, which then connects to some
> (any) sort of MIDI device,
> either directly to an on-board device, or via a jack/connector. In a
> sense, the MIDI device that this
> connects to is 'generic', as the identity of the device does not need
> to be known to interface with it
> over MIDI for most use cases.

Okay, maybe it is appropriate. The key part is 'most use cases'. What 
about ones that don't fit into 'most'? It's possible to do both (generic 
binding and device specific bindings), but we need to define when 
generic bindings are appropriate or not.

Do devices ever need power controls or other sideband interfaces? 
Regulators, resets, clocks? If so, you need to describe the specific 
device.

Is a jack/connector in any way standard and have signals other than UART 
(or whatever is the other side of the MIDI decoupling circuit)? We have 
bindings for standard connectors.

I don't really know anything about what this h/w looks like, so any 
pointers or examples would help. 

> I see how this is a bit of an oddball, since it's not specifically
> describing a particular hardware
> device attached to a UART (like some of the bluetooth modules are),

To follow that comparison, all/most BT modules use a standard/generic 
protocol over the serial port. But we don't have compatibles aligned to 
the protocol because the devices are more than just a serial protocol. 
They have GPIOs, regulators, clocks, etc. Furthermore, the serial 
protocols themselves can have extensions and/or quirks.


> but thought this sort of
> binding might be permissible because of things like the
> gpio-matrix-keypad binding, which doesn't
> describe specific switches, just how thoise switches are wired, and
> what GPIO they use, which is all
> that is needed to interface with them. Some MIDI devices implement
> extra low-level features like device
> multiplexing, but these aren't (to my knowledge) common, and are
> beyond what this supports.

At some point devices become simple enough to model generically.

> The reason that the corresponding driver written has the name
> 'generic' is for an entirely
> different reason. A "serial MIDI" driver already exists in the kernel,
> however, it  interfaces only with
> u16550-compatible UARTs. This driver uses the serial bus, making it
> work with 'generic' serial devices.

Bindings are separate from the kernel (though they live in the same 
repository for convenience). A 'generic' binding often appears with a 
'generic' driver. You can have specific bindings with a generic driver. 
The difference with doing that is the OS can evolve without changing the 
binding (an ABI). Maybe initially you use a generic driver until there's 
extra/custom features of the device you want to support with a custom 
driver.

Rob

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

[Dan K]

Thanks for taking the time for a thorough reply!

On Tue, Apr 26, 2022 at 7:47 PM Rob Herring <robh@kernel.org> wrote:
>
> 'Generic' is really just a red flag.
>
> We've had generic or simple bindings before. The result tends to be a
> never ending stream of new properties to deal with every new variation
> in devices. These can be quirks for device behavior, timing for power
> control, etc.
>

Makes sense, I see why that's a concern. I think it's probably unlikely
that would happen here (for reasons described below).

>
> Okay, maybe it is appropriate. The key part is 'most use cases'. What
> about ones that don't fit into 'most'? It's possible to do both (generic
> binding and device specific bindings), but we need to define when
> generic bindings are appropriate or not.
>

Sorry about the vague language.

In many/most cases, a raw/serial MIDI device is an independent external
device, and its connection to another MIDI device would be transient and
through an external cable. Usually, this is a device that a user plugs in
at runtime, such as a MIDI keyboard (/piano) that simply sends and receives
bytes using the MIDI protocol, and its identity isn't known at the time of
devicetree compilation (and doesn't need to be known).

This binding is only describing that a serial port is dedicated to MIDI,
and the only hardware it describes is the circuitry and electrical
connections needed to connect to a MIDI device (likely through a jack).
This covers almost all of the use cases for (serial) MIDI (MIDI is now also
often done over USB / network, in case you aren't familiar). As you can
probably imagine from its use of DT in general - this is targeted toward
embedded devices, allowing an off-the-shelf SOC in an audio product to
interface with an external raw MIDI device.

The only exceptions to 'most use cases' I'm aware of are with some
antiquated MIDI interface devices that connect to an RS232 port and have
multiple output ports (selectable via a special MIDI message), enabling
someone to connect multiple MIDI devices to a PC simultaneously. I only
discovered that these exist because of the existing serial MIDI driver in
the kernel, and some research reveals that few devices like these (with
multiplexed I/O) exist. This is also probably well outside of the use case
for an embedded device.


> Do devices ever need power controls or other sideband interfaces?
> Regulators, resets, clocks? If so, you need to describe the specific
> device.
>
> Is a jack/connector in any way standard and have signals other than UART
> (or whatever is the other side of the MIDI decoupling circuit)? We have
> bindings for standard connectors.
>

The standard connector is a DIN5 connector, but only two signal pins are
used, for RX and TX. No sideband interfaces are used - the MIDI device
connected is typically a completely independent system. Neither device for
MIDI will power the other (except for USB MIDI). Really the only parameter
possible for just the serial MIDI interface itself is the baudrate - which
is fixed to 31.25k in the standard, but a device could feasibly be
connected to an onboard / non-transient custom MIDI controller with a
different baudrate (my use case contains this, as well as the earlier use
case for an external MIDI device).


> I don't really know anything about what this h/w looks like, so any
> pointers or examples would help.
>

I hope the above helps to clarify.

> > I see how this is a bit of an oddball, since it's not specifically
> > describing a particular hardware
> > device attached to a UART (like some of the bluetooth modules are),
>
> To follow that comparison, all/most BT modules use a standard/generic
> protocol over the serial port. But we don't have compatibles aligned to
> the protocol because the devices are more than just a serial protocol.
> They have GPIOs, regulators, clocks, etc. Furthermore, the serial
> protocols themselves can have extensions and/or quirks.
>

I think I would contend that for MIDI, the 'device' this binding describes
more or less is just the serial protocol (and hardware to support the
transmission). Any specific handling of special functions of a device would
be done in userspace.

>
> At some point devices become simple enough to model generically.
>
> > The reason that the corresponding driver written has the name
> > 'generic' is for an entirely
> > different reason. A "serial MIDI" driver already exists in the kernel,
> > however, it  interfaces only with
> > u16550-compatible UARTs. This driver uses the serial bus, making it
> > work with 'generic' serial devices.
>
> Bindings are separate from the kernel (though they live in the same
> repository for convenience). A 'generic' binding often appears with a
> 'generic' driver. You can have specific bindings with a generic driver.
> The difference with doing that is the OS can evolve without changing the
> binding (an ABI). Maybe initially you use a generic driver until there's
> extra/custom features of the device you want to support with a custom
> driver.
>

I've seen that sort of 'specific binding - > generic driver' model before -
but I think you'll agree that since the nature of the external device is
typically transient, the generic binding -> generic driver is probably what
would make sense here.

Thanks,
Daniel Kaehn

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

[Rob Herring]

On Wed, Apr 27, 2022 at 04:29:06AM -0500, Dan K wrote:
> Thanks for taking the time for a thorough reply!
> 
> On Tue, Apr 26, 2022 at 7:47 PM Rob Herring <robh@kernel.org> wrote:
> >
> > 'Generic' is really just a red flag.
> >
> > We've had generic or simple bindings before. The result tends to be a
> > never ending stream of new properties to deal with every new variation
> > in devices. These can be quirks for device behavior, timing for power
> > control, etc.
> >
> 
> Makes sense, I see why that's a concern. I think it's probably unlikely
> that would happen here (for reasons described below).
> 
> >
> > Okay, maybe it is appropriate. The key part is 'most use cases'. What
> > about ones that don't fit into 'most'? It's possible to do both (generic
> > binding and device specific bindings), but we need to define when
> > generic bindings are appropriate or not.
> >
> 
> Sorry about the vague language.
> 
> In many/most cases, a raw/serial MIDI device is an independent external
> device, and its connection to another MIDI device would be transient and
> through an external cable. Usually, this is a device that a user plugs in
> at runtime, such as a MIDI keyboard (/piano) that simply sends and receives
> bytes using the MIDI protocol, and its identity isn't known at the time of
> devicetree compilation (and doesn't need to be known).
> 
> This binding is only describing that a serial port is dedicated to MIDI,
> and the only hardware it describes is the circuitry and electrical
> connections needed to connect to a MIDI device (likely through a jack).
> This covers almost all of the use cases for (serial) MIDI (MIDI is now also
> often done over USB / network, in case you aren't familiar). As you can
> probably imagine from its use of DT in general - this is targeted toward
> embedded devices, allowing an off-the-shelf SOC in an audio product to
> interface with an external raw MIDI device.
> 
> The only exceptions to 'most use cases' I'm aware of are with some
> antiquated MIDI interface devices that connect to an RS232 port and have
> multiple output ports (selectable via a special MIDI message), enabling
> someone to connect multiple MIDI devices to a PC simultaneously. I only
> discovered that these exist because of the existing serial MIDI driver in
> the kernel, and some research reveals that few devices like these (with
> multiplexed I/O) exist. This is also probably well outside of the use case
> for an embedded device.
> 
> 
> > Do devices ever need power controls or other sideband interfaces?
> > Regulators, resets, clocks? If so, you need to describe the specific
> > device.
> >
> > Is a jack/connector in any way standard and have signals other than UART
> > (or whatever is the other side of the MIDI decoupling circuit)? We have
> > bindings for standard connectors.
> >
> 
> The standard connector is a DIN5 connector, but only two signal pins are
> used, for RX and TX. No sideband interfaces are used - the MIDI device
> connected is typically a completely independent system. Neither device for
> MIDI will power the other (except for USB MIDI). Really the only parameter
> possible for just the serial MIDI interface itself is the baudrate - which
> is fixed to 31.25k in the standard, but a device could feasibly be
> connected to an onboard / non-transient custom MIDI controller with a
> different baudrate (my use case contains this, as well as the earlier use
> case for an external MIDI device).
> 
> 
> > I don't really know anything about what this h/w looks like, so any
> > pointers or examples would help.
> >
> 
> I hope the above helps to clarify.
> 
> > > I see how this is a bit of an oddball, since it's not specifically
> > > describing a particular hardware
> > > device attached to a UART (like some of the bluetooth modules are),
> >
> > To follow that comparison, all/most BT modules use a standard/generic
> > protocol over the serial port. But we don't have compatibles aligned to
> > the protocol because the devices are more than just a serial protocol.
> > They have GPIOs, regulators, clocks, etc. Furthermore, the serial
> > protocols themselves can have extensions and/or quirks.
> >
> 
> I think I would contend that for MIDI, the 'device' this binding describes
> more or less is just the serial protocol (and hardware to support the
> transmission). Any specific handling of special functions of a device would
> be done in userspace.
> 
> >
> > At some point devices become simple enough to model generically.
> >
> > > The reason that the corresponding driver written has the name
> > > 'generic' is for an entirely
> > > different reason. A "serial MIDI" driver already exists in the kernel,
> > > however, it  interfaces only with
> > > u16550-compatible UARTs. This driver uses the serial bus, making it
> > > work with 'generic' serial devices.
> >
> > Bindings are separate from the kernel (though they live in the same
> > repository for convenience). A 'generic' binding often appears with a
> > 'generic' driver. You can have specific bindings with a generic driver.
> > The difference with doing that is the OS can evolve without changing the
> > binding (an ABI). Maybe initially you use a generic driver until there's
> > extra/custom features of the device you want to support with a custom
> > driver.
> >
> 
> I've seen that sort of 'specific binding - > generic driver' model before -
> but I think you'll agree that since the nature of the external device is
> typically transient, the generic binding -> generic driver is probably what
> would make sense here.

Thanks for the all the details and I do agree. Can you add some 
description of the h/w from above into the binding description.

Rob

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

[Dan K]

Will do, thanks.

Daniel Kaehn