[alsa-devel] [PATCH] [RFC 3/13] Intel SST driver include headers

Takashi Iwai tiwai at suse.de
Tue Jul 7 08:46:28 CEST 2009


At Tue, 7 Jul 2009 11:59:37 +0530,
Harsha, Priya wrote:
> 
> >>
> >> >> +/* Firmware Version info */
> >> >> +struct snd_sst_fw_version {
> >> >> +	__u8 build;	/* build number*/
> >> >> +	__u8 minor;	/* minor number*/
> >> >> +	__u8 major;	/* major number*/
> >> >> +	__u8 type; /* build type*/
> >> >> +};
> >> >> +
> >> >> +/* Port info structure */
> >> >> +struct snd_sst_port_info {
> >> >> +	__u16 port_type;
> >> >
> >> >Just wondering -- is there big-endian support?
> >> We have not tested this on big-endian, Could you tell us that should
> >> we make it endian safe?
> >
> >When a field is more than one byte, you'd need endian conversion at
> >read/write.  I didn't find it thus I suspected it.
> So, does it look safe to keep the code as such?

Unlikely...

> We are using the bit
> field in union only for ease of setting/getting bit values and not
> for communication.

What I meant is the data used for the communication.  Not the
communication method itself.  Here the communication includes the
firmware data, too.

If the data is created by the driver locally and used only inside the
driver, then it's fine.  Otherwise, you shouldn't use bit fields.


> >
> >> >> +struct snd_sst_vol {
> >> >> +	unsigned int	stream_id;
> >> >> +	int		volume;
> >> >> +	unsigned long	ramp_duration;
> >> >
> >> >Are you sure to use long?
> >> >Long can be different between 32 and 64bit architectures.
> >> We will change it to u32.
> >>
> >> >
> >> >> +struct snd_sst_buff_entry {
> >> >> +	union {
> >> >> +		void *user;
> >> >> +		unsigned int offset;
> >> >> +	} buffer;
> >> >
> >> >Is it OK?
> >> >The pointer and int can be different sizes.
> >> void* user - is the pointer to the buffer
> >> unsigned int offset - is the offset inside the buffer area.
> >> Not sure if I understand your comment. Please let us know if there
> >> is any issue you see with this structure.
> >
> >A pointer can be 64bit while offset is always 32bit.
> >If they don't have to share the same space, why need to be a union?
> I think I will give more clear explanation here before this
> structure declaration. I know its confusing. Basically we support
> both mmap and user buffers. When user buffers are passed to the
> driver then void *user field is used and when mmap buffers are
> passed to the driver, the offset in the mmap buffer is passed in
> this structure. Either mmap buffer's offset or user buffer address
> will be used and not both.

Then avoid union unless you really need it (i.e. the tight memory
packing).  In general, union is often a cause of troubles.
As a simple example against union, imagine that you put a debug print
of this struct...


thanks,

Takashi


More information about the Alsa-devel mailing list