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 Mon, Feb 26, 2024 at 03:01:38PM +0100, Alexandre Mergnat wrote:
> This serie aim to add the following audio support for the Genio 350-evk:
> - Playback
> - 2ch Headset Jack (Earphone)
> - 1ch Line-out Jack (Speaker)
> - 8ch HDMI Tx
> - Capture
> - 1ch DMIC (On-board Digital Microphone)
> - 1ch AMIC (On-board Analogic Microphone)
> - 1ch Headset Jack (External Analogic Microphone)
>
> Of course, HDMI playback need the MT8365 display patches [1] and a DTS
> change documented in "mediatek,mt8365-mt6357.yaml".
Given the number of custom controls here could you please post the
output of mixer-test and pcm-test from a system with this driver running
next time you post, this will help with review since it checks a bunch
of things around the new controls.
On 27/02/2024 11:23, Alexandre Mergnat wrote:
>>> +
>>> +examples:
>>> + - |
>>> + sound {
>>> + compatible = "mediatek,mt8365-mt6357";
>>> + mediatek,platform = <&afe>;
>>
>> Please:
>>
>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>
> Is it about the wrong pinctrl-names tab alignment ?
> Also, 2ND I2S BE => 2ND_I2S_BE ?
> Otherwise, I don't get it sorry.
Alignment of continued lines, order of properties.
Best regards,
Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add Digital Micro Device Audio Interface support for MT8365 SoC.
>
> Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com>
> +
> +static int init_dmic_priv_data(struct mtk_base_afe *afe)
> +{
> + struct mt8365_afe_private *afe_priv = afe->platform_priv;
> + struct mt8365_dmic_data *dmic_priv;
> + struct device_node *np = afe->dev->of_node;
> + unsigned int temps[4];
> + int ret;
> +
> + dmic_priv = devm_kzalloc(afe->dev, sizeof(struct mt8365_dmic_data),
> + GFP_KERNEL);
You have very inconsistent style of coding. Some patches are done
correctly, some repeast known issues. All over. This is sizeof(*). This
comment (and all others) apply everywhere, just in case.
Best regards,
Krzysztof
On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add the codec property along with the mt6357.c codec driver support.
Describe the hardware, not the Linux drivers. There is no codec driver
support in the bindings.
https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b…
>
> Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com>
> ---
> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> index 37423c2e0fdf..d25a78070744 100644
> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml
> @@ -37,6 +37,17 @@ properties:
> "#interrupt-cells":
> const: 2
>
> + codec:
> + type: object
> + unevaluatedProperties: false
> + description:
> + MT6357 sound codec.
> + properties:
> + compatible:
> + const: mediatek,mt6357-sound
> + required:
> + - compatible
No resources? Then no need for this node.
We have it even documented (if my repeating every time is not enough)...
https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b…
Best regards,
Krzysztof