Re: [PATCH v5 2/2] Add generic serial MIDI driver using serial bus API

[Daniel Kaehn <kaehndan@gmail.com>]

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);
> > +     else
> > +             clear_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