On Tue, May 3, 2022 at 2:10 PM Rob Herring <robh@xxxxxxxxxx> 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
