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
