Re: [PATCH v5] sound: rawmidi: Add framing mode

[Jaroslav Kysela <perex@perex.cz>]

Dne 18. 04. 21 v 17:12 David Henningsson napsal(a):

> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 16

SNDRV_ prefix should be here.

> +
> +struct snd_rawmidi_framing_tstamp {
> +	/* For now, frame_type is always 0. Midi 2.0 is expected to add new
> +	 * types here. Applications are expected to skip unknown frame types.
> +	 */
> +	u8 frame_type;
> +	u8 length; /* number of valid bytes in data field */
> +	u8 reserved[2];
> +	u32 tv_nsec;		/* nanoseconds */
> +	u64 tv_sec;		/* seconds */
> +	u8 data[SND_RAWMIDI_FRAMING_DATA_LENGTH];

What about to move the fields to union (except for frame_type) like we do for
'struct snd_ctl_event' in case when we need to reorganize the contents for
future types?

> +};
> +
>  struct snd_rawmidi_params {
>  	int stream;
>  	size_t buffer_size;		/* queue size in bytes */
>  	size_t avail_min;		/* minimum avail bytes for wakeup */
>  	unsigned int no_active_sensing: 1; /* do not send active sensing byte in close() */
> -	unsigned char reserved[16];	/* reserved for future use */
> +	unsigned char framing;		/* For input data only, frame incoming data */
> +	unsigned char clock_type;	/* Type of clock to use for framing, same as clockid_t */
> +	unsigned char reserved[14];	/* reserved for future use */

As I noted, I would prefer to add 'unsigned int mode;' and define
SNDRV_RAWMID_MODE_XXX bit flags and groups with framing and clock_type groups.
There's no reason to stick with 'clockid_t' (which is integer anyway). We're
using just a subset.

#define SNDRV_RAWMIDI_MODE_FRAMING_MASK        (7<<0)
#define SNDRV_RAWMIDI_MODE_FRAMING_SHIFT       0
#define SNDRV_RAWMIDI_MODE_FRAMING_NONE	       (0<<0)
#define SNDRV_RAWMIDI_MODE_FRAMING_32BYTES     (1<<0)
#define SNDRV_RAWMIDI_MODE_CLOCK_MASK          (7<<3)
#define SNDRV_RAWMIDI_MODE_CLOCK_SHIFT         3
#define SNDRV_RAWMIDI_MODE_CLOCK_NONE	       (0<<3)
#define SNDRV_RAWMIDI_MODE_CLOCK_REALTIME      (1<<3)
#define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC     (2<<3)
#define SNDRV_RAWMIDI_MODE_CLOCK_MONOTONIC_RAW (3<<3)

In this case, we can use 26-bits in future for extensions.

> +struct timespec64 get_framing_tstamp(struct snd_rawmidi_substream *substream)
> +{
> +	struct timespec64 ts64 = {0, 0};
> +
> +	if (substream->framing != SNDRV_RAWMIDI_FRAMING_TSTAMP)
> +		return ts64;
> +	if (substream->clock_type == CLOCK_MONOTONIC_RAW)
> +		ktime_get_raw_ts64(&ts64);
> +	else
> +		ktime_get_ts64(&ts64);
> +	return ts64;
> +}

Missing the realtime clock type here.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.