On Tue, May 3, 2022 at 2:10 PM Rob Herring robh@kernel.org wrote:
On Mon, May 02, 2022 at 10:04:04AM -0500, 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.
Curious, what would it take to remove serial-u16550? I suppose some way to use it without DT.
Take my thoughts with a grain of salt - but I think using without DT is one of the main things.
The serial-u16550 driver uses module params for pretty much everything, including mapping of devices - but I must admit I don't fully understand how exactly the device mapping part works. It appears to have some register-level device detection logic as well.
Additionally, that driver does support a few of those "oddball" cases that we briefly discussed earlier. That driver is written specifically with a PC serial port in mind, to be connected to a MIDI interface device, rather than to directly connect to a raw MIDI channel. It supports a few devices that have multiple output MIDI ports, and multiplexes them in the driver with a special MIDI message. It even supplies power to the device over the RTS and DTR pins of the port, which I don't think could be done from the serdev abstraction layer. Again, that's not really a part of the MIDI standard, it's just how those specific older MIDI interfaces worked, which happened to connect to a PC over a serial port and use "MIDI" to communicate between the PC and the interface.
Overall, I think this probably couldn't replace that driver unless it were to violate the serdev abstraction for special cases.
1 blank line. Here and elsewhere.
Will remove. Thought this was an acceptable way of dividing "sections" of the driver.
+static int snd_serial_generic_ensure_serdev_open(struct snd_serial_generic *drvdata) +{
int err;unsigned int actual_baud;if (!drvdata->filemode) {if (drvdata->filemode) return 0;
And save some indentation. Though, can multiple opens happen or does the upper layer prevent that?
Good call. This function is called from both the input_open() and output_open() -- if one is called while the other is already open, the serial device will be open already.
dev_dbg(drvdata->card->dev, "DEBUG - Opening serial port for card %s\n",drvdata->card->shortname);err = serdev_device_open(drvdata->serdev);if (err < 0)return err;if (drvdata->baudrate) {Supporting the default, random baudrates of the underlying UARTs doesn't seem that useful to me. Perhaps 38400 should be the default? If so, the binding should define that.
That seems reasonable. I was thinking there might be a scenario where 'current-speed' might be defined on the dt-node of the serial device itself, and that would save needing to call `serdev_device_set_baudrate` each time the MIDI device is opened.
+static void snd_serial_generic_input_trigger(struct snd_rawmidi_substream *substream,
int up)+{
struct snd_serial_generic *drvdata = substream->rmidi->card->private_data;if (up)set_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);elseclear_bit(SERIAL_MODE_INPUT_TRIGGERED, &drvdata->filemode);This bit is never read, only filemode as a whole. I'd assume this won't run unless the input is opened first and this can be dropped?
My misplaced propensity to trust code already in the kernel over documentation is certainly showing here... Technically a call to the input or output `trigger` function with a zero `up` parameter should cause the driver to stop actually receiving or transmitting data (respectively). This construct seems a bit odd, as it could result in dropping input or output data... But it seems to be intended to put the module in a "warm" standby mode. I'll update the driver to behave this way, I think that would address this part of the comment. I was initially reluctant to do so, since the serial-u16550 driver behaves how this driver currently does.
If the upper layer doesn't control the ordering, this looks racy to me. If the above bit is set and snd_serial_generic_input_close() is called, then you've left the serdev open forever.
As for the ordering of calls.. I've observed drain() -> trigger()-> close() always being the ordering (sometimes with repeated drain() and trigger() calls), but looking through the code, I agree that it doesn't seem like this is enforced. I'll update the _close() functions to clear the corresponding _TRIGGERED bit to make sure that is covered.
+static void snd_serial_generic_parse_dt(struct serdev_device *serdev,
struct snd_serial_generic *drvdata)+{
int err;if (serdev->dev.of_node) {Always true.
I think this was from before the module depended on CONFIG_OF - but it doesn't really seem possible to use the driver as-is without DT unless some other way of specifying which serial devices are connected to MIDI is implemented. Will remove.
err = of_property_read_u32(serdev->dev.of_node, "current-speed",&drvdata->baudrate);if (err < 0) {dev_warn(drvdata->card->dev,"MIDI device reading of current-speed DT param failed with error %d, using default baudrate of serial device\n",err);drvdata->baudrate = 0;}} else {dev_info(drvdata->card->dev, "MIDI device current-speed DT param not set for %s, using default baudrate of serial device\n",drvdata->card->shortname);That message is somewhat misleading as the node would be missing, but I don't think you can get here.
Agreed, will remove.
err = snd_card_register(card);if (err < 0)return err;serdev_device_set_client_ops(serdev, &snd_serial_generic_serdev_device_ops);serdev_device_set_drvdata(drvdata->serdev, drvdata);Don't these need to be called before snd_card_register()? What guarantees open or any of the API is not called between snd_card_register and these calls?
True, my logic is definitely wrong here. Will correct.
return 0;+}
+#define SND_SERIAL_GENERIC_DRIVER "snd-serial-generic"
I'd drop the define used only once.
Will do.
Thanks, Daniel