On Tue, Oct 29, 2013 at 10:05:40PM -0500, Matt Sealey wrote:
To be specific, there are several "braindeadisms" in the current bindings for mxs-audio-sgtl5000 and imx-audio-sgtl5000 and imx-audio-wm8962 existing bindings which are being dragged into Eukrea's bindings.
*Please* also try to be a bit more concise and focused, this mail is very long and verbose for the amount of content in it. As I said in my previous e-mail please review the previous extensive discussions on these topics and avoid going over old ground, similiarly like I said if you want to work on making things more generic that's great but please work with the people doing this already.
Endlessly going over old ground and starting from scratch is not going to help move anything forward.
- saif-controllers and ssi-controller could be a single property -
controllers - which list the controller. It will be obvious which kind of controller it is by delving into that node referenced by the phandle.
Picking a consistent name for this might be nice, yes. Feel free to send patches...
- audio-codec is redundantly named as we're defining audio devices
here.. but it should/could be "codecs" since it should be an array of phandles just like controllers (there's no reason you can't have multiple codecs attached to the same i2s bus, it's just not entirely common).
I don't think bikeshedding the name is a worthwhile activity.
If you want to convert it into an array you're adding substantial complication to both defining the link and the software required to support it, it's definitely specialist hardware time and there's a lot of ways that cat can be skinned.
- the compatible properties define a specific board and codec style
which simply isn't used in the drivers, because there's no opportunity to use it. The only reason 3 different compatible properties exist are to probe the 3 different drivers which all do nearly exactly the same thing - collect controllers, codecs, the routing information (for power management, of all reasons..) and stuff them in a handoff structure to allow ASoC to individually probe components.
To repeat yet again, if this concerns you please work with Morimoto-san on his generic card driver. There are some code differences with clock setup for the devices which are encoded in the board definitions as well, plus the external connections.
- The drivers themselves just call imx_audmux_v2_foo() with the
contents of the mux-int-port and mux-ext-port properties - this is Linux subsystem layout dictated by quickly and poorly architected
I don't think anyone is happy with the audmux code but nobody has much interest in spending the time and effort on it so we're stuck just typing the data in statically. If the situation improves we can always just ignore the properties in future.
A lot of it comes from this weird concept that for every file in Linux that can probe on it's own, a device tree needs to somehow define ONE node per file, and define platform properties in that node. This comes eventually from the platform_data concept the original drivers were written against.
Writing device tree bindings is not about looking an an existing driver, platform, or private data structure and creating a new node
That's not what's going on here, let me once again ask you to review the previous discussion on device tree bindings and make sure that you understand the needs of advanced audio hardware.
- for some reason the drivers picking up these nodes do some really
strange things like use the "model" property to fill in the card name. This is a perfectly valid case, but everyone has missed the opportunity to give this an actual friendly name. If you get to a "desktop" and open the mixer properties in PulseAudio, "imx51-babbage-sgtl5000" is not a particularly useful name, nor is "MXC_SPDIF" (from the BSP, I always hated this) or "imx-spdif". This is purely a nitpick at how this ends up in userspace, but it's kind of important as it shows nobody is really caring about how these items get represented in the real system.
So submit patches changing people's model names in the DTSs for the affected boards to be something you find more pleasing... that's what they're there for. I imagine that the people using these boards aren't using UIs that present the names to the user and hence didn't care.
In any case this doesn't seem to be anything to do with the device tree bindings, if you want to complain about specific DTS files go and talk to them.
codec@b { controllers = <&ssi2>; audio-sinks = <&connector1>, <&connector1>, <&mic_in>; audio-sources = <&hp_l>, <&hp_r>, <&connector2>; }
That's not going to scale well to boards that have inter CODEC analogue links, we'd need a node for every single thing that could be a source and sink on every device which seems tedious.
match the hardware in the cases described above - they're defined by picking what the existing Linux drivers used. "Ext Spk", for example, in the SGTL5000 bindings is a Linuxism dragged into the tree. There are many ways to define the actual hardware outputs depending on which part of the docs you trust most, but strings like "LINE_OUT" "Ext Spk" then this is not only poorly documented in it's intent, but erroneous in describing actual hardware (if it's external pin/pads, none of the names match the pad names in the electrical spec for SGTL5000, and a couple of the internal connections are also misnamed..).
No, you've not understood what these are. They are not the pads on the CODEC (as you can tell from the fact that the routing map connects the pads on the CODEC to them...), they are external things on the boards like transducers or jacks.
Feel free to send patches for the documentation if you feel that the documentation is not adequate for your needs. Again I don't think it's terribly useful to spend much time bikeshedding the names, improving the documentation and adding a set of standard names that seem useful might be worthwhile.
Please take a look at Morimoto-san's work on the generic sound card if you want to work on a generic card, it'd be good if some of the people complaining about this stuff could help him work on that as he doesn't seem to be getting any help or review from anyone.
I'll have a good look at it some more if I can find anything more than the file in Linus' current tree and it's rather small history of commits.. if anything new popped up it didn't hit my radar as I'm apparently not subscribed to every Linux mailing list under the sun (nor do I have the time to watch every one of them).
There are patches on the list to add device tree bindings to it, which he's got precious little feedback on - this one e-mail from you is far more feedback by volume than he's had up until now.
In theory, the purpose of these functions is to notify the codec of a change to one of it's input clocks. In what world where the clk_id and the direction be able to be 0 from simple-card if every driver we can see here seems to need something very specific?
We can't, one of the problems with this stuff is that clocking design (especially for the advanced use cases) is not as simple or general as one might desire for a bunch of totally sensible electrical engineering and system design reasons. When we get to the point of deploying simple card widely it's just typing to define a standard constant that everyone can use for whatever the default sensible clocks are, and of course people working on simple card can do that as they're going.
Why would the card driver need to set the sysclk in a very codec-specific way like this *at init time*? Surely this gets hammered into the codec as it starts up it's first stream (and possibly torn down by the upper layers afterwards, and informed again next stream)?
A large proportion of systems (especially simple ones) have a fixed master clock for audio, telling it what rate it's running at is orthogonal to starting and stopping it (assuming that control even exists).
Same for setting the dai_fmt in the same places in both above drivers (and all the rest).
It is vanishingly rare (though possible and possibly even sensible in extremely unusual situations) for the system to want to reconfigure the DAI format dynamically at runtime. No point in doing the same thing over and over again...
Same for the bias level callbacks at the card level which are called by abstraction from another part of the framework, but they nigh on always set codec-level configuration items such as PLLs and sysclks in most if not all cases. The codec has bias level callbacks too, which get called after the card callbacks - I can't find a good example of where snd_soc_card isn't ultimately dereferenced to snd_soc_dai or snd_soc_codec member and then passed to lower level subsystems. They never seem to do anything at the *card* level.
The particular decisions about what to do are a system integration decision, the system integration is a card decision since devices are in general flexible enough to support many system designs.