| Message ID | 20240226-audio-i350-v1-1-4fa1cea1667f@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add audio support for the MediaTek Genio 350-evk board | expand |
On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add MT8365 audio front-end bindings > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > .../bindings/sound/mediatek,mt8365-afe.yaml | 164 +++++++++++++++++++++ > 1 file changed, 164 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml > new file mode 100644 > index 000000000000..1d7eb2532ad2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml > @@ -0,0 +1,164 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-afe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek AFE PCM controller for MT8365 > + > +maintainers: > + - Alexandre Mergnat <amergnat@baylibre.com> > + > +properties: > + compatible: > + const: mediatek,mt8365-afe-pcm > + > + reg: > + maxItems: 2 You must describe the items. > + > + interrupts: > + maxItems: 1 > + > + mediatek,topckgen: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek topckgen controller What for? Don't repeat the property name. Say something useful. > + > + power-domains: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 0 > + > + clocks: > + items: > + - description: 26M clock > + - description: mux for audio clock > + - description: audio i2s0 mck > + - description: audio i2s1 mck > + - description: audio i2s2 mck > + - description: audio i2s3 mck > + - description: engen 1 clock > + - description: engen 2 clock > + - description: audio 1 clock > + - description: audio 2 clock > + - description: mux for i2s0 > + - description: mux for i2s1 > + - description: mux for i2s2 > + - description: mux for i2s3 > + > + clock-names: > + items: > + - const: top_clk26m_clk > + - const: top_audio_sel > + - const: audio_i2s0_m > + - const: audio_i2s1_m > + - const: audio_i2s2_m > + - const: audio_i2s3_m > + - const: engen1 > + - const: engen2 > + - const: aud1 > + - const: aud2 > + - const: i2s0_m_sel > + - const: i2s1_m_sel > + - const: i2s2_m_sel > + - const: i2s3_m_sel > + > + mediatek,i2s-shared-clock: Why do you need this property? Linux (and other systems) handle sharing clocks properly. > + description: > + i2s modules can share the same external clock pin. > + If this property is not present the clock mode is separrate. Typo > + type: boolean > + > + mediatek,dmic-iir-on: > + description: > + Boolean which specifies whether the DMIC IIR is enabled. > + If this property is not present the IIR is disabled. "is enabled" or "enable it"? You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + type: boolean > + > + mediatek,dmic-irr-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Selects stop band of IIR DC-removal filter. > + 0 = Software programmable custom coeff loaded by the driver. Bindings are for hardware, not drivers. Why is this a property of board DTS? > + 1 = 5Hz if 48KHz mode. > + 2 = 10Hz if 48KHz mode. > + 3 = 25Hz if 48KHz mode. > + 4 = 50Hz if 48KHz mode. > + 5 = 65Hz if 48KHz mode. Use proper unit suffixes - hz. > + enum: > + - 0 > + - 1 > + - 2 > + - 3 > + - 4 > + - 5 > + > + mediatek,dmic-two-wire-mode: > + description: > + Boolean which turns on digital microphone for two wire mode. > + If this property is not present the two wire mode is disabled. This looks like hardware property, but the naming looks like SW. Again you instruct what driver should do. Standard disclaimer: You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + type: boolean > + > + Just one blank line. > +required: > + - compatible > + - reg > + - interrupts > + - power-domains > + - mediatek,topckgen > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/mediatek,mt8365-clk.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/mediatek,mt8365-power.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + afe@11220000 { What is AFE? https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "mediatek,mt8365-afe-pcm"; Best regards, Krzysztof
On 27/02/2024 10:01, Krzysztof Kozlowski wrote: > On 26/02/2024 15:01, Alexandre Mergnat wrote: >> Add MT8365 audio front-end bindings >> >> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> >> --- >> .../bindings/sound/mediatek,mt8365-afe.yaml | 164 +++++++++++++++++++++ >> 1 file changed, 164 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml >> new file mode 100644 >> index 000000000000..1d7eb2532ad2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml >> @@ -0,0 +1,164 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-afe.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: MediaTek AFE PCM controller for MT8365 >> + >> +maintainers: >> + - Alexandre Mergnat <amergnat@baylibre.com> >> + >> +properties: >> + compatible: >> + const: mediatek,mt8365-afe-pcm >> + >> + reg: >> + maxItems: 2 > > You must describe the items. Actually, I've took downstream code that I'm not able to explain because the second reg isn't on my MTK documentation. So I've remove the second reg and passed all functional tests successfully. Then I will remove the extra reg for v2. > >> + >> + interrupts: >> + maxItems: 1 >> + >> + mediatek,topckgen: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: The phandle of the mediatek topckgen controller > > What for? Don't repeat the property name. Say something useful. got it > >> + >> + power-domains: >> + maxItems: 1 >> + >> + "#sound-dai-cells": >> + const: 0 >> + >> + clocks: >> + items: >> + - description: 26M clock >> + - description: mux for audio clock >> + - description: audio i2s0 mck >> + - description: audio i2s1 mck >> + - description: audio i2s2 mck >> + - description: audio i2s3 mck >> + - description: engen 1 clock >> + - description: engen 2 clock >> + - description: audio 1 clock >> + - description: audio 2 clock >> + - description: mux for i2s0 >> + - description: mux for i2s1 >> + - description: mux for i2s2 >> + - description: mux for i2s3 >> + >> + clock-names: >> + items: >> + - const: top_clk26m_clk >> + - const: top_audio_sel >> + - const: audio_i2s0_m >> + - const: audio_i2s1_m >> + - const: audio_i2s2_m >> + - const: audio_i2s3_m >> + - const: engen1 >> + - const: engen2 >> + - const: aud1 >> + - const: aud2 >> + - const: i2s0_m_sel >> + - const: i2s1_m_sel >> + - const: i2s2_m_sel >> + - const: i2s3_m_sel >> + >> + mediatek,i2s-shared-clock: > > Why do you need this property? Linux (and other systems) handle sharing > clocks properly. Indeed, not needed. It was used by the downstream code driver but I remove it for v2. > >> + description: >> + i2s modules can share the same external clock pin. >> + If this property is not present the clock mode is separrate. > > Typo > >> + type: boolean >> + >> + mediatek,dmic-iir-on: >> + description: >> + Boolean which specifies whether the DMIC IIR is enabled. >> + If this property is not present the IIR is disabled. > > "is enabled" or "enable it"? > > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. I will rephrase: True to enable the Infinite Impulse Response (IIR) filter on the digital microphone inputs. > >> + type: boolean >> + >> + mediatek,dmic-irr-mode: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Selects stop band of IIR DC-removal filter. >> + 0 = Software programmable custom coeff loaded by the driver. > > Bindings are for hardware, not drivers. Why is this a property of board DTS? Actually this is a hardware feature. Mode 1 t 5 are predefined filters. Mode 0, the HW will read some "coef filter registers" to setup the custom filter. the "coef filter regs" are written by the driver. Currently the coef values are hardcoded in the driver. > >> + 1 = 5Hz if 48KHz mode. >> + 2 = 10Hz if 48KHz mode. >> + 3 = 25Hz if 48KHz mode. >> + 4 = 50Hz if 48KHz mode. >> + 5 = 65Hz if 48KHz mode. > > Use proper unit suffixes - hz. > > >> + enum: >> + - 0 >> + - 1 >> + - 2 >> + - 3 >> + - 4 >> + - 5 >> + >> + mediatek,dmic-two-wire-mode: >> + description: >> + Boolean which turns on digital microphone for two wire mode. >> + If this property is not present the two wire mode is disabled. > > This looks like hardware property, but the naming looks like SW. Again > you instruct what driver should do. Standard disclaimer: > > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. Actually this is a hardware feature. This is ALL I have to describe the HW behavior from the datasheet: " bit name: ul_dmic_two_wire_ctl Turns on digital microphone for two wire mode. 0: Turn off 1: Turn on " On the board schematic, SoC and DMIC and linked by 3 pins: - clk - data0 - data1 IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must be aware of that by reading the register value written by the driver, using the value found in the DTS. I don't get why you think it wouldn't be hardware behavior. Rephrase description: "True to enable the two wire mode of the digital microphone" Is it better ? About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? > > >> + type: boolean >> + >> + > > Just one blank line. > >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - power-domains >> + - mediatek,topckgen >> + - clocks >> + - clock-names >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/mediatek,mt8365-clk.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + #include <dt-bindings/power/mediatek,mt8365-power.h> >> + >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + afe@11220000 { > > What is AFE? Audio Front End, this is the same name used for other MTK SoC, to be consistent. Cook a new patch serie to change "afe" by "audio-controller" for all MTK SoC would be great. > > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > >> + compatible = "mediatek,mt8365-afe-pcm"; > > > Best regards, > Krzysztof >
On 27/02/2024 16:18, Alexandre Mergnat wrote: >> >>> + type: boolean >>> + >>> + mediatek,dmic-iir-on: >>> + description: >>> + Boolean which specifies whether the DMIC IIR is enabled. >>> + If this property is not present the IIR is disabled. >> >> "is enabled" or "enable it"? >> >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > > I will rephrase: > > True to enable the Infinite Impulse Response (IIR) filter > on the digital microphone inputs. I still don't know why this is DT-specific. You still tell driver what to do... > >> >>> + type: boolean >>> + >>> + mediatek,dmic-irr-mode: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Selects stop band of IIR DC-removal filter. >>> + 0 = Software programmable custom coeff loaded by the driver. >> >> Bindings are for hardware, not drivers. Why is this a property of board DTS? > > Actually this is a hardware feature. Mode 1 t 5 are predefined filters. > Mode 0, the HW will read some "coef filter registers" to setup the > custom filter. the "coef filter regs" are written by the driver. > Currently the coef values are hardcoded in the driver. You don't get the point. Just because you choose some mode it does not mean is hardware feature for DT. Sampling frequency done by hardware is also "hardware feature", but do you put it to DT? No. Explain why this is board-specific, not runtime configuration. > >> >>> + 1 = 5Hz if 48KHz mode. >>> + 2 = 10Hz if 48KHz mode. >>> + 3 = 25Hz if 48KHz mode. >>> + 4 = 50Hz if 48KHz mode. >>> + 5 = 65Hz if 48KHz mode. >> >> Use proper unit suffixes - hz. >> >> >>> + enum: >>> + - 0 >>> + - 1 >>> + - 2 >>> + - 3 >>> + - 4 >>> + - 5 >>> + >>> + mediatek,dmic-two-wire-mode: >>> + description: >>> + Boolean which turns on digital microphone for two wire mode. >>> + If this property is not present the two wire mode is disabled. >> >> This looks like hardware property, but the naming looks like SW. Again >> you instruct what driver should do. Standard disclaimer: >> >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > > Actually this is a hardware feature. This is ALL I have to describe the > HW behavior from the datasheet: > " > bit name: ul_dmic_two_wire_ctl > Turns on digital microphone for two wire mode. > 0: Turn off > 1: Turn on That's rather suggestion it is not a description of hardware but you want driver to do something... > " > > On the board schematic, SoC and DMIC and linked by 3 pins: > - clk > - data0 > - data1 > > IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must > be aware of that by reading the register value written by the driver, > using the value found in the DTS. So this depends on type of connection of DMIC? Then rephrase description property like this. > > I don't get why you think it wouldn't be hardware behavior. Because telling what to write to the registers which is typical sign of people stuffing to DT whatever they need to configure the hardware. > > Rephrase description: > "True to enable the two wire mode of the digital microphone" > Is it better ? No, because again you describe some sort of mode. If you insist on such description, then my answer is: it's runtime, so not suitable for DT. Instead describe what is the hardware problem/configuration, e.g. "DMIC is connected with only CLK and DATA0, without third pin" etc. > > About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? To sound more like a register less like physical characteristic of the board? No. The name can stay, I don't have better ideas. Best regards, Krzysztof
I think I got it. - mediatek,i2s-shared-clock: will be remove from DT - mediatek,dmic-iir-on: will be remove from DT - mediatek,dmic-irr-mode: will be remove from DT - mediatek,dmic-two-wire-mode: rephrase description needed I've did abstraction (despite me) that IIR settings are runtime config because the driver implement its usage like a one-time-setup -_-' Thanks for the explanations, that help. Regards, Alexandre On 28/02/2024 08:28, Krzysztof Kozlowski wrote: > On 27/02/2024 16:18, Alexandre Mergnat wrote: >>> >>>> + type: boolean >>>> + >>>> + mediatek,dmic-iir-on: >>>> + description: >>>> + Boolean which specifies whether the DMIC IIR is enabled. >>>> + If this property is not present the IIR is disabled. >>> >>> "is enabled" or "enable it"? >>> >>> You described the desired Linux feature or behavior, not the actual >>> hardware. The bindings are about the latter, so instead you need to >>> rephrase the property and its description to match actual hardware >>> capabilities/features/configuration etc. >> >> I will rephrase: >> >> True to enable the Infinite Impulse Response (IIR) filter >> on the digital microphone inputs. > > I still don't know why this is DT-specific. You still tell driver what > to do... > >> >>> >>>> + type: boolean >>>> + >>>> + mediatek,dmic-irr-mode: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + Selects stop band of IIR DC-removal filter. >>>> + 0 = Software programmable custom coeff loaded by the driver. >>> >>> Bindings are for hardware, not drivers. Why is this a property of board DTS? >> >> Actually this is a hardware feature. Mode 1 t 5 are predefined filters. >> Mode 0, the HW will read some "coef filter registers" to setup the >> custom filter. the "coef filter regs" are written by the driver. >> Currently the coef values are hardcoded in the driver. > > You don't get the point. Just because you choose some mode it does not > mean is hardware feature for DT. Sampling frequency done by hardware is > also "hardware feature", but do you put it to DT? No. > > Explain why this is board-specific, not runtime configuration. > >> >>> >>>> + 1 = 5Hz if 48KHz mode. >>>> + 2 = 10Hz if 48KHz mode. >>>> + 3 = 25Hz if 48KHz mode. >>>> + 4 = 50Hz if 48KHz mode. >>>> + 5 = 65Hz if 48KHz mode. >>> >>> Use proper unit suffixes - hz. >>> >>> >>>> + enum: >>>> + - 0 >>>> + - 1 >>>> + - 2 >>>> + - 3 >>>> + - 4 >>>> + - 5 >>>> + >>>> + mediatek,dmic-two-wire-mode: >>>> + description: >>>> + Boolean which turns on digital microphone for two wire mode. >>>> + If this property is not present the two wire mode is disabled. >>> >>> This looks like hardware property, but the naming looks like SW. Again >>> you instruct what driver should do. Standard disclaimer: >>> >>> You described the desired Linux feature or behavior, not the actual >>> hardware. The bindings are about the latter, so instead you need to >>> rephrase the property and its description to match actual hardware >>> capabilities/features/configuration etc. >> >> Actually this is a hardware feature. This is ALL I have to describe the >> HW behavior from the datasheet: >> " >> bit name: ul_dmic_two_wire_ctl >> Turns on digital microphone for two wire mode. >> 0: Turn off >> 1: Turn on > > That's rather suggestion it is not a description of hardware but you > want driver to do something... > >> " >> >> On the board schematic, SoC and DMIC and linked by 3 pins: >> - clk >> - data0 >> - data1 >> >> IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must >> be aware of that by reading the register value written by the driver, >> using the value found in the DTS. > > So this depends on type of connection of DMIC? Then rephrase description > property like this. > >> >> I don't get why you think it wouldn't be hardware behavior. > > Because telling what to write to the registers which is typical sign of > people stuffing to DT whatever they need to configure the hardware. > >> >> Rephrase description: >> "True to enable the two wire mode of the digital microphone" >> Is it better ? > > No, because again you describe some sort of mode. If you insist on such > description, then my answer is: it's runtime, so not suitable for DT. > Instead describe what is the hardware problem/configuration, e.g. "DMIC > is connected with only CLK and DATA0, without third pin" etc. > >> >> About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? > > To sound more like a register less like physical characteristic of the > board? No. The name can stay, I don't have better ideas. > > > Best regards, > Krzysztof >
Il 28/02/24 10:57, Alexandre Mergnat ha scritto: > I think I got it. > > - mediatek,i2s-shared-clock: will be remove from DT > - mediatek,dmic-iir-on: will be remove from DT > - mediatek,dmic-irr-mode: will be remove from DT > - mediatek,dmic-two-wire-mode: rephrase description needed > > I've did abstraction (despite me) that IIR settings are runtime config because the > driver implement its usage like a one-time-setup -_-' > Yes but just one more thing I just noticed: `mediatek,dmic-two-wire-mode` - can we please rename this to `mediatek,dmic-mode` ? That'd be for consistency check mt6359.yaml and mt6358.txt mediatek,dmic-mode: $ref: /schemas/types.yaml#/definitions/uint32 description: | Indicates how many data pins are used to transmit two channels of PDM signal. 0 means two wires, 1 means one wire. Default value is 0. enum: - 0 # one wire - 1 # two wires Cheers, Angelo > Thanks for the explanations, that help. > > Regards, > Alexandre > > On 28/02/2024 08:28, Krzysztof Kozlowski wrote: >> On 27/02/2024 16:18, Alexandre Mergnat wrote: >>>> >>>>> + type: boolean >>>>> + >>>>> + mediatek,dmic-iir-on: >>>>> + description: >>>>> + Boolean which specifies whether the DMIC IIR is enabled. >>>>> + If this property is not present the IIR is disabled. >>>> >>>> "is enabled" or "enable it"? >>>> >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> >>> I will rephrase: >>> >>> True to enable the Infinite Impulse Response (IIR) filter >>> on the digital microphone inputs. >> >> I still don't know why this is DT-specific. You still tell driver what >> to do... >> >>> >>>> >>>>> + type: boolean >>>>> + >>>>> + mediatek,dmic-irr-mode: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: >>>>> + Selects stop band of IIR DC-removal filter. >>>>> + 0 = Software programmable custom coeff loaded by the driver. >>>> >>>> Bindings are for hardware, not drivers. Why is this a property of board DTS? >>> >>> Actually this is a hardware feature. Mode 1 t 5 are predefined filters. >>> Mode 0, the HW will read some "coef filter registers" to setup the >>> custom filter. the "coef filter regs" are written by the driver. >>> Currently the coef values are hardcoded in the driver. >> >> You don't get the point. Just because you choose some mode it does not >> mean is hardware feature for DT. Sampling frequency done by hardware is >> also "hardware feature", but do you put it to DT? No. >> >> Explain why this is board-specific, not runtime configuration. >> >>> >>>> >>>>> + 1 = 5Hz if 48KHz mode. >>>>> + 2 = 10Hz if 48KHz mode. >>>>> + 3 = 25Hz if 48KHz mode. >>>>> + 4 = 50Hz if 48KHz mode. >>>>> + 5 = 65Hz if 48KHz mode. >>>> >>>> Use proper unit suffixes - hz. >>>> >>>> >>>>> + enum: >>>>> + - 0 >>>>> + - 1 >>>>> + - 2 >>>>> + - 3 >>>>> + - 4 >>>>> + - 5 >>>>> + >>>>> + mediatek,dmic-two-wire-mode: >>>>> + description: >>>>> + Boolean which turns on digital microphone for two wire mode. >>>>> + If this property is not present the two wire mode is disabled. >>>> >>>> This looks like hardware property, but the naming looks like SW. Again >>>> you instruct what driver should do. Standard disclaimer: >>>> >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> >>> Actually this is a hardware feature. This is ALL I have to describe the >>> HW behavior from the datasheet: >>> " >>> bit name: ul_dmic_two_wire_ctl >>> Turns on digital microphone for two wire mode. >>> 0: Turn off >>> 1: Turn on >> >> That's rather suggestion it is not a description of hardware but you >> want driver to do something... >> >>> " >>> >>> On the board schematic, SoC and DMIC and linked by 3 pins: >>> - clk >>> - data0 >>> - data1 >>> >>> IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must >>> be aware of that by reading the register value written by the driver, >>> using the value found in the DTS. >> >> So this depends on type of connection of DMIC? Then rephrase description >> property like this. >> >>> >>> I don't get why you think it wouldn't be hardware behavior. >> >> Because telling what to write to the registers which is typical sign of >> people stuffing to DT whatever they need to configure the hardware. >> >>> >>> Rephrase description: >>> "True to enable the two wire mode of the digital microphone" >>> Is it better ? >> >> No, because again you describe some sort of mode. If you insist on such >> description, then my answer is: it's runtime, so not suitable for DT. >> Instead describe what is the hardware problem/configuration, e.g. "DMIC >> is connected with only CLK and DATA0, without third pin" etc. >> >>> >>> About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? >> >> To sound more like a register less like physical characteristic of the >> board? No. The name can stay, I don't have better ideas. >> >> >> Best regards, >> Krzysztof >> >
On 28/02/2024 11:25, AngeloGioacchino Del Regno wrote: > Il 28/02/24 10:57, Alexandre Mergnat ha scritto: >> I think I got it. >> >> - mediatek,i2s-shared-clock: will be remove from DT >> - mediatek,dmic-iir-on: will be remove from DT >> - mediatek,dmic-irr-mode: will be remove from DT >> - mediatek,dmic-two-wire-mode: rephrase description needed >> >> I've did abstraction (despite me) that IIR settings are runtime config >> because the driver implement its usage like a one-time-setup -_-' >> > > Yes but just one more thing I just noticed: > `mediatek,dmic-two-wire-mode` - can we > please rename this to `mediatek,dmic-mode` ? Sure, I wasn't aware of this property. I will do it. Note: the description isn't consistent with the enum comments " 0 means two wires, 1 means one wire. ... - 0 # one wire - 1 # two wires " > > That'd be for consistency check mt6359.yaml and mt6358.txt > > mediatek,dmic-mode: > $ref: /schemas/types.yaml#/definitions/uint32 > description: | > Indicates how many data pins are used to transmit two channels of > PDM > signal. 0 means two wires, 1 means one wire. Default value is 0. > enum: > - 0 # one wire > - 1 # two wires > > Cheers, > Angelo > > > >> Thanks for the explanations, that help. >> >> Regards, >> Alexandre >> >> On 28/02/2024 08:28, Krzysztof Kozlowski wrote: >>> On 27/02/2024 16:18, Alexandre Mergnat wrote: >>>>> >>>>>> + type: boolean >>>>>> + >>>>>> + mediatek,dmic-iir-on: >>>>>> + description: >>>>>> + Boolean which specifies whether the DMIC IIR is enabled. >>>>>> + If this property is not present the IIR is disabled. >>>>> >>>>> "is enabled" or "enable it"? >>>>> >>>>> You described the desired Linux feature or behavior, not the actual >>>>> hardware. The bindings are about the latter, so instead you need to >>>>> rephrase the property and its description to match actual hardware >>>>> capabilities/features/configuration etc. >>>> >>>> I will rephrase: >>>> >>>> True to enable the Infinite Impulse Response (IIR) filter >>>> on the digital microphone inputs. >>> >>> I still don't know why this is DT-specific. You still tell driver what >>> to do... >>> >>>> >>>>> >>>>>> + type: boolean >>>>>> + >>>>>> + mediatek,dmic-irr-mode: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: >>>>>> + Selects stop band of IIR DC-removal filter. >>>>>> + 0 = Software programmable custom coeff loaded by the driver. >>>>> >>>>> Bindings are for hardware, not drivers. Why is this a property of >>>>> board DTS? >>>> >>>> Actually this is a hardware feature. Mode 1 t 5 are predefined filters. >>>> Mode 0, the HW will read some "coef filter registers" to setup the >>>> custom filter. the "coef filter regs" are written by the driver. >>>> Currently the coef values are hardcoded in the driver. >>> >>> You don't get the point. Just because you choose some mode it does not >>> mean is hardware feature for DT. Sampling frequency done by hardware is >>> also "hardware feature", but do you put it to DT? No. >>> >>> Explain why this is board-specific, not runtime configuration. >>> >>>> >>>>> >>>>>> + 1 = 5Hz if 48KHz mode. >>>>>> + 2 = 10Hz if 48KHz mode. >>>>>> + 3 = 25Hz if 48KHz mode. >>>>>> + 4 = 50Hz if 48KHz mode. >>>>>> + 5 = 65Hz if 48KHz mode. >>>>> >>>>> Use proper unit suffixes - hz. >>>>> >>>>> >>>>>> + enum: >>>>>> + - 0 >>>>>> + - 1 >>>>>> + - 2 >>>>>> + - 3 >>>>>> + - 4 >>>>>> + - 5 >>>>>> + >>>>>> + mediatek,dmic-two-wire-mode: >>>>>> + description: >>>>>> + Boolean which turns on digital microphone for two wire mode. >>>>>> + If this property is not present the two wire mode is disabled. >>>>> >>>>> This looks like hardware property, but the naming looks like SW. Again >>>>> you instruct what driver should do. Standard disclaimer: >>>>> >>>>> You described the desired Linux feature or behavior, not the actual >>>>> hardware. The bindings are about the latter, so instead you need to >>>>> rephrase the property and its description to match actual hardware >>>>> capabilities/features/configuration etc. >>>> >>>> Actually this is a hardware feature. This is ALL I have to describe the >>>> HW behavior from the datasheet: >>>> " >>>> bit name: ul_dmic_two_wire_ctl >>>> Turns on digital microphone for two wire mode. >>>> 0: Turn off >>>> 1: Turn on >>> >>> That's rather suggestion it is not a description of hardware but you >>> want driver to do something... >>> >>>> " >>>> >>>> On the board schematic, SoC and DMIC and linked by 3 pins: >>>> - clk >>>> - data0 >>>> - data1 >>>> >>>> IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC >>>> must >>>> be aware of that by reading the register value written by the driver, >>>> using the value found in the DTS. >>> >>> So this depends on type of connection of DMIC? Then rephrase description >>> property like this. >>> >>>> >>>> I don't get why you think it wouldn't be hardware behavior. >>> >>> Because telling what to write to the registers which is typical sign of >>> people stuffing to DT whatever they need to configure the hardware. >>> >>>> >>>> Rephrase description: >>>> "True to enable the two wire mode of the digital microphone" >>>> Is it better ? >>> >>> No, because again you describe some sort of mode. If you insist on such >>> description, then my answer is: it's runtime, so not suitable for DT. >>> Instead describe what is the hardware problem/configuration, e.g. "DMIC >>> is connected with only CLK and DATA0, without third pin" etc. >>> >>>> >>>> About the property name, "mediatek,dmic-two-wire-ctl" sound better >>>> for you ? >>> >>> To sound more like a register less like physical characteristic of the >>> board? No. The name can stay, I don't have better ideas. >>> >>> >>> Best regards, >>> Krzysztof >>> >> >
On 28/02/2024 11:25, AngeloGioacchino Del Regno wrote: > Il 28/02/24 10:57, Alexandre Mergnat ha scritto: >> I think I got it. >> >> - mediatek,i2s-shared-clock: will be remove from DT >> - mediatek,dmic-iir-on: will be remove from DT >> - mediatek,dmic-irr-mode: will be remove from DT >> - mediatek,dmic-two-wire-mode: rephrase description needed >> >> I've did abstraction (despite me) that IIR settings are runtime config because the >> driver implement its usage like a one-time-setup -_-' >> > > Yes but just one more thing I just noticed: `mediatek,dmic-two-wire-mode` - can we > please rename this to `mediatek,dmic-mode` ? > > That'd be for consistency check mt6359.yaml and mt6358.txt > > mediatek,dmic-mode: > $ref: /schemas/types.yaml#/definitions/uint32 > description: | > Indicates how many data pins are used to transmit two channels of PDM > signal. 0 means two wires, 1 means one wire. Default value is 0. > enum: > - 0 # one wire > - 1 # two wires Thanks for checking. Now I wonder if entire binding just ignored existing work and started doing things from scratch... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml new file mode 100644 index 000000000000..1d7eb2532ad2 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml @@ -0,0 +1,164 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-afe.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MediaTek AFE PCM controller for MT8365 + +maintainers: + - Alexandre Mergnat <amergnat@baylibre.com> + +properties: + compatible: + const: mediatek,mt8365-afe-pcm + + reg: + maxItems: 2 + + interrupts: + maxItems: 1 + + mediatek,topckgen: + $ref: /schemas/types.yaml#/definitions/phandle + description: The phandle of the mediatek topckgen controller + + power-domains: + maxItems: 1 + + "#sound-dai-cells": + const: 0 + + clocks: + items: + - description: 26M clock + - description: mux for audio clock + - description: audio i2s0 mck + - description: audio i2s1 mck + - description: audio i2s2 mck + - description: audio i2s3 mck + - description: engen 1 clock + - description: engen 2 clock + - description: audio 1 clock + - description: audio 2 clock + - description: mux for i2s0 + - description: mux for i2s1 + - description: mux for i2s2 + - description: mux for i2s3 + + clock-names: + items: + - const: top_clk26m_clk + - const: top_audio_sel + - const: audio_i2s0_m + - const: audio_i2s1_m + - const: audio_i2s2_m + - const: audio_i2s3_m + - const: engen1 + - const: engen2 + - const: aud1 + - const: aud2 + - const: i2s0_m_sel + - const: i2s1_m_sel + - const: i2s2_m_sel + - const: i2s3_m_sel + + mediatek,i2s-shared-clock: + description: + i2s modules can share the same external clock pin. + If this property is not present the clock mode is separrate. + type: boolean + + mediatek,dmic-iir-on: + description: + Boolean which specifies whether the DMIC IIR is enabled. + If this property is not present the IIR is disabled. + type: boolean + + mediatek,dmic-irr-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Selects stop band of IIR DC-removal filter. + 0 = Software programmable custom coeff loaded by the driver. + 1 = 5Hz if 48KHz mode. + 2 = 10Hz if 48KHz mode. + 3 = 25Hz if 48KHz mode. + 4 = 50Hz if 48KHz mode. + 5 = 65Hz if 48KHz mode. + enum: + - 0 + - 1 + - 2 + - 3 + - 4 + - 5 + + mediatek,dmic-two-wire-mode: + description: + Boolean which turns on digital microphone for two wire mode. + If this property is not present the two wire mode is disabled. + type: boolean + + +required: + - compatible + - reg + - interrupts + - power-domains + - mediatek,topckgen + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/mediatek,mt8365-clk.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/power/mediatek,mt8365-power.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + afe@11220000 { + compatible = "mediatek,mt8365-afe-pcm"; + reg = <0 0x11220000 0 0x1000>, + <0 0x11221000 0 0xA000>; + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_LOW>; + power-domains = <&spm MT8365_POWER_DOMAIN_AUDIO>; + mediatek,topckgen = <&topckgen>; + #sound-dai-cells = <0>; + clocks = <&clk26m>, + <&topckgen CLK_TOP_AUDIO_SEL>, + <&topckgen CLK_TOP_AUD_I2S0_M>, + <&topckgen CLK_TOP_AUD_I2S1_M>, + <&topckgen CLK_TOP_AUD_I2S2_M>, + <&topckgen CLK_TOP_AUD_I2S3_M>, + <&topckgen CLK_TOP_AUD_ENGEN1_SEL>, + <&topckgen CLK_TOP_AUD_ENGEN2_SEL>, + <&topckgen CLK_TOP_AUD_1_SEL>, + <&topckgen CLK_TOP_AUD_2_SEL>, + <&topckgen CLK_TOP_APLL_I2S0_SEL>, + <&topckgen CLK_TOP_APLL_I2S1_SEL>, + <&topckgen CLK_TOP_APLL_I2S2_SEL>, + <&topckgen CLK_TOP_APLL_I2S3_SEL>; + clock-names = "top_clk26m_clk", + "top_audio_sel", + "audio_i2s0_m", + "audio_i2s1_m", + "audio_i2s2_m", + "audio_i2s3_m", + "engen1", + "engen2", + "aud1", + "aud2", + "i2s0_m_sel", + "i2s1_m_sel", + "i2s2_m_sel", + "i2s3_m_sel"; + }; + }; + +...
Add MT8365 audio front-end bindings Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- .../bindings/sound/mediatek,mt8365-afe.yaml | 164 +++++++++++++++++++++ 1 file changed, 164 insertions(+)