| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200307023742.GC1094083@builder>
Date: Fri, 6 Mar 2020 18:37:42 -0800
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: agross@...nel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, robh+dt@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] arm64: dts: qcom: sdm845: Add ADSP audio support
On Thu 05 Mar 06:53 PST 2020, Srinivas Kandagatla wrote:
> This patch adds support to basic dsp audio, codec, slimbus
> and soundwire controller DT nodes.
>
I wouldn't be against the idea of splitting this patch in a few...
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 338 +++++++++++++++++++++++++++
> 1 file changed, 338 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 061f49faab19..705d8a0c3a1e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -20,6 +20,7 @@
> #include <dt-bindings/soc/qcom,rpmh-rsc.h>
> #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> #include <dt-bindings/thermal/thermal.h>
> +#include <dt-bindings/soc/qcom,apr.h>
Please keep these sorted alphabetically.
>
> / {
> interrupt-parent = <&intc>;
> @@ -491,6 +492,54 @@
> label = "lpass";
> qcom,remote-pid = <2>;
> mboxes = <&apss_shared 8>;
Please add an empty line here.
> + apr {
> + compatible = "qcom,apr-v2";
> + qcom,glink-channels = "apr_audio_svc";
> + qcom,apr-domain = <APR_DOMAIN_ADSP>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + qcom,intents = <512 20>;
> +
> + q6core {
q6core@3
Due to the reg.
> + reg = <APR_SVC_ADSP_CORE>;
> + compatible = "qcom,q6core";
Don't we want some qcom,protection-domain properties on these?
> + };
> +
> + q6afe: q6afe {
q6afe@4
> + compatible = "qcom,q6afe";
> + reg = <APR_SVC_AFE>;
> + q6afedai: dais {
> + compatible = "qcom,q6afe-dais";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #sound-dai-cells = <1>;
> +
> + qi2s@22 {
> + reg = <22>;
> + qcom,sd-lines = <0 1 2 3>;
> + };
> + };
> + };
> +
> + q6asm: q6asm {
q6asm@7
> + compatible = "qcom,q6asm";
> + reg = <APR_SVC_ASM>;
> + q6asmdai: dais {
> + compatible = "qcom,q6asm-dais";
> + #sound-dai-cells = <1>;
> + iommus = <&apps_smmu 0x1821 0x0>;
> + };
> + };
> +
> + q6adm: q6adm {
q6adm@8
Or perhaps then, as we have a unit address, these could use a generic
name and be on the form:
q6adm: apr-service@8 {
> + compatible = "qcom,q6adm";
> + reg = <APR_SVC_ADM>;
> + q6routing: routing {
> + compatible = "qcom,q6adm-routing";
> + #sound-dai-cells = <0>;
> + };
> + };
> + };
Please take the opportunity of adding an empty line here as well.
> fastrpc {
> compatible = "qcom,fastrpc";
> qcom,glink-channels = "fastrpcglink-apps-dsp";
> @@ -513,6 +562,9 @@
> };
> };
>
> + sound: sound {
> + };
> +
> cdsp_pas: remoteproc-cdsp {
> compatible = "qcom,sdm845-cdsp-pas";
>
> @@ -1782,6 +1834,142 @@
> };
> };
>
> + quat_mi2s_sleep: quat_mi2s_sleep {
Are these all board-agnostic or should they live in the board.dts files
instead?
For all of these, please replace _ with - in the node names.
> + mux {
> + pins = "gpio58", "gpio59";
> + function = "gpio";
> + };
> +
> + config {
> + pins = "gpio58", "gpio59";
> + drive-strength = <2>; /* 2 mA */
> + bias-pull-down; /* PULL DOWN */
Please omit these comments, given that the properties are quite
descriptive already.
> + input-enable;
> + };
And you don't need the subnode level these days, i.e. this can be
written as:
quat_mi2s_sleep: quat-mi2s-sleep {
pins = "gpio58", "gpio59";
function = "gpio";
drive-strength = <2>;
bias-pull-down;
input-enable;
};
> + };
> +
[..]
> @@ -2602,6 +2843,91 @@
> status = "disabled";
> };
>
> + slim_msm: slim@...c0000 {
> + compatible = "qcom,slim-ngd-v2.1.0";
> + reg = <0 0x171c0000 0 0x2C000>;
Please lowercase the digits of the size.
> + reg-names = "ctrl";
reg-names is not in binding, nor used by driver.
> + interrupts = <0 163 IRQ_TYPE_LEVEL_HIGH>;
s/0/GIC_SPI/
> +
> + qcom,apps-ch-pipes = <0x780000>;
> + qcom,ea-pc = <0x270>;
> + status = "okay";
> + dmas = <&slimbam 3>, <&slimbam 4>,
> + <&slimbam 5>, <&slimbam 6>;
> + dma-names = "rx", "tx", "tx2", "rx2";
> +
> + iommus = <&apps_smmu 0x1806 0x0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ngd@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + wcd9340_ifd: tas-ifd {
@0 given the reg, perhaps codec@0?
> + compatible = "slim217,250";
> + reg = <0 0>;
Out of curiosity, why does ngd@1 have #size-cells = <1>, but then all
codecs have size 0?
> + };
> +
> + wcd9340: codec@1{
> + pinctrl-0 = <&wcd_intr_default>;
> + pinctrl-names = "default";
> + compatible = "slim217,250";
> + reg = <1 0>;
I do prefer when the nodes start with compatible and then reg...
> + reset-gpios = <&tlmm 64 0>;
> + slim-ifc-dev = <&wcd9340_ifd>;
> +
> + #sound-dai-cells = <1>;
> +
> + interrupt-parent = <&tlmm>;
> + interrupts = <54 IRQ_TYPE_LEVEL_HIGH>;
How about combining the interrupt-parent and interrupts as:
interrupts-extended = <&tlmm 54 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + #clock-cells = <0>;
> + clock-frequency = <9600000>;
> + clock-output-names = "mclk";
> + qcom,micbias1-millivolt = <1800>;
> + qcom,micbias2-millivolt = <1800>;
> + qcom,micbias3-millivolt = <1800>;
> + qcom,micbias4-millivolt = <1800>;
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + wcdpinctrl: wcd-pinctrl@42 {
s/wcd-pinctrl/gpio-controller/
> + compatible = "qcom,wcd9340-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x42 0x2>;
> + };
> +
> + swm: swm@c85 {
> + compatible = "qcom,soundwire-v1.3.0";
> + reg = <0xc85 0x40>;
> + interrupt-parent = <&wcd9340>;
> + interrupts = <20 IRQ_TYPE_EDGE_RISING>;
interrupts-extended?
> + interrupt-names = "soundwire";
No interrupt-names in binding and driver resolves the interrupt by
index, so you can omit this.
> +
> + qcom,dout-ports = <6>;
> + qcom,din-ports = <2>;
> + qcom,ports-sinterval-low =/bits/ 8 <0x07 0x1F 0x3F 0x7 0x1F 0x3F 0x0F 0x0F>;
> + qcom,ports-offset1 = /bits/ 8 <0x01 0x02 0x0C 0x6 0x12 0x0D 0x07 0x0A >;
> + qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x1F 0x00 0x00 0x1F 0x00 0x00>;
> +
> + #sound-dai-cells = <1>;
> + clocks = <&wcd9340>;
> + clock-names = "iface";
> + #address-cells = <2>;
> + #size-cells = <0>;
Odd indentation on these two.
> +
> +
Empty lines?
> + };
> + };
> + };
> + };
> +
> usb_1_hsphy: phy@...2000 {
> compatible = "qcom,sdm845-qusb2-phy";
> reg = <0 0x088e2000 0 0x400>;
> @@ -3446,6 +3772,18 @@
> };
> };
>
> + slimbam: bamdma@...84000 {
s/bamdma/dma/
Regards,
Bjorn
> + compatible = "qcom,bam-v1.7.0";
> + qcom,controlled-remotely;
> + reg = <0 0x17184000 0 0x2a000>;
> + num-channels = <31>;
> + interrupts = <0 164 IRQ_TYPE_LEVEL_HIGH>;
s/0/GIC_SPI/
Regards,
Bjorn
> + #dma-cells = <1>;
> + qcom,ee = <1>;
> + qcom,num-ees = <2>;
> + iommus = <&apps_smmu 0x1806 0x0>;
> + };
> +
> timer@...90000 {
> #address-cells = <2>;
> #size-cells = <2>;
> --
> 2.21.0
>
Powered by blists - more mailing lists