Re: [RFC PATCH] ALSA: pcm: Introduce MSBITS subformat API extension

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2023-09-19 12:07 PM, Jaroslav Kysela wrote:
> On 19. 09. 23 11:28, Cezary Rojewski wrote:

...

>>> 2) 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@intel.com/
[2]: 
https://lore.kernel.org/alsa-devel/8d76a1d8-e85c-b699-34a0-ecea6edc2fe1@intel.com/
[3]: 
https://lore.kernel.org/alsa-devel/20230918133940.3676091-1-cezary.rojewski@intel.com/

> 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.