| Message ID | 20230512122838.243002-6-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add cs42l43 PC focused SoundWire CODEC | expand |
On 12/05/2023 14:28, Charles Keepax wrote: > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > for portable applications. It provides a high dynamic range, stereo > DAC for headphone output, two integrated Class D amplifiers for > loudspeakers, and two ADCs for wired headset microphone input or > stereo line input. PDM inputs are provided for digital microphones. > > Add a YAML DT binding document for this device. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > .../bindings/mfd/cirrus,cs42l43.yaml | 212 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 213 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml b/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml > new file mode 100644 > index 0000000000000..e1fd70e0a3467 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml > @@ -0,0 +1,212 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Cirrus Logic CS42L43 Audio CODEC That's audio codec, so it should be in sound, not MFD. > + > +maintainers: > + - patches@opensource.cirrus.com > + > +description: | > + The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > + (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > + for portable applications. It provides a high dynamic range, stereo > + DAC for headphone output, two integrated Class D amplifiers for > + loudspeakers, and two ADCs for wired headset microphone input or > + stereo line input. PDM inputs are provided for digital microphones. > + > +required: > + - compatible > + - reg > + - VDD_P-supply > + - VDD_A-supply > + - VDD_D-supply > + - VDD_IO-supply > + - VDD_CP-supply lowercase, no underscores in all property names. > + > +additionalProperties: false This order is quite unexpected... please do not invent your own layout. Use example-schema as your starting point. I suspect there will be many things to fix, so limited review follows (not complete). Missing ref to dai-common > + > +properties: > + compatible: > + enum: > + - cirrus,cs42l43 > + > + reg: > + maxItems: 1 > + > + VDD_P-supply: > + description: > + Power supply for the high voltage interface. > + > + VDD_A-supply: > + description: > + Power supply for internal analog circuits. > + > + VDD_D-supply: > + description: > + Power supply for internal digital circuits. > + > + VDD_IO-supply: > + description: > + Power supply for external interface and internal digital logic. > + > + VDD_CP-supply: > + description: > + Power supply for the amplifier 3 and 4 charge pump. > + > + VDD_AMP-supply: > + description: > + Power supply for amplifier 1 and 2. > + > + reset-gpios: > + maxItems: 1 > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + > + gpio-ranges: > + items: > + - description: A phandle to the CODEC pinctrl node > + minimum: 0 > + - const: 0 > + - const: 0 > + - const: 3 > + > + interrupt-controller: true > + > + '#interrupt-cells': > + const: 2 > + > + interrupts: > + maxItems: 1 > + > + '#sound-dai-cells': > + const: 1 > + > + clocks: > + items: > + - description: Synchronous audio clock provided on mclk_in. > + > + clock-names: > + const: mclk > + > + pinctrl: > + type: object additionalProperties: false > + > + allOf: > + - $ref: "../pinctrl/pinctrl.yaml#" No quotes, absolute path, so /schemas/pinctrl/.... > + > + properties: > + pin-settings: What's this node about? pinctrl/pinctrl/pins? One level too much. > + type: object > + > + additionalProperties: false > + > + patternProperties: > + '-pins$': > + type: object > + > + allOf: > + - $ref: "../pinctrl/pincfg-node.yaml#" > + - $ref: "../pinctrl/pinmux-node.yaml#" Same comments. > + > + oneOf: > + - required: [ groups ] > + - required: [ pins ] > + > + unevaluatedProperties: false > + > + properties: > + groups: > + enum: [ gpio1, gpio2, gpio3, asp, pdmout2, pdmout1, i2c, spi ] > + > + pins: > + enum: [ gpio1, gpio2, gpio3, > + asp_dout, asp_fsync, asp_bclk, > + pdmout2_clk, pdmout2_data, pdmout1_clk, pdmout1_data, > + i2c_sda, i2c_scl, > + spi_miso, spi_sck, spi_ssb ] > + > + function: > + enum: [ gpio, spdif, irq, mic-shutter, spk-shutter ] > + > + drive-strength: > + description: Set drive strength in mA > + enum: [ 1, 2, 4, 8, 9, 10, 12, 16 ] > + > + input-debounce: > + description: Set input debounce in uS > + enum: [ 0, 85 ] > + > + spi: > + type: object > + > + allOf: > + - $ref: "../spi/spi-controller.yaml#" Same comments. > + > + unevaluatedProperties: false > + > +examples: > + - | > + i2c@e0004000 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0xe0004000 0x1000>; Drop, just i2c > + > + cs42l43: codec@1a { > + compatible = "cirrus,cs42l43"; > + reg = <0x1a>; > + > + VDD_P-supply = <&vdd5v0>; > + VDD_D-supply = <&vdd1v8>; > + VDD_A-supply = <&vdd1v8>; > + VDD_IO-supply = <&vdd1v8>; > + VDD_CP-supply = <&vdd1v8>; > + VDD_AMP-supply = <&vdd5v0>; > + > + reset-gpios = <&gpio 0>; Use proper defines for flags. Best regards, Krzysztof
On 12/05/2023 14:28, Charles Keepax wrote: > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > for portable applications. It provides a high dynamic range, stereo > DAC for headphone output, two integrated Class D amplifiers for ... > + > + interrupt-controller: true > + > + '#interrupt-cells': > + const: 2 Hm, are you sure? Who is the consumer/user of this interrupt controller? > + > + interrupts: > + maxItems: 1 > + > + '#sound-dai-cells': > + const: 1 > + > + clocks: Best regards, Krzysztof
On Fri, May 12, 2023 at 05:23:22PM +0200, Krzysztof Kozlowski wrote: > On 12/05/2023 14:28, Charles Keepax wrote: > > +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic CS42L43 Audio CODEC > > That's audio codec, so it should be in sound, not MFD. > Is this true even despite the device being implemented as an MFD? I am happy to move it, and will do so unless I hear otherwise. > > + - VDD_P-supply > > + - VDD_A-supply > > + - VDD_D-supply > > + - VDD_IO-supply > > + - VDD_CP-supply > > lowercase, no underscores in all property names. I guess we can rename all the regulators to lower case. > > +additionalProperties: false > > This order is quite unexpected... please do not invent your own layout. > Use example-schema as your starting point. I suspect there will be many > things to fix, so limited review follows (not complete). > > > Missing ref to dai-common Apologies for that I was a little hesitant about this but this order did make the binding document much more readable, the intentation got really hard to follow in the traditional order. I guess since I have things working now I can put it back, again I will do so unless I hear otherwise. > > + pinctrl: > > + type: object > > additionalProperties: false > Can do. > > + > > + allOf: > > + - $ref: "../pinctrl/pinctrl.yaml#" > > No quotes, absolute path, so /schemas/pinctrl/.... > Can do. > > + > > + properties: > > + pin-settings: > > What's this node about? pinctrl/pinctrl/pins? One level too much. > codec/pinctrl/pins The device is a codec, so the main node should be called codec, then it has a subnode called pinctrl to satisfy the pinctrl DT binding. > > +examples: > > + - | > > + i2c@e0004000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0xe0004000 0x1000>; > > Drop, just i2c > Can do. > > + > > + cs42l43: codec@1a { > > + compatible = "cirrus,cs42l43"; > > + reg = <0x1a>; > > + > > + VDD_P-supply = <&vdd5v0>; > > + VDD_D-supply = <&vdd1v8>; > > + VDD_A-supply = <&vdd1v8>; > > + VDD_IO-supply = <&vdd1v8>; > > + VDD_CP-supply = <&vdd1v8>; > > + VDD_AMP-supply = <&vdd5v0>; > > + > > + reset-gpios = <&gpio 0>; > > Use proper defines for flags. Can do. Thanks, Charles
On Fri, May 12, 2023 at 05:25:52PM +0200, Krzysztof Kozlowski wrote: > On 12/05/2023 14:28, Charles Keepax wrote: > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > > for portable applications. It provides a high dynamic range, stereo > > DAC for headphone output, two integrated Class D amplifiers for > > ... > > > + > > + interrupt-controller: true > > + > > + '#interrupt-cells': > > + const: 2 > > Hm, are you sure? Who is the consumer/user of this interrupt controller? > Anyone who wants the device has GPIOs that can signal IRQs. Some of the other IRQs could be more generally useful, such as some of the jack detection ones. Thanks, Charles
On 12/05/2023 18:15, Charles Keepax wrote: > On Fri, May 12, 2023 at 05:23:22PM +0200, Krzysztof Kozlowski wrote: >> On 12/05/2023 14:28, Charles Keepax wrote: >>> +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Cirrus Logic CS42L43 Audio CODEC >> >> That's audio codec, so it should be in sound, not MFD. >> > > Is this true even despite the device being implemented as an MFD? Implementation in Linux almost does not matter here. Bindings location match the device main purpose. We indeed stuff into MFD bindings things like PMIC, because PMIC is a bit more than just regulators, and we do not have here subsystem for PMICs. If you call it Audio Codec, then I vote for sound directory for bindings. > I am happy to move it, and will do so unless I hear otherwise. > >>> + - VDD_P-supply >>> + - VDD_A-supply >>> + - VDD_D-supply >>> + - VDD_IO-supply >>> + - VDD_CP-supply >> >> lowercase, no underscores in all property names. > > I guess we can rename all the regulators to lower case. > >>> +additionalProperties: false >> >> This order is quite unexpected... please do not invent your own layout. >> Use example-schema as your starting point. I suspect there will be many >> things to fix, so limited review follows (not complete). >> >> >> Missing ref to dai-common > > Apologies for that I was a little hesitant about this but this > order did make the binding document much more readable, the > intentation got really hard to follow in the traditional order. I > guess since I have things working now I can put it back, again I > will do so unless I hear otherwise. The additional/unevaluatedProperties from child nodes are indeed moved then up - following the property: pinctrl: type: object additionalProperties: false but that's exception and for the rest I don't see any troubles with indentation. That would be the only binding... so what's here so special? > >>> + pinctrl: >>> + type: object >> >> additionalProperties: false >> > > Can do. > >>> + >>> + allOf: >>> + - $ref: "../pinctrl/pinctrl.yaml#" >> >> No quotes, absolute path, so /schemas/pinctrl/.... >> > > Can do. > >>> + >>> + properties: >>> + pin-settings: >> >> What's this node about? pinctrl/pinctrl/pins? One level too much. >> > > codec/pinctrl/pins > > The device is a codec, so the main node should be called codec, > then it has a subnode called pinctrl to satisfy the pinctrl DT > binding. Sure, but then you do not need pin-settings. Look at Qualcomm bindings for example: Documentation/devicetree/bindings/pinctrl/qcom,sm8550-tlmm.yaml Best regards, Krzysztof
On 12/05/2023 18:18, Charles Keepax wrote: > On Fri, May 12, 2023 at 05:25:52PM +0200, Krzysztof Kozlowski wrote: >> On 12/05/2023 14:28, Charles Keepax wrote: >>> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface >>> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed >>> for portable applications. It provides a high dynamic range, stereo >>> DAC for headphone output, two integrated Class D amplifiers for >> >> ... >> >>> + >>> + interrupt-controller: true >>> + >>> + '#interrupt-cells': >>> + const: 2 >> >> Hm, are you sure? Who is the consumer/user of this interrupt controller? >> > > Anyone who wants the device has GPIOs that can signal IRQs. Some > of the other IRQs could be more generally useful, such as some of > the jack detection ones. OK, makes sense, but it is a bit odd then to have: codec { which is GPIO and interrupt controller, but not pin controller pinctrl { pin controller, which is not GPIO and not interrupt controller } } Maybe all the GPIO/pin/related interrupt properties should be moved to pinctrl node? Best regards, Krzysztof
On Sat, May 13, 2023 at 08:08:05PM +0200, Krzysztof Kozlowski wrote: > On 12/05/2023 18:18, Charles Keepax wrote: > > On Fri, May 12, 2023 at 05:25:52PM +0200, Krzysztof Kozlowski wrote: > >> On 12/05/2023 14:28, Charles Keepax wrote: > >>> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > >>> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > >>> for portable applications. It provides a high dynamic range, stereo > >>> DAC for headphone output, two integrated Class D amplifiers for > >> > >> ... > >> > >>> + > >>> + interrupt-controller: true > >>> + > >>> + '#interrupt-cells': > >>> + const: 2 > >> > >> Hm, are you sure? Who is the consumer/user of this interrupt controller? > >> > > > > Anyone who wants the device has GPIOs that can signal IRQs. Some > > of the other IRQs could be more generally useful, such as some of > > the jack detection ones. > > > OK, makes sense, but it is a bit odd then to have: > codec { > which is GPIO and interrupt controller, but not pin controller > pinctrl { > pin controller, which is not GPIO and not interrupt controller > } > } > Maybe all the GPIO/pin/related interrupt properties should be moved to > pinctrl node? > I will have a look at that although I have a vague memory that this made the pinctrl very sad doing this. Presumably a bug somewhere but might take me a while to figure that out. The interrupt controller is a bit of a grey area as well since it has a bunch of IRQs that don't relate to the pins as well as some that do. So logically it might be better suited on the root node. Thanks, Charles
diff --git a/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml b/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml new file mode 100644 index 0000000000000..e1fd70e0a3467 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml @@ -0,0 +1,212 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Cirrus Logic CS42L43 Audio CODEC + +maintainers: + - patches@opensource.cirrus.com + +description: | + The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface + (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed + for portable applications. It provides a high dynamic range, stereo + DAC for headphone output, two integrated Class D amplifiers for + loudspeakers, and two ADCs for wired headset microphone input or + stereo line input. PDM inputs are provided for digital microphones. + +required: + - compatible + - reg + - VDD_P-supply + - VDD_A-supply + - VDD_D-supply + - VDD_IO-supply + - VDD_CP-supply + +additionalProperties: false + +properties: + compatible: + enum: + - cirrus,cs42l43 + + reg: + maxItems: 1 + + VDD_P-supply: + description: + Power supply for the high voltage interface. + + VDD_A-supply: + description: + Power supply for internal analog circuits. + + VDD_D-supply: + description: + Power supply for internal digital circuits. + + VDD_IO-supply: + description: + Power supply for external interface and internal digital logic. + + VDD_CP-supply: + description: + Power supply for the amplifier 3 and 4 charge pump. + + VDD_AMP-supply: + description: + Power supply for amplifier 1 and 2. + + reset-gpios: + maxItems: 1 + + gpio-controller: true + + '#gpio-cells': + const: 2 + + gpio-ranges: + items: + - description: A phandle to the CODEC pinctrl node + minimum: 0 + - const: 0 + - const: 0 + - const: 3 + + interrupt-controller: true + + '#interrupt-cells': + const: 2 + + interrupts: + maxItems: 1 + + '#sound-dai-cells': + const: 1 + + clocks: + items: + - description: Synchronous audio clock provided on mclk_in. + + clock-names: + const: mclk + + pinctrl: + type: object + + allOf: + - $ref: "../pinctrl/pinctrl.yaml#" + + properties: + pin-settings: + type: object + + additionalProperties: false + + patternProperties: + '-pins$': + type: object + + allOf: + - $ref: "../pinctrl/pincfg-node.yaml#" + - $ref: "../pinctrl/pinmux-node.yaml#" + + oneOf: + - required: [ groups ] + - required: [ pins ] + + unevaluatedProperties: false + + properties: + groups: + enum: [ gpio1, gpio2, gpio3, asp, pdmout2, pdmout1, i2c, spi ] + + pins: + enum: [ gpio1, gpio2, gpio3, + asp_dout, asp_fsync, asp_bclk, + pdmout2_clk, pdmout2_data, pdmout1_clk, pdmout1_data, + i2c_sda, i2c_scl, + spi_miso, spi_sck, spi_ssb ] + + function: + enum: [ gpio, spdif, irq, mic-shutter, spk-shutter ] + + drive-strength: + description: Set drive strength in mA + enum: [ 1, 2, 4, 8, 9, 10, 12, 16 ] + + input-debounce: + description: Set input debounce in uS + enum: [ 0, 85 ] + + spi: + type: object + + allOf: + - $ref: "../spi/spi-controller.yaml#" + + unevaluatedProperties: false + +examples: + - | + i2c@e0004000 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0xe0004000 0x1000>; + + cs42l43: codec@1a { + compatible = "cirrus,cs42l43"; + reg = <0x1a>; + + VDD_P-supply = <&vdd5v0>; + VDD_D-supply = <&vdd1v8>; + VDD_A-supply = <&vdd1v8>; + VDD_IO-supply = <&vdd1v8>; + VDD_CP-supply = <&vdd1v8>; + VDD_AMP-supply = <&vdd5v0>; + + reset-gpios = <&gpio 0>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&cs42l43_pins 0 0 3>; + + interrupt-controller; + #interrupt-cells = <2>; + interrupt-parent = <&gpio>; + interrupts = <56 8>; + + #sound-dai-cells = <1>; + + clocks = <&clks 0>; + clock-names = "mclk"; + + cs42l43_pins: pinctrl { + pinctrl-names = "default"; + pinctrl-0 = <&pinsettings>; + + pinsettings: pin-settings { + shutter-pins { + groups = "gpio3"; + function = "mic-shutter"; + }; + }; + }; + + spi { + #address-cells = <1>; + #size-cells = <0>; + + cs-gpios = <&cs42l43 1 0>; + + sensor@0 { + compatible = "bosch,bme680"; + reg = <0>; + spi-max-frequency = <1400000>; + }; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 7e0b87d5aa2e5..0db9f37eec258 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4926,6 +4926,7 @@ M: Richard Fitzgerald <rf@opensource.cirrus.com> L: alsa-devel@alsa-project.org (moderated for non-subscribers) L: patches@opensource.cirrus.com S: Maintained +F: Documentation/devicetree/bindings/mfd/cirrus,cs* F: Documentation/devicetree/bindings/sound/cirrus,cs* F: include/dt-bindings/sound/cs* F: include/sound/cs*
The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed for portable applications. It provides a high dynamic range, stereo DAC for headphone output, two integrated Class D amplifiers for loudspeakers, and two ADCs for wired headset microphone input or stereo line input. PDM inputs are provided for digital microphones. Add a YAML DT binding document for this device. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- .../bindings/mfd/cirrus,cs42l43.yaml | 212 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 213 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,cs42l43.yaml