| Message ID | 20180214220741.28306-2-sebastian.reichel@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 14, 2018 at 11:07:38PM +0100, Sebastian Reichel wrote: > +&cpcap { > + audio-codec { > + compatible = "motorola,cpcap-audio-codec"; > + }; Why are we adding a separate DT node with no content for this? This is a single chip, we already know that the CODEC part is there from the DT telling us that the chip is there and what we decide is part of the CODEC is going to depend on what the OS running on the system is doing.
Hi Mark, On Fri, Feb 16, 2018 at 11:30:08AM +0000, Mark Brown wrote: > On Wed, Feb 14, 2018 at 11:07:38PM +0100, Sebastian Reichel wrote: > > > +&cpcap { > > + audio-codec { > > + compatible = "motorola,cpcap-audio-codec"; > > + }; > > Why are we adding a separate DT node with no content for this? This is > a single chip, we already know that the CODEC part is there from the DT > telling us that the chip is there and what we decide is part of the > CODEC is going to depend on what the OS running on the system is doing. While it looks empty in the DT binding file, it's actually not empty once some standard properties are added to support audio-graph-card. A real world example looks like this: &cpcap { audio-codec { compatible = "motorola,cpcap-audio-codec"; #sound-dai-cells = <1>; port@0 { cpcap_audio_codec0: endpoint { remote-endpoint = <&cpu_dai2>; }; }; port@1 { cpcap_audio_codec1: endpoint { remote-endpoint = <&cpu_dai3>; }; }; }; }; Having all of this directly in the cpcap node doesn't look like an improvement for any OS. Once there is a node it makes sense to add a compatible IMHO. As a side effect this also avoids having special handling for the audio codec. This is the last missing sub-component, see arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi -- Sebastian
On Fri, Feb 16, 2018 at 02:25:38PM +0100, Sebastian Reichel wrote: > On Fri, Feb 16, 2018 at 11:30:08AM +0000, Mark Brown wrote: > > Why are we adding a separate DT node with no content for this? This is > > a single chip, we already know that the CODEC part is there from the DT > > telling us that the chip is there and what we decide is part of the > > CODEC is going to depend on what the OS running on the system is doing. > While it looks empty in the DT binding file, it's actually not empty > once some standard properties are added to support audio-graph-card. This tells me you're missing something in the binding defining the DAIs and... > A real world example looks like this: > &cpcap { > audio-codec { > compatible = "motorola,cpcap-audio-codec"; > #sound-dai-cells = <1>; ...that still doesn't require a compatible here.
Hi, On Fri, Feb 16, 2018 at 01:44:48PM +0000, Mark Brown wrote: > On Fri, Feb 16, 2018 at 02:25:38PM +0100, Sebastian Reichel wrote: > > On Fri, Feb 16, 2018 at 11:30:08AM +0000, Mark Brown wrote: > > > > Why are we adding a separate DT node with no content for this? This is > > > a single chip, we already know that the CODEC part is there from the DT > > > telling us that the chip is there and what we decide is part of the > > > CODEC is going to depend on what the OS running on the system is doing. > > > While it looks empty in the DT binding file, it's actually not empty > > once some standard properties are added to support audio-graph-card. > > This tells me you're missing something in the binding defining the > DAIs and... Well it is described by the following document: Documentation/devicetree/bindings/sound/audio-graph-card.txt Previous revision of the codec also worked perfectly fine with the simple card binding, which does the DAI stuff differently: Documentation/devicetree/bindings/sound/simple-card.txt A quick check of the other codecs suggested, that none of them descibes the graph based binding style. AFAIUI they could be used with it, though. So if you have a suggestion for a better binding document I can adopt this in the next version. > > A real world example looks like this: > > > &cpcap { > > audio-codec { > > compatible = "motorola,cpcap-audio-codec"; > > #sound-dai-cells = <1>; > > ...that still doesn't require a compatible here. I agree, that it's not required. Also the node is not required. Everything could be dumped into the main node. Many things are not required, but they make implementations easier and help in regards to DT readability and consistency. Having the compatible means, that all sub-functions _can_ be handled equally by the operating system. Not having the compatible means you _always_ need special handling for the audio codec. This basically makes the codec node different for the simple purpose of "because it is not strictly required". If we have a compatible node, other operating systems can still decide to ignore it, right? -- Sebastian
On Fri, Feb 16, 2018 at 03:12:37PM +0100, Sebastian Reichel wrote: > On Fri, Feb 16, 2018 at 01:44:48PM +0000, Mark Brown wrote: > > On Fri, Feb 16, 2018 at 02:25:38PM +0100, Sebastian Reichel wrote: > > > While it looks empty in the DT binding file, it's actually not empty > > > once some standard properties are added to support audio-graph-card. > > This tells me you're missing something in the binding defining the > > DAIs and... > Well it is described by the following document: > Documentation/devicetree/bindings/sound/audio-graph-card.txt You still need to say which DAIs exist on the device and how they are identified - if there's only one DAI it's obviously easy but if a device has multiple DAIs then there's some naming to do. > > ...that still doesn't require a compatible here. > I agree, that it's not required. Also the node is not required. > Everything could be dumped into the main node. Many things are > not required, but they make implementations easier and help in > regards to DT readability and consistency. Having the compatible > means, that all sub-functions _can_ be handled equally by the > operating system. Not having the compatible means you _always_ > need special handling for the audio codec. This basically makes > the codec node different for the simple purpose of "because it is > not strictly required". If we have a compatible node, other > operating systems can still decide to ignore it, right? It's not just other operating systems, it's also other versions of Linux we have to think about here. The most obvious issue with audio is the clocking where the division between ASoC and clock APIs is not super obvious and could easily change in the future.
* Mark Brown <broonie@kernel.org> [180216 15:17]: > On Fri, Feb 16, 2018 at 03:12:37PM +0100, Sebastian Reichel wrote: > > On Fri, Feb 16, 2018 at 01:44:48PM +0000, Mark Brown wrote: > > > On Fri, Feb 16, 2018 at 02:25:38PM +0100, Sebastian Reichel wrote: > > > > > While it looks empty in the DT binding file, it's actually not empty > > > > once some standard properties are added to support audio-graph-card. > > > > This tells me you're missing something in the binding defining the > > > DAIs and... > > > Well it is described by the following document: > > > Documentation/devicetree/bindings/sound/audio-graph-card.txt > > You still need to say which DAIs exist on the device and how they are > identified - if there's only one DAI it's obviously easy but if a device > has multiple DAIs then there's some naming to do. > > > > ...that still doesn't require a compatible here. > > > I agree, that it's not required. Also the node is not required. > > Everything could be dumped into the main node. Many things are > > not required, but they make implementations easier and help in > > regards to DT readability and consistency. Having the compatible > > means, that all sub-functions _can_ be handled equally by the > > operating system. Not having the compatible means you _always_ > > need special handling for the audio codec. This basically makes > > the codec node different for the simple purpose of "because it is > > not strictly required". If we have a compatible node, other > > operating systems can still decide to ignore it, right? > > It's not just other operating systems, it's also other versions of > Linux we have to think about here. The most obvious issue with audio is > the clocking where the division between ASoC and clock APIs is not super > obvious and could easily change in the future. Yeah let's stick to describing how the hardware is wired in the dts files. In this case it's really only the routing to the SoC, right? One advantage of using a compatible property for the pmic subdevices though is that it leaves out a dependency between various device drivers things happen automagically. The mfd core driver can be minimal and just implement interrupt handling and regmap. So no need to to parse the child nodes in the pmic mfd driver :) So personally I'd prefer the option that requires least amount of custom code if compatible vs no compatible property is the only issue here. Regards, Tony
Hi, On Fri, Feb 16, 2018 at 03:16:09PM +0000, Mark Brown wrote: > On Fri, Feb 16, 2018 at 03:12:37PM +0100, Sebastian Reichel wrote: > > On Fri, Feb 16, 2018 at 01:44:48PM +0000, Mark Brown wrote: > > > On Fri, Feb 16, 2018 at 02:25:38PM +0100, Sebastian Reichel wrote: > > > > > While it looks empty in the DT binding file, it's actually not empty > > > > once some standard properties are added to support audio-graph-card. > > > > This tells me you're missing something in the binding defining the > > > DAIs and... > > > Well it is described by the following document: > > > Documentation/devicetree/bindings/sound/audio-graph-card.txt > > You still need to say which DAIs exist on the device and how they are > identified - if there's only one DAI it's obviously easy but if a device > has multiple DAIs then there's some naming to do. Ok, I will add some more description of the codec's capabilities. > > > ...that still doesn't require a compatible here. > > > I agree, that it's not required. Also the node is not required. > > Everything could be dumped into the main node. Many things are > > not required, but they make implementations easier and help in > > regards to DT readability and consistency. Having the compatible > > means, that all sub-functions _can_ be handled equally by the > > operating system. Not having the compatible means you _always_ > > need special handling for the audio codec. This basically makes > > the codec node different for the simple purpose of "because it is > > not strictly required". If we have a compatible node, other > > operating systems can still decide to ignore it, right? > > It's not just other operating systems, it's also other versions of > Linux we have to think about here. The most obvious issue with audio is > the clocking where the division between ASoC and clock APIs is not super > obvious and could easily change in the future. Which does not change my argument. Other handling in Linux is basically equal to other operating system. We don't loose anything by having a compatible available. -- Sebastian
On Fri, Feb 16, 2018 at 07:57:07AM -0800, Tony Lindgren wrote: > * Mark Brown <broonie@kernel.org> [180216 15:17]: > > It's not just other operating systems, it's also other versions of > > Linux we have to think about here. The most obvious issue with audio is > > the clocking where the division between ASoC and clock APIs is not super > > obvious and could easily change in the future. > Yeah let's stick to describing how the hardware is wired in the > dts files. In this case it's really only the routing to the SoC, > right? The SoC and anything else on the board like external amps and so on. > One advantage of using a compatible property for the pmic subdevices > though is that it leaves out a dependency between various device > drivers things happen automagically. The mfd core driver can be > minimal and just implement interrupt handling and regmap. So no need > to to parse the child nodes in the pmic mfd driver :) There's no need to do that anyway - with a MFD the child devices can assume that they're part of the MFD and reference their parent. > So personally I'd prefer the option that requires least amount > of custom code if compatible vs no compatible property is the > only issue here. It's a few lines of code to register the child devices from code rather than the DT, and keeps it out of the ABI.
On Fri, Feb 16, 2018 at 04:58:37PM +0100, Sebastian Reichel wrote: > On Fri, Feb 16, 2018 at 03:16:09PM +0000, Mark Brown wrote: > > It's not just other operating systems, it's also other versions of > > Linux we have to think about here. The most obvious issue with audio is > > the clocking where the division between ASoC and clock APIs is not super > > obvious and could easily change in the future. > Which does not change my argument. Other handling in Linux is > basically equal to other operating system. We don't loose anything > by having a compatible available. It's bad practice to do it - we need to remember we're designing ABIs here and try to do the best job we can of them.
* Mark Brown <broonie@kernel.org> [180219 12:05]: > On Fri, Feb 16, 2018 at 07:57:07AM -0800, Tony Lindgren wrote: > > One advantage of using a compatible property for the pmic subdevices > > though is that it leaves out a dependency between various device > > drivers things happen automagically. The mfd core driver can be > > minimal and just implement interrupt handling and regmap. So no need > > to to parse the child nodes in the pmic mfd driver :) > > There's no need to do that anyway - with a MFD the child devices can > assume that they're part of the MFD and reference their parent. > > > So personally I'd prefer the option that requires least amount > > of custom code if compatible vs no compatible property is the > > only issue here. > > It's a few lines of code to register the child devices from code rather > than the DT, and keeps it out of the ABI. OK yeah that's a good point with avoiding the ABI. Seems we still want the dts child node(s) though. That way audio device can be disabled for devices where audio is not wired up at all on this PMIC. Regards, Tony
Hi, On Thu, Feb 22, 2018 at 11:54:19AM -0800, Tony Lindgren wrote: > * Mark Brown <broonie@kernel.org> [180219 12:05]: > > On Fri, Feb 16, 2018 at 07:57:07AM -0800, Tony Lindgren wrote: > > > One advantage of using a compatible property for the pmic subdevices > > > though is that it leaves out a dependency between various device > > > drivers things happen automagically. The mfd core driver can be > > > minimal and just implement interrupt handling and regmap. So no need > > > to to parse the child nodes in the pmic mfd driver :) > > > > There's no need to do that anyway - with a MFD the child devices can > > assume that they're part of the MFD and reference their parent. > > > > > So personally I'd prefer the option that requires least amount > > > of custom code if compatible vs no compatible property is the > > > only issue here. > > > > It's a few lines of code to register the child devices from code rather > > than the DT, and keeps it out of the ABI. > > OK yeah that's a good point with avoiding the ABI. Seems > we still want the dts child node(s) though. That way audio > device can be disabled for devices where audio is not wired > up at all on this PMIC. We need something to identify the correct child node. If there is no compatible, the node name will become ABI. So I don't think we gain anything by removing the compatible. As far as I can see it seems to be unusual to use fixed node names. I could find some examples, but most sub-devices are identified using compatibles. This method also has direct support in MFD (using .of_compatible in mfd_cell). Based on a bit of grepping through drivers/mfd/, the node name based identification seems to be mainly used by ASoC, while most other subsystems seem to prefer compatible based identification. Anyways, I have implemented the node name based identification and will post patches once I fixed the enum issue. I will probably send them later today. -- Sebastian
diff --git a/Documentation/devicetree/bindings/sound/motorola,cpcap-audio-codec.txt b/Documentation/devicetree/bindings/sound/motorola,cpcap-audio-codec.txt new file mode 100644 index 000000000000..a6b64132b545 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/motorola,cpcap-audio-codec.txt @@ -0,0 +1,17 @@ +Motorola CPCAP audio CODEC +-------------------------- + +This module is part of the CPCAP. For more details about the whole +chip see Documentation/devicetree/bindings/mfd/motorola-cpcap.txt. + +Required properties: + + - compatible : "motorola,cpcap-audio-codec" + +Example: + +&cpcap { + audio-codec { + compatible = "motorola,cpcap-audio-codec"; + }; +};