On 2023-09-19 12:07 PM, Jaroslav Kysela wrote:
On 19. 09. 23 11:28, Cezary Rojewski wrote:
...
- 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?
I don't follow you. My patch can be also changed - I've not heard any review except cosmetic changes. I am just telling you that the patch is a good base for all other changes. I think that the best way is to finish the base extension in sound/core at first without any helpers and so on and then work on other parts.
(save)
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.
It does not make sense to extend API without constraints. The splitting does not help here.
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
Yes, I missed that. I can put it to my v3 when we agree on the constraints.
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].
Unfortunately, you are not working with the technical discussion anyway. Changing comments adding empty lines, renaming variables to make you happy is not a nice way to co-operate with others and then act as the author of the CODE (not comments) is really bad. Everyone has own coding style and you're forcing your opinion.
We have a different view on the subject. Readability shall be treated on the same level as performance is. It should not be marginalized.
When taking a look at RFC series [1] and the WIP [2] there's a clear resemblance to the code here. I believe the differences between this and v2 [3] are also being skipped over. And as stated previously, closing the review by separating a patch from the series and sending it without any references to the original series and modifying the CC list is not nice.
What I propose is: leave the MSBITS-introduction and MSBITS-constraints patches split. Have my authorship for the former and yours for the latter with relevant Co-developed-by: tags added in both of them. Is this proposal acceptable from your perspective?
Kind regards, Czarek
[1]: https://lore.kernel.org/alsa-devel/20230811164853.1797547-1-cezary.rojewski@... [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@...
Also, I though about {} end-of-array mark (remove SNDRV_PCM_FORMAT_CONSTRAINT_END), but I found that I prefer to have the possibility to skip the MSBITS_MAX settings for the given format. It may make sense in a situation when the MSBITS configuration is too rare to be added as the API bit.