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

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

[Daniel Kaehn]

Changes in v2:
- Fix 'snd_serial_generic_write_wakeup' missing static keyword as 
  reported by the kernel test robot
- Correct 3.125 kBaud -> 31.25 kBaud and 3.84 kBaud -> 38.4 kBaud
  in documentation for MIDI
Reported-by: kernel test robot <lkp@intel.com>

Background as included in the initial cover letter:


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.

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                         |  17 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 344 ++++++++++++++++++
 4 files changed, 404 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: b253435746d9a4a701b5f09211b9c14d3370d0da
-- 
2.35.1

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

[Daniel Kaehn]

Changes in v2:
- Fix 'snd_serial_generic_write_wakeup' missing static keyword as 
  reported by the kernel test robot
- Correct 3.125 kBaud -> 31.25 kBaud and 3.84 kBaud -> 38.4 kBaud
  in documentation for MIDI
Reported-by: kernel test robot <lkp@intel.com>

Background as included in the initial cover letter:


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.

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                         |  17 +
 sound/drivers/Makefile                        |   2 +
 sound/drivers/serial-generic.c                | 344 ++++++++++++++++++
 4 files changed, 404 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
 create mode 100644 sound/drivers/serial-generic.c


base-commit: b253435746d9a4a701b5f09211b9c14d3370d0da
-- 
2.35.1

[PATCH v2 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..74f02a9e1190
--- /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.35.1

[PATCH v2 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..74f02a9e1190
--- /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.35.1

[PATCH v2 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>
---

One ugly portion in the code I wanted to point out, but didn't find a
'nice' way of solving. `snd_serial_generic_output_write` is called to
read from ALSA's output MIDI buffer and write to the serdev_device's
input buffer. While copying directly from the former to the later would
be desirable for performance, I assume violating the abstraction would
never be permissable. The current implementation creates an internal buffer of
an arbitrary size (currently 256) and copies there as an intermediate
step. Any advice on how to make this better is appreciated.

 sound/drivers/Kconfig          |  17 ++
 sound/drivers/Makefile         |   2 +
 sound/drivers/serial-generic.c | 344 +++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 sound/drivers/serial-generic.c

diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
index ca4cdf666f82..edea507fb8e7 100644
--- a/sound/drivers/Kconfig
+++ b/sound/drivers/Kconfig
@@ -165,6 +165,23 @@ 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
+	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..1fa0b3206847
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,344 @@
+// 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 <sound/core.h>
+#include <sound/rawmidi.h>
+#include <sound/initval.h>
+
+MODULE_DESCRIPTION("MIDI serial");
+MODULE_LICENSE("GPL");
+
+#define SERIAL_MODE_NOT_OPENED		(0)
+#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 = 0;
+	unsigned int actual_baud;
+
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {
+		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) {
+				snd_printk(KERN_WARNING "snd-serial-generic: requested %d baud for %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 = 0;
+	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
+
+	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0) {
+		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
+			drvdata->card->shortname);
+		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->private_data;
+
+	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
+	drvdata->midi_input = NULL;
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
+		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->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->private_data;
+	int err;
+
+	snd_printd("snd-serial-generic: DEBUG - Opening output for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0) {
+		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
+			drvdata->card->shortname);
+		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->private_data;
+
+	drvdata->filemode &= ~SERIAL_MODE_OUTPUT_OPEN;
+	drvdata->midi_output = NULL;
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
+		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->private_data;
+
+	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->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 = 0;
+	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 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 int snd_serial_generic_create(struct serdev_device *serdev,
+				struct snd_card *card,
+				struct snd_serial_generic **rserialmidi)
+{
+	struct snd_serial_generic *drvdata;
+	int err;
+
+	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->serdev = serdev;
+	drvdata->card = card;
+
+	if (serdev->dev.of_node) {
+		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);
+		if (err < 0) {
+			snd_printk(KERN_WARNING "snd-serial-generic: MIDI device reading of speed DT param failed with error %d, using default baudrate of serial device\n",
+						err);
+			drvdata->baudrate = 0;
+		}
+	} else {
+		snd_printk(KERN_INFO "snd-serial-generic: MIDI device speed DT param not set for %s, using default baudrate of serial device\n",
+			drvdata->card->shortname);
+		drvdata->baudrate = 0;
+	}
+
+	if (rserialmidi)
+		*rserialmidi = drvdata;
+	return 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;
+
+	rrawmidi->private_data = drvdata;
+	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;
+
+	pr_debug("snd-serial-generic: probe called with:\n\tcontroller number: %d\n",
+		serdev->ctrl->nr);
+
+	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
+				SNDRV_DEFAULT_STR1, THIS_MODULE, 0, &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);
+
+	err = snd_serial_generic_create(serdev, card, &drvdata);
+	if (err < 0)
+		return err;
+
+	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
+	if (err < 0)
+		return err;
+
+	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
+	serdev_device_set_drvdata(drvdata->serdev, drvdata);
+
+	err = snd_card_register(card);
+
+	if (err < 0)
+		return err;
+
+	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,
+};
+
+static int __init alsa_card_serial_generic_init(void)
+{
+	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
+	return serdev_device_driver_register(&snd_serial_generic_driver);
+}
+
+static void __exit alsa_card_serial_generic_exit(void)
+{
+	serdev_device_driver_unregister(&snd_serial_generic_driver);
+}
+
+module_init(alsa_card_serial_generic_init)
+module_exit(alsa_card_serial_generic_exit)
-- 
2.35.1

[PATCH v2 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>
---

One ugly portion in the code I wanted to point out, but didn't find a
'nice' way of solving. `snd_serial_generic_output_write` is called to
read from ALSA's output MIDI buffer and write to the serdev_device's
input buffer. While copying directly from the former to the later would
be desirable for performance, I assume violating the abstraction would
never be permissable. The current implementation creates an internal buffer of
an arbitrary size (currently 256) and copies there as an intermediate
step. Any advice on how to make this better is appreciated.

 sound/drivers/Kconfig          |  17 ++
 sound/drivers/Makefile         |   2 +
 sound/drivers/serial-generic.c | 344 +++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 sound/drivers/serial-generic.c

diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
index ca4cdf666f82..edea507fb8e7 100644
--- a/sound/drivers/Kconfig
+++ b/sound/drivers/Kconfig
@@ -165,6 +165,23 @@ 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
+	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..1fa0b3206847
--- /dev/null
+++ b/sound/drivers/serial-generic.c
@@ -0,0 +1,344 @@
+// 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 <sound/core.h>
+#include <sound/rawmidi.h>
+#include <sound/initval.h>
+
+MODULE_DESCRIPTION("MIDI serial");
+MODULE_LICENSE("GPL");
+
+#define SERIAL_MODE_NOT_OPENED		(0)
+#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 = 0;
+	unsigned int actual_baud;
+
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {
+		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) {
+				snd_printk(KERN_WARNING "snd-serial-generic: requested %d baud for %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 = 0;
+	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
+
+	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0) {
+		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
+			drvdata->card->shortname);
+		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->private_data;
+
+	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
+	drvdata->midi_input = NULL;
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
+		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->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->private_data;
+	int err;
+
+	snd_printd("snd-serial-generic: DEBUG - Opening output for card %s\n",
+		drvdata->card->shortname);
+
+	err = snd_serial_generic_ensure_serdev_open(drvdata);
+	if (err < 0) {
+		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
+			drvdata->card->shortname);
+		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->private_data;
+
+	drvdata->filemode &= ~SERIAL_MODE_OUTPUT_OPEN;
+	drvdata->midi_output = NULL;
+	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
+		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->private_data;
+
+	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->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 = 0;
+	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 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 int snd_serial_generic_create(struct serdev_device *serdev,
+				struct snd_card *card,
+				struct snd_serial_generic **rserialmidi)
+{
+	struct snd_serial_generic *drvdata;
+	int err;
+
+	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->serdev = serdev;
+	drvdata->card = card;
+
+	if (serdev->dev.of_node) {
+		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);
+		if (err < 0) {
+			snd_printk(KERN_WARNING "snd-serial-generic: MIDI device reading of speed DT param failed with error %d, using default baudrate of serial device\n",
+						err);
+			drvdata->baudrate = 0;
+		}
+	} else {
+		snd_printk(KERN_INFO "snd-serial-generic: MIDI device speed DT param not set for %s, using default baudrate of serial device\n",
+			drvdata->card->shortname);
+		drvdata->baudrate = 0;
+	}
+
+	if (rserialmidi)
+		*rserialmidi = drvdata;
+	return 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;
+
+	rrawmidi->private_data = drvdata;
+	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;
+
+	pr_debug("snd-serial-generic: probe called with:\n\tcontroller number: %d\n",
+		serdev->ctrl->nr);
+
+	err  = snd_devm_card_new(&serdev->dev, SNDRV_DEFAULT_IDX1,
+				SNDRV_DEFAULT_STR1, THIS_MODULE, 0, &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);
+
+	err = snd_serial_generic_create(serdev, card, &drvdata);
+	if (err < 0)
+		return err;
+
+	err = snd_serial_generic_rmidi(drvdata, 1, 1, &drvdata->rmidi);
+	if (err < 0)
+		return err;
+
+	serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);
+	serdev_device_set_drvdata(drvdata->serdev, drvdata);
+
+	err = snd_card_register(card);
+
+	if (err < 0)
+		return err;
+
+	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,
+};
+
+static int __init alsa_card_serial_generic_init(void)
+{
+	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
+	return serdev_device_driver_register(&snd_serial_generic_driver);
+}
+
+static void __exit alsa_card_serial_generic_exit(void)
+{
+	serdev_device_driver_unregister(&snd_serial_generic_driver);
+}
+
+module_init(alsa_card_serial_generic_init)
+module_exit(alsa_card_serial_generic_exit)
-- 
2.35.1

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

[Takashi Iwai]

On Thu, 21 Apr 2022 19:24:27 +0200,
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.
> 
> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
> 
> One ugly portion in the code I wanted to point out, but didn't find a
> 'nice' way of solving. `snd_serial_generic_output_write` is called to
> read from ALSA's output MIDI buffer and write to the serdev_device's
> input buffer. While copying directly from the former to the later would
> be desirable for performance, I assume violating the abstraction would
> never be permissable. The current implementation creates an internal buffer of
> an arbitrary size (currently 256) and copies there as an intermediate
> step. Any advice on how to make this better is appreciated.

It's OK, as MIDI data isn't that huge and fast, and the optimization
is done at any time later.

About the code: in general, please avoid the use of snd_printk() and
co.  Those are old helpers, and better to use dev_err(), dev_dbg(),
etc, if possible.

Some more nitpicking:

> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> +{
> +	int err = 0;

Superfluous initialization.

> +	unsigned int actual_baud;
> +
> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {

This expression is rather confusing.  It's essentially a zero check,
and the simple zero check is rather easier to understand that there is
no opener, i.e.

	if (!drvdata->filemode) {
		.....

> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
> +{
> +	int err = 0;

Superfluous.

> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
> +
> +	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0) {
> +		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
> +			drvdata->card->shortname);

Spewing an error message at each time would fill up the kernel log
unnecessarily.  Make it a debug message, if you really need to print
something.

> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
> +
> +	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
> +	drvdata->midi_input = NULL;
> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)

Use zero check instead.  (Ditto for *_output functions).

> +#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->private_data;
> +
> +	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);

This needs to be a loop to process all pending bytes?

> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *buf, size_t count)
> +{
> +	int ret = 0;

Superfluous initialization.

> +static int snd_serial_generic_create(struct serdev_device *serdev,
> +				struct snd_card *card,
> +				struct snd_serial_generic **rserialmidi)
> +{
> +	struct snd_serial_generic *drvdata;
> +	int err;
> +
> +	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->serdev = serdev;
> +	drvdata->card = card;

You can use card's private_data instead of an extra kmalloc().
(Pass sizeof(*drvdata) to the extra_size argument of
snd_devm_card_new()).

> +	if (serdev->dev.of_node) {
> +		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);

So, as we rely on of_node, the Kconfig should have the dependency,
too?

> +static int __init alsa_card_serial_generic_init(void)
> +{
> +	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
> +	return serdev_device_driver_register(&snd_serial_generic_driver);
> +}
> +
> +static void __exit alsa_card_serial_generic_exit(void)
> +{
> +	serdev_device_driver_unregister(&snd_serial_generic_driver);
> +}
> +
> +module_init(alsa_card_serial_generic_init)
> +module_exit(alsa_card_serial_generic_exit)

Those are simplified with module_serdev_device_driver()?


thanks,

Takashi

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

[Takashi Iwai]

On Thu, 21 Apr 2022 19:24:27 +0200,
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.
> 
> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> ---
> 
> One ugly portion in the code I wanted to point out, but didn't find a
> 'nice' way of solving. `snd_serial_generic_output_write` is called to
> read from ALSA's output MIDI buffer and write to the serdev_device's
> input buffer. While copying directly from the former to the later would
> be desirable for performance, I assume violating the abstraction would
> never be permissable. The current implementation creates an internal buffer of
> an arbitrary size (currently 256) and copies there as an intermediate
> step. Any advice on how to make this better is appreciated.

It's OK, as MIDI data isn't that huge and fast, and the optimization
is done at any time later.

About the code: in general, please avoid the use of snd_printk() and
co.  Those are old helpers, and better to use dev_err(), dev_dbg(),
etc, if possible.

Some more nitpicking:

> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
> +{
> +	int err = 0;

Superfluous initialization.

> +	unsigned int actual_baud;
> +
> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {

This expression is rather confusing.  It's essentially a zero check,
and the simple zero check is rather easier to understand that there is
no opener, i.e.

	if (!drvdata->filemode) {
		.....

> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
> +{
> +	int err = 0;

Superfluous.

> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
> +
> +	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
> +		drvdata->card->shortname);
> +
> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
> +	if (err < 0) {
> +		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
> +			drvdata->card->shortname);

Spewing an error message at each time would fill up the kernel log
unnecessarily.  Make it a debug message, if you really need to print
something.

> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
> +{
> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
> +
> +	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
> +	drvdata->midi_input = NULL;
> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)

Use zero check instead.  (Ditto for *_output functions).

> +#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->private_data;
> +
> +	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);

This needs to be a loop to process all pending bytes?

> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
> +				const unsigned char *buf, size_t count)
> +{
> +	int ret = 0;

Superfluous initialization.

> +static int snd_serial_generic_create(struct serdev_device *serdev,
> +				struct snd_card *card,
> +				struct snd_serial_generic **rserialmidi)
> +{
> +	struct snd_serial_generic *drvdata;
> +	int err;
> +
> +	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->serdev = serdev;
> +	drvdata->card = card;

You can use card's private_data instead of an extra kmalloc().
(Pass sizeof(*drvdata) to the extra_size argument of
snd_devm_card_new()).

> +	if (serdev->dev.of_node) {
> +		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);

So, as we rely on of_node, the Kconfig should have the dependency,
too?

> +static int __init alsa_card_serial_generic_init(void)
> +{
> +	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
> +	return serdev_device_driver_register(&snd_serial_generic_driver);
> +}
> +
> +static void __exit alsa_card_serial_generic_exit(void)
> +{
> +	serdev_device_driver_unregister(&snd_serial_generic_driver);
> +}
> +
> +module_init(alsa_card_serial_generic_init)
> +module_exit(alsa_card_serial_generic_exit)

Those are simplified with module_serdev_device_driver()?


thanks,

Takashi

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

[Daniel Kaehn]

Thanks for taking the time to look at this. I've responded to your 
comments below, but ultimately agree with all of them, and will update 
accordingly.


On 4/22/22 09:27, Takashi Iwai wrote:
> On Thu, 21 Apr 2022 19:24:27 +0200,
> 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.
>>
>> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
>> ---
>>
>> One ugly portion in the code I wanted to point out, but didn't find a
>> 'nice' way of solving. `snd_serial_generic_output_write` is called to
>> read from ALSA's output MIDI buffer and write to the serdev_device's
>> input buffer. While copying directly from the former to the later would
>> be desirable for performance, I assume violating the abstraction would
>> never be permissable. The current implementation creates an internal buffer of
>> an arbitrary size (currently 256) and copies there as an intermediate
>> step. Any advice on how to make this better is appreciated.
> 
> It's OK, as MIDI data isn't that huge and fast, and the optimization
> is done at any time later.
>  > About the code: in general, please avoid the use of snd_printk() and
> co.  Those are old helpers, and better to use dev_err(), dev_dbg(),
> etc, if possible.
> 

Good to know, will fix. Saw them in the ALSA headers and assumed that's 
what I should be using.


> Some more nitpicking:
> 
>> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
>> +{
>> +	int err = 0;
> 
> Superfluous initialization.
> 

That it is... will fix.


>> +	unsigned int actual_baud;
>> +
>> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {
> 
> This expression is rather confusing.  It's essentially a zero check,
> and the simple zero check is rather easier to understand that there is
> no opener, i.e.
> 
> 	if (!drvdata->filemode) {
> 		.....
> 

Fair enough. My goal was to be as consistent as I could with the 
existing serial midi driver - but I suppose that is quite old. This will 
be changed.


>> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
>> +{
>> +	int err = 0;
> 
> Superfluous.
> 

Will fix as well.


>> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
>> +
>> +	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
>> +		drvdata->card->shortname);
>> +
>> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
>> +	if (err < 0) {
>> +		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
>> +			drvdata->card->shortname);
> 
> Spewing an error message at each time would fill up the kernel log
> unnecessarily.  Make it a debug message, if you really need to print
> something.
> 

Good point - now that I think of it, the real purpose of this was for 
debugging.

Will remove.

>> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
>> +{
>> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
>> +
>> +	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
>> +	drvdata->midi_input = NULL;
>> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
> 
> Use zero check instead.  (Ditto for *_output functions).
> 

Will update.


>> +#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->private_data;
>> +
>> +	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);
> 
> This needs to be a loop to process all pending bytes?
> 

Part of me was assuming the buffer size should be made big enough so 
that this wouldn't have to loop (and the rest of the data would still 
get processed when _wakeup is called) - but agree that it'd definitely 
better to make sure that either the rawmidi buffer is empty or the 
serdev buffer is full before exiting. Will update.

>> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
>> +				const unsigned char *buf, size_t count)
>> +{
>> +	int ret = 0;
> 
> Superfluous initialization.
> 

I guess I really got in the habit of initializing values to zero.. will fix.


>> +static int snd_serial_generic_create(struct serdev_device *serdev,
>> +				struct snd_card *card,
>> +				struct snd_serial_generic **rserialmidi)
>> +{
>> +	struct snd_serial_generic *drvdata;
>> +	int err;
>> +
>> +	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
>> +	if (!drvdata)
>> +		return -ENOMEM;
>> +
>> +	drvdata->serdev = serdev;
>> +	drvdata->card = card;
> 
> You can use card's private_data instead of an extra kmalloc().
> (Pass sizeof(*drvdata) to the extra_size argument of
> snd_devm_card_new()).
> 
>> +	if (serdev->dev.of_node) {
>> +		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);
> 
> So, as we rely on of_node, the Kconfig should have the dependency,
> too?
> 

Good point. I thought about this for a while, which is partially why I 
wrote in the ability for the only DT param of 'speed' to not be 
specified, using the default of the underlying serial device, in case 
there was a use case for this on a non DT-enabled system, where someone 
might manually bind the driver. I do agree though, since this was 
definitely intended to be used with DT.

I'll add that as a dependency for now, and see if there's a future need 
to use without DT.


>> +static int __init alsa_card_serial_generic_init(void)
>> +{
>> +	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
>> +	return serdev_device_driver_register(&snd_serial_generic_driver);
>> +}
>> +
>> +static void __exit alsa_card_serial_generic_exit(void)
>> +{
>> +	serdev_device_driver_unregister(&snd_serial_generic_driver);
>> +}
>> +
>> +module_init(alsa_card_serial_generic_init)
>> +module_exit(alsa_card_serial_generic_exit)
> 
> Those are simplified with module_serdev_device_driver()?
> 

Will update.
> 
> thanks,
> 
> Takashi

Thanks again for the comments!

Daniel Kaehn

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

[Daniel Kaehn]

Thanks for taking the time to look at this. I've responded to your 
comments below, but ultimately agree with all of them, and will update 
accordingly.


On 4/22/22 09:27, Takashi Iwai wrote:
> On Thu, 21 Apr 2022 19:24:27 +0200,
> 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.
>>
>> Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
>> ---
>>
>> One ugly portion in the code I wanted to point out, but didn't find a
>> 'nice' way of solving. `snd_serial_generic_output_write` is called to
>> read from ALSA's output MIDI buffer and write to the serdev_device's
>> input buffer. While copying directly from the former to the later would
>> be desirable for performance, I assume violating the abstraction would
>> never be permissable. The current implementation creates an internal buffer of
>> an arbitrary size (currently 256) and copies there as an intermediate
>> step. Any advice on how to make this better is appreciated.
> 
> It's OK, as MIDI data isn't that huge and fast, and the optimization
> is done at any time later.
>  > About the code: in general, please avoid the use of snd_printk() and
> co.  Those are old helpers, and better to use dev_err(), dev_dbg(),
> etc, if possible.
> 

Good to know, will fix. Saw them in the ALSA headers and assumed that's 
what I should be using.


> Some more nitpicking:
> 
>> +static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata)
>> +{
>> +	int err = 0;
> 
> Superfluous initialization.
> 

That it is... will fix.


>> +	unsigned int actual_baud;
>> +
>> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED) {
> 
> This expression is rather confusing.  It's essentially a zero check,
> and the simple zero check is rather easier to understand that there is
> no opener, i.e.
> 
> 	if (!drvdata->filemode) {
> 		.....
> 

Fair enough. My goal was to be as consistent as I could with the 
existing serial midi driver - but I suppose that is quite old. This will 
be changed.


>> +static int snd_serial_generic_input_open(struct snd_rawmidi_substream *substream)
>> +{
>> +	int err = 0;
> 
> Superfluous.
> 

Will fix as well.


>> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
>> +
>> +	snd_printd("snd-serial-generic: DEBUG - Opening input for card %s\n",
>> +		drvdata->card->shortname);
>> +
>> +	err = snd_serial_generic_ensure_serdev_open(drvdata);
>> +	if (err < 0) {
>> +		snd_printk(KERN_WARNING "snd-serial-generic: failed to open input for card %s",
>> +			drvdata->card->shortname);
> 
> Spewing an error message at each time would fill up the kernel log
> unnecessarily.  Make it a debug message, if you really need to print
> something.
> 

Good point - now that I think of it, the real purpose of this was for 
debugging.

Will remove.

>> +static int snd_serial_generic_input_close(struct snd_rawmidi_substream *substream)
>> +{
>> +	struct snd_serial_generic *drvdata = substream->rmidi->private_data;
>> +
>> +	drvdata->filemode &= ~SERIAL_MODE_INPUT_OPEN;
>> +	drvdata->midi_input = NULL;
>> +	if (drvdata->filemode == SERIAL_MODE_NOT_OPENED)
> 
> Use zero check instead.  (Ditto for *_output functions).
> 

Will update.


>> +#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->private_data;
>> +
>> +	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);
> 
> This needs to be a loop to process all pending bytes?
> 

Part of me was assuming the buffer size should be made big enough so 
that this wouldn't have to loop (and the rest of the data would still 
get processed when _wakeup is called) - but agree that it'd definitely 
better to make sure that either the rawmidi buffer is empty or the 
serdev buffer is full before exiting. Will update.

>> +static int snd_serial_generic_receive_buf(struct serdev_device *serdev,
>> +				const unsigned char *buf, size_t count)
>> +{
>> +	int ret = 0;
> 
> Superfluous initialization.
> 

I guess I really got in the habit of initializing values to zero.. will fix.


>> +static int snd_serial_generic_create(struct serdev_device *serdev,
>> +				struct snd_card *card,
>> +				struct snd_serial_generic **rserialmidi)
>> +{
>> +	struct snd_serial_generic *drvdata;
>> +	int err;
>> +
>> +	drvdata = devm_kzalloc(card->dev, sizeof(*drvdata), GFP_KERNEL);
>> +	if (!drvdata)
>> +		return -ENOMEM;
>> +
>> +	drvdata->serdev = serdev;
>> +	drvdata->card = card;
> 
> You can use card's private_data instead of an extra kmalloc().
> (Pass sizeof(*drvdata) to the extra_size argument of
> snd_devm_card_new()).
> 
>> +	if (serdev->dev.of_node) {
>> +		err = of_property_read_u32(serdev->dev.of_node, "speed", &drvdata->baudrate);
> 
> So, as we rely on of_node, the Kconfig should have the dependency,
> too?
> 

Good point. I thought about this for a while, which is partially why I 
wrote in the ability for the only DT param of 'speed' to not be 
specified, using the default of the underlying serial device, in case 
there was a use case for this on a non DT-enabled system, where someone 
might manually bind the driver. I do agree though, since this was 
definitely intended to be used with DT.

I'll add that as a dependency for now, and see if there's a future need 
to use without DT.


>> +static int __init alsa_card_serial_generic_init(void)
>> +{
>> +	snd_printk(KERN_INFO "snd-serial-generic: Generic serial-based MIDI device\n");
>> +	return serdev_device_driver_register(&snd_serial_generic_driver);
>> +}
>> +
>> +static void __exit alsa_card_serial_generic_exit(void)
>> +{
>> +	serdev_device_driver_unregister(&snd_serial_generic_driver);
>> +}
>> +
>> +module_init(alsa_card_serial_generic_init)
>> +module_exit(alsa_card_serial_generic_exit)
> 
> Those are simplified with module_serdev_device_driver()?
> 

Will update.
> 
> thanks,
> 
> Takashi

Thanks again for the comments!

Daniel Kaehn