[alsa-devel] [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Wed, 10 Dec 2014 11:27:26 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/10/14, 10:35 AM, Takashi Iwai wrote:
> > Just a minor issue before going to detailed review:
> >
> > At Mon,  8 Dec 2014 16:23:42 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> +/* user space provides config to kernel */
> >> +struct snd_pcm_audio_tstamp_config {
> >> +	__u32 type_requested:4;
> >> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
> >> +};
> > ....
> >> +/* kernel provides report to user-space */
> >> +struct snd_pcm_audio_tstamp_report {
> >> +	/* actual type if hardware could not support requested timestamp */
> >> +	__u32 actual_type:4;
> >> +
> >> +	/* accuracy represented in mantissa/exponent form */
> >> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
> >> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
> >> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
> >> +};
> >
> > Please avoid the bit fields in these, since these values will be a
> > part of ABI.  There is absolutely no compatibility when you're using
> > bitfields.  Use the explicit bit operations.
> 
> Those definitions are not used as part of the kernel/user interaction, I 
> did use pack/unpack operations as requested in previous reviews. see 
> snd_pcm_unpack_tstamp_config and snd_pcm_pack_audio_tstamp_report.
> A similar pack/unpack operation is provided in the alsa-lib patches
> The data is exchanged using the reclaimed 32-bit word only.
> 
> Would that work?

Ah OK, then it's fine.  Then could you add more comments mentioning
that the structs are internal only and converted by the dedicated
functions for kernel ABI?

Also, use simple u32 instead of __u32.  Then it's clearer that it's an
internal usage.


thanks,

Takashi