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