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
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
