On 2023-09-21 8:25 AM, Jaroslav Kysela wrote:
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Improve granularity of format selection for S32/U32 formats by adding
constants representing 20, 24 and MAX most significant bits.
To make it easy for drivers to utilize those constants, introduce
snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former
is self-explanatory, the latter returns the bit-per-sample value based
on format and subformat characteristics of the provided hw_params.
snd_pcm_hw_copy() helps with copying a hardware parameters space as with
introduction of subformats the operations becomes non-trivial.
Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
...
struct snd_pcm_hardware {
unsigned int info; /* SNDRV_PCM_INFO_* */
u64 formats; /* SNDRV_PCM_FMTBIT_* */
+ struct snd_pcm_subformat *subformats;
I don't think that it's required to add subformats to the hardware
template. The new constraint can handle subformat refining without the
template modifications (pointer to map table is stored privately to this
constraint).
After iterating over the feedback, modified the field to u32 and all the
handlers to acknowledge that it is S32_LE-specific. I agree with
statement that we can expand on the API in the future if there's demand
for it.
Also, I miss the constraint handling here. Without the constraint, the
new API is not functional and it does not make sense to split the
constraint code to other patch.
Ack, merging the two.
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);
This may be probably inline function. See bellow.
+ kfree(runtime->hw.subformats);
Do we really need to do an assumption about allocations for this field?
The driver may use a static structure. No problem, when this is not
added to runtime->hw.
This code is gone in v3 since there is no need to kfree() a u32.
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)
+{
+ snd_pcm_subformat_t subformat = params_subformat(p);
+ snd_pcm_format_t format = params_format(p);
+ int width;
+
+ switch (format) {
+ case SNDRV_PCM_FORMAT_S32_LE:
+ case SNDRV_PCM_FORMAT_U32_LE:
+ case SNDRV_PCM_FORMAT_S32_BE:
+ case SNDRV_PCM_FORMAT_U32_BE:
+ width = snd_pcm_subformat_width(subformat);
This is not a correct implementation. The width should be returned for
MAX subformat (== snd_pcm_format_width value). See bellow.
snd_pcm_subformat_width() returns 0 if subformat==MAX and fallthrough
ensures snd_pcm_format_width() is returned in such case.
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
This function should probably have two arguments - format + subformat to
return the correct information. The MAX subformat should return
snd_pcm_format_width value.
My intention is to have two granular functions. One for obtaining
subformat-width explicitly and the other for calculating bits-per-sample.
+int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct
snd_pcm_hardware *from)
This function is not required, if only the constraint function handles
the subformat refining.
Ack, removing in v3.
