Re: [PATCH 1/2] ALSA: core: add hooks for audio timestamps read from WALLCLOCK

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>
> The biggest problem of struct timespec is that it's pretty much
> arch-dependent.  But since it's used in all other places, we need to
> live with that...
>
> Usually when we add a new field, we don't use union.  Just decrease
> sizeof(struct timespec) from reserved[] size.
>
> No matter whether using union or not, it doesn't mean that the whole
> struct size work is kept.  The field might be aligned since we haven't
> added the packed attribute.  Maybe better to add a padding to align
> 64bit before audio_tstamp, then cross your finger.

Ok, this is the feedback I needed, I had no idea if I could add a new 
field and why exactly the reserved field had to be zeroed out. On the 
first try I had an invalid ioctl error and found this workaround.
Will fix this.


>>   struct snd_pcm_mmap_status {
>> @@ -430,6 +434,7 @@ struct snd_pcm_mmap_status {
>>   	int pad1;			/* Needed for 64 bit alignment */
>>   	snd_pcm_uframes_t hw_ptr;	/* RO: hw ptr (0...boundary-1) */
>>   	struct timespec tstamp;		/* Timestamp */
>> +	struct timespec audio_tstamp;	/* audio wall clock timestamp */
>>   	snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>>   };
>
> struct snd_pcm_mmap_status is mmapped to user-space, thus it must be
> backward compatible.  Always append the new field.

will do.

> In addition, if you change the ABI, please change the PCM protocol
> version, so that alsa-lib can detect the ABI change

ok

> Also, last not but least, don't forget to convert pcm_compat.c.

will look into it

> Other than these, changes look good to me.

Cool, thanks for the feedback.
-Pierre