On 13. 09. 23 10:22, Cezary Rojewski wrote:
+struct snd_pcm_hw_constraint_subformat {
- snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */
- u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */
From what I know, we are dealing with u64 masks here. Why u32 here?
It's not true. See the removed code which calls snd_pcm_hw_constraint_mask() for SNDRV_PCM_HW_PARAM_SUBFORMAT. Only the format is handled for 64 bits and the handling of other bits is purely optional, because masks can handle up to 256 bits:
#define SNDRV_MASK_MAX 256
struct snd_mask { __u32 bits[(SNDRV_MASK_MAX+31)/32]; };
Because we used only few bits for SUBFORMAT, the 32 bit code is enough. We can expand this latter without any impact to the user space interface.
+static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
struct snd_pcm_hw_rule *rule)+{
- const struct snd_pcm_hw_constraint_subformat *sf;
What's 'sf'? I'd suggest to be more descriptive here.
SubFormat. The larger name will not make the for loop more readable. The function is small, so reader is not lost.
- snd_pcm_format_t k;
Internally I was utilizing 'f' as that's what macro expects in its declaration. s/k/f/
Copied from pcm_native.c - snd_pcm_hw_rule_format().
- struct snd_mask m;
- struct snd_mask *fmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_mask *mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT);
So, the reason I opted for 'subformat_mask' and 'format_mask' is that otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd avoid shortcuts when multiple variables touch the same subject.
s/fmask/format_mask/ s/mask/subformat_mask/
Too long, the function is small.
- snd_mask_none(&m);
- snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
- bool found;
Suggestion is to add newline before declaration and execution blocks. Also, why not reserve-christmass-tree model? There quite a few variables here.
Yes it should be shuffled.
- pcm_for_each_format(k) {
if (!snd_mask_test(fmask, k))continue;Similarly here. A newline would effectively separate conditional for-loop from the actual execution block.
It's questionable.
found = false;for (sf = rule->private; sf && sf->format != SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) {if (sf->format != k)continue;found = true;m.bits[0] |= sf->subformats;break;}if (!found && snd_pcm_format_linear(k))For my own education, why checking if format is linear is essential here? Perhaps a comment?
Subformat MSBITS are valid only for linear formats, aren't? It does not make sense to set MAX for other formats.
snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);- }
- return snd_mask_refine(mask, &m);
+}
+/**
- snd_pcm_hw_constraint_subformats - add a hw constraint subformats rule
- @runtime: PCM runtime instance
- @cond: condition bits
- @subformats: array with struct snd_pcm_subformat elements
- @nmemd: size of array with struct snd_pcm_subformat elements
- This constraint will set relation between format and subformats.
I do not believe 'This constaint' brings any value. Reader is already aware of it. Starting from the 'Set' part brings the same value with fewer words.
Copied from snd_pcm_hw_constraint_msbits function.
- The STD and MAX subformats are handled automatically. If the driver
- does not set this constraint, only STD and MAX subformats are handled.
- Return: Zero if successful, or a negative error code on failure.
- */
+int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
unsigned int cond,struct snd_pcm_hw_constraint_subformat *subformats)+{
...
- err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT,
PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD));- if (err < 0)
return err;
- if (!runtime->subformat_constraint) {
I'd try to avoid another special-bit in the runtime space. But I might be wrong here and it's unavoidable. Let me ask though, why cannot we do the constraint unconditionally?
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'll send v2 to address some things from this discussion and kernel test robot.
Jaroslav