On 2023-09-18 5:04 PM, Jaroslav Kysela wrote:
On 18. 09. 23 15:55, Cezary Rojewski wrote:
...
This is for a special case when the drivers do not set snd_pcm_hw_constraint_subformats (all current drivers). In this case, the default is to handle STD and MAX subformat bits.
This constraint should be applied only one time. So this prevents to install it twice.
I believe we could avoid special-case approach. Have a copy/intersection helpers in place and utilize iterations-with-sentinel-entry. Provided such in v2 of my series.
I don't think that it's required to carry the format->subformat map in struct snd_pcm_hardware. Only few drivers will use it, so the separate constraint is fine. Also, you can remove a lot of your added code to the pcm_misc and ASoC core (copy, masking, allocating) when the affected drivers install the map using the constraint call.
I believe the question isn't how few or how many, but are there users or not. The answer to that question is: there are users of the subformat feature.
Adding an array of subformats to the snd_pcm_hardware makes things explicit, example being sound/soc/intel/avs/pcm.c. That's a win from maintenance point of view. Another thing is that we could utilize subformat to drop msbits entirely in the future. To summarize, to make subformat a first class citizen, we should avoid special-casing anything related to it.
Few points:
- PCM API interface changes should be separate, you mixing unused
helpers and separating vital functionality for drivers
What I could do is shuffle the code a bit e.g.: let snd_pcm_hw_copy() be introduced along the ASoC changes. Frankly that was the initial approach (forgotten to update the commit message of 04/17 so it still talks about code that's no longer part of the commit).
- if you copy 90% of my code, I don't think that Suggested-by: tag is fine
Could you do your work on top of my patch?
I'm afraid this isn't a fair claim. The feature is driven by validation and this has been conducted be me or my folks entirely. Given the scarce guidance provided in [1] I still provided a valid WIP in [2] and expected to iterate over it given the feedback. Closing the discussion by taking a single patch away from the series and re-authoring it is not a welcoming way to do a review. Perhaps Co-developed-by: then?
Please note that the code differs. I do believe that splitting the API and the constrains into separate patches is a better approach from maintenance point of view. Proposed readability improvements have also been applied in v2. For reasons provided in previous paragraphs, I chose to avoid the chicken-bit and treat subformat constraints in generic fashion. Also, validation shows that without updating snd_pcm_subformat_names[] in pcm.c the code ends with UBSAN during runtime. I've already addressed that, even in v1.
I'm happy to continue the technical discussion as there are still points to discuss. Let's do so in v2 of the series [3].
Kind regards, Czarek
[1]: https://lore.kernel.org/alsa-devel/337fe790-fdbc-1208-080d-5bcf9264fc65@pere... [2]: https://lore.kernel.org/alsa-devel/8d76a1d8-e85c-b699-34a0-ecea6edc2fe1@inte... [3]: https://lore.kernel.org/alsa-devel/20230918133940.3676091-1-cezary.rojewski@...