On 7/30/24 11:19, Krzysztof Kozlowski wrote:
On 30/07/2024 10:59, Pierre-Louis Bossart wrote:
/* Read dpn properties for source port(s) */ sdw_slave_read_dpn(slave, prop->src_dpn_prop, nval, prop->source_ports, "source");
IOW, this is a valid change, but it's an optimization, not a fix in the usual sense of 'kernel oops otherwise'.
Am I missing something?
BTW, the notion of DPn is that n > 0. DP0 is a special case with different properties, BIT(0) cannot be set for either of the sink/source port bitmask.
I think we speak about two different things. port num > 1, that's correct. But index for src_dpn_prop array is something different. Look at mipi-disco sdw_slave_read_dpn():
173 u32 bit, i = 0; ... 178 addr = ports; 179 /* valid ports are 1 to 14 so apply mask */ 180 addr &= GENMASK(14, 1); 181 182 for_each_set_bit(bit, &addr, 32) { ... 186 dpn[i].num = bit;
so dpn[0..i] = 1..n where i is also the bit in the mask.
yes, agreed on the indexing.
But are we in agreement that the case of non-contiguous ports would not create any issues? the existing code is not efficient but it wouldn't crash, would it?
There are multiple cases of non-contiguous ports, I am not aware of any issues...
rt700-sdw.c: prop->source_ports = 0x14; /* BITMAP: 00010100 */ rt711-sdca-sdw.c: prop->source_ports = 0x14; /* BITMAP: 00010100 rt712-sdca-sdw.c: prop->source_ports = BIT(8) | BIT(4); rt715-sdca-sdw.c: prop->source_ports = 0x50;/* BITMAP: 01010000 */ rt722-sdca-sdw.c: prop->source_ports = BIT(6) | BIT(2); /* BITMAP: 01000100 */
same for sinks:
rt712-sdca-sdw.c: prop->sink_ports = BIT(3) | BIT(1); /* BITMAP: 00001010 */ rt722-sdca-sdw.c: prop->sink_ports = BIT(3) | BIT(1); /* BITMAP: 00001010 */
All these work because they have separate source and sink dpn_prop arrays. Separate arrays, separate number of ports, separate masks - all this is good. Now going to my code...
Similar implementation was done in Qualcomm wsa and wcd codecs like: array indexed from 0: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
genmask from 0, with a mistake: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...
The mistake I corrected here: https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d...
To summarize, the mask does not denote port numbers (1...14) but indices of the dpn array which are from 0..whatever (usually -1 from port number).
Let me also complete this with a real life example of my work in progress. I want to use same dpn_prop array for sink and source ports and use different masks. The code in progress is:
https://git.codelinaro.org/krzysztof.kozlowski/linux/-/commit/ef709a0e8ab249...
Without this patch, I get -EINVAL from sdw_get_slave_dpn_prop(): soundwire sdw-master-1-0: Program transport params failed: -2
Not following, sorry. The sink and source masks are separate on purpose, to allow for bi-directional ports. The SoundWire spec allows a port to be configured at run-time either as source or sink. In practice I've never seen this happen, all existing hardware relies on ports where the direction is hard-coded/fixed, but still we want to follow the spec.
The ports are indeed hard-coded/fixed.
So if ports can be either source or sink, I am not sure how the properties could be shared with a single array?
Because I could, just easier to code. :) Are you saying the code is not correct? If I understand the concept of source/sink dpn port mask, it should be correct. I have some array with source and sink ports. I pass it to Soundwire with a mask saying which ports are source and which are sink.
Those two lines aren't clear to me at all:
pdev->prop.sink_dpn_prop = wsa884x_sink_dpn_prop; pdev->prop.src_dpn_prop = wsa884x_sink_dpn_prop;
I could do: s/wsa884x_sink_dpn_prop/wsa884x_dpn_prop/ and expect the code to be correct.
Ah I think I see what you are trying to do, you have a single dpn_prop array but each entry is valid for either sink or source depending on the sink / source_mask which don't overlap.
Did I get this right?