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/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n51
>>>
>>> genmask from 0, with a mistake:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/wcd938x-sdw.c?h=v6.11-rc1#n1255
>>>
>>> The mistake I corrected here:
>>> https://lore.kernel.org/all/20240726-asoc-wcd-wsa-swr-ports-genmask-v1-0-d4d7a8b56f05@xxxxxxxxxx/
>>>
>>> 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/ef709a0e8ab2498751305367e945df18d7a05c78#6f965d7b74e712a5cfcbc1cca407b85443a66bac_2147_2157
>>
>> 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.
Best regards,
Krzysztof
