On 02/03/2021 14:29, Pierre-Louis Bossart wrote:
for (i = 0; i < nports; i++) { ctrl->pconfig[i].si = si[i]; ctrl->pconfig[i].off1 = off1[i]; ctrl->pconfig[i].off2 = off2[i]; ctrl->pconfig[i].bp_mode = bp_mode[i]; + ctrl->pconfig[i].hstart = hstart[i]; + ctrl->pconfig[i].hstop = hstop[i]; + ctrl->pconfig[i].word_length = word_length[i]; + ctrl->pconfig[i].blk_group_count = blk_group_count[i]; + ctrl->pconfig[i].lane_control = lane_control[i]; }
I don't get why you test the values parsed from DT before writing the registers. Why do test them here? if some values are incorrect it's much better to provide an error log instead of writing a partially valid setup to hardware, no?
from DT we pass parameters for all the master ports, however some of these parameters are not really applicable for some of the ports! so the way we handle this is by marking them as 0xFF which means these values are not applicable for those ports! Having said that I think I should probably redefine SWR_INVALID_PARAM to QCOM_SWR_PARAM_NA or something on those lines!
Humm, do you have an example here? It's a bit odd to define DT
In DT we describe parameters for each port in an array so, parameters for ports that are not applicable/available for that platform can be marked with 0xFF.
Most importantly, Some of these registers are only implemented based on the data ports. Ex: GROUP_CONTROL register is only implemented for data ports that support Group Count other than 0. Like wise for HSTART/STOP for Full Data ports only! So avoiding reading/writing to registers by passing invalid/not-applicable value 0xFF made more sense!
Here are some examples of controller instances on SM8250 SoC.
soundwire-controller@3230000 { reg = <0 0x3230000 0 0x2000>; compatible = "qcom,soundwire-v1.5.1"; interrupts-extended = <&intc GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>, <&pdc 109 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "core", "wakeup";
qcom,clock-stop-mode0; clocks = <&txmacro>; clock-names = "iface";
qcom,din-ports = <5>; qcom,dout-ports = <0>; qcom,ports-sinterval-low = /bits/ 8 <0xFF 0x01 0x01 0x03 0x03>; qcom,ports-offset1 = /bits/ 8 <0xFF 0x01 0x00 0x02 0x00>; qcom,ports-offset2 = /bits/ 8 <0xFF 0x00 0x00 0x00 0x00>; qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>; qcom,ports-hstart = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>; qcom,ports-hstop = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>; qcom,ports-word-length = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>; qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>; qcom,ports-lane-control = /bits/ 8 <0xFF 0x00 0x01 0x00 0x01>; qcom,port-offset = <1>; #sound-dai-cells = <1>; #address-cells = <2>; #size-cells = <0>; };
soundwire-controller@3210000 { reg = <0 0x3210000 0 0x2000>; compatible = "qcom,soundwire-v1.5.1"; interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>; clocks = <&rxmacro>; clock-names = "iface"; qcom,clock-stop-mode0; qcom,din-ports = <0>; qcom,dout-ports = <5>;
qcom,ports-sinterval-low = /bits/ 8 <0x03 0x1F 0x1F 0x07 0x00>; qcom,ports-offset1 = /bits/ 8 <0x00 0x00 0x0B 0x01 0x00>; qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x0B 0x00 0x00>; qcom,ports-hstart = /bits/ 8 <0xFF 0x03 0xFF 0xFF 0xFF>; qcom,ports-hstop = /bits/ 8 <0xFF 0x06 0xFF 0xFF 0xFF>; qcom,ports-word-length = /bits/ 8 <0x01 0x07 0x04 0xFF 0xFF>; qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0x00 0x01 0xFF 0xFF>; qcom,ports-lane-control = /bits/ 8 <0x01 0x00 0x00 0x00 0x00>; qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0x00>;
#sound-dai-cells = <1>; #address-cells = <2>; #size-cells = <0>; };
properties that may or may not be valid. If this is intentional and desired, this should still be captured somehow, e.g. in the bindings documentation or in the code with a comment, no?
Yes, I agree with you on this, I should document this in bindings!
--srini