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

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