Re: [PATCH v4 1/2] dt-bindings: sound: Add generic serial MIDI device

[Dan K <kaehndan@gmail.com>]

On Mon, Apr 25, 2022 at 5:16 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Apr 25, 2022 at 02:16:02PM -0500, Daniel Kaehn wrote:
> > Adds dt-binding for snd-serial-generic serial MIDI driver
>
> Bindings are for h/w and there's no such thing as generic h/w. There are
> some exceptions but you'll have to justify why this is special.
>

Thanks for taking the time to look at this!

Not entirely sure if you mean that I'll need to justify the existence
/ need for this binding,
or the use of the term 'generic' -- just in case, I'll make sure to
respond to both. Note that
I'm justifying my reasoning for submitting the binding - but I'm
uncertain myself if my reasoning
is valid, as someone new to kernel development.

The intent of this binding is to signify that a serial port (namely a
UART) is connected
in hardware to a MIDI decoupling circuit, which then connects to some
(any) sort of MIDI device,
either directly to an on-board device, or via a jack/connector. In a
sense, the MIDI device that this
connects to is 'generic', as the identity of the device does not need
to be known to interface with it
over MIDI for most use cases.

I see how this is a bit of an oddball, since it's not specifically
describing a particular hardware
device attached to a UART (like some of the bluetooth modules are),
but thought this sort of
binding might be permissible because of things like the
gpio-matrix-keypad binding, which doesn't
describe specific switches, just how thoise switches are wired, and
what GPIO they use, which is all
that is needed to interface with them. Some MIDI devices implement
extra low-level features like device
multiplexing, but these aren't (to my knowledge) common, and are
beyond what this supports.


The reason that the corresponding driver written has the name
'generic' is for an entirely
different reason. A "serial MIDI" driver already exists in the kernel,
however, it  interfaces only with
u16550-compatible UARTs. This driver uses the serial bus, making it
work with 'generic' serial devices.


If this comment was directed toward the use of 'generic' in the commit
message and binding
descriptions: I can reword them to be more specific and to avoid the term.


>
> > Signed-off-by: Daniel Kaehn <kaehndan@gmail.com>
> > ---
> >  .../devicetree/bindings/sound/serialmidi.yaml | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/serialmidi.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/serialmidi.yaml b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> > new file mode 100644
> > index 000000000000..38ef49a0c2f9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/serialmidi.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/serialmidi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic Serial MIDI Device
> > +
> > +maintainers:
> > +  - Daniel Kaehn <kaehndan@gmail.com>
> > +
> > +description: |
>
> Don't need '|' unless there is formatting to preserve.
>

Will fix.

> > +  Generic MIDI interface using a serial device. Can only be set to use standard speeds
> > +  corresponding to supported baud rates of the underlying serial device. If standard MIDI
> > +  speed of 31.25 kBaud is needed, configure the clocks of the underlying serial device
> > +  so that a requested speed of 38.4 kBaud resuts in the standard MIDI baud rate.
> > +
> > +properties:
> > +  compatible:
> > +    const: serialmidi
> > +
> > +  speed:
>
> Not a standard property and we already have 2 of them concerning baud
> rate.
>

Thanks for the correction - I'll switch this to the existing "baud" property.

I somehow missed that there was a fixed list of standard properties to be used,
and just chose 'speed' as opposed to 'current-speed' which I saw on
serial bindings,
since this speed is fixed and can't (and shouldn't need to be)
changed. "baud" describes
this well enough.

> > +    maxItems: 1
> > +    description: |
> > +      Speed to set the serial port to when the MIDI device is opened.
> > +      If not specified, the underlying serial device is allowed to use its configured default speed.
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    serial {
> > +        midi {
> > +            compatible = "serialmidi";
> > +            speed = <38400>;
> > +        };
> > +    };
> > --
> > 2.33.0
> >
> >

Thanks,

Daneil Kaehn