[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 at 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