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@xxxxxxxxx>
---
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
