Takashi Iwai wrote:
> No, please not yet. The code is still not good enough for commit.
>
> For example, some things to be fixed regarding the code (and coding
> style) are below:
>
> +static int uint32_to_message(struct snd_ctl_misc *misc, u32 value)
> +{
> + unsigned int pointer = misc->length;
> + u32 *store = (u32*) &misc->message[pointer];
>
> This cast isn't always allowed.
>
> + *store = value;
> + misc->length += sizeof(u32);
> + return 0;
> +}
>
>
What do I use instead?
+ misc->message[pointer++] = value & 0xff;
+ misc->message[pointer++] = (value >> 8) & 0xff;
+ misc->message[pointer++] = (value >> 16) & 0xff;
+ misc->message[pointer++] = (value >> 24) & 0xff;
> +static int add_message_tlv_to_container(
> + struct snd_ctl_misc *misc, unsigned int length, unsigned char *message)
>
> Unconventional indentation.
>
> + unsigned int pointer = misc->length;
> + unsigned int n;
> + for(n=0; n<length; n++) {
> + misc->message[pointer++] = message[n];
> + }
>
> What's wrong with memcpy()?
>
Nothing wrong with memcpy().
> +static int snd_ctl_misc_user(struct snd_ctl_file *ctl,
> + struct snd_ctl_misc __user *_misc)
> +{
> + struct snd_ctl_misc *misc;
> + struct snd_card *card = ctl->card;
> + struct snd_kcontrol *kctl;
> + struct snd_ctl_elem_info2 info2;
> + int result=0;
> + int numid;
> + unsigned int received_length;
> + unsigned int tlv_type;
> + unsigned int tlv_length;
> + unsigned int tlv_value;
> + unsigned int container_type;
> + unsigned int container_length;
> +
> + misc = kmalloc(2048, GFP_KERNEL);
> + if (misc == NULL)
> + return -ENOMEM;
>
> Why always 2048?
>
No particular reason, just seemed like a ball park value that I should
not have to change any time soon. I could probably replace it with some
getuser32 calls to get the required length from the userspace app. Is
getuser32() the correct call to use instead of copy_from_user(), if all
one needs is a 4 byte integer from a particular memory location?
> Also, cases should be aligned to switch().
>
Ok.
> But, above all, such a large switch() is ugly. Better to put the
> container-type specific handling out of this function.
>
> Although this whole block basically belongs only to DB_SCALE case, the
> current code looks as if this block is commonly handled regardless of
> the container type.
>
That's true. I will look at putting the case action as a separate function.
> +struct snd_ctl_misc {
> + u32 version;
> + u32 length;
> + unsigned char message[];
> +};
>
> The empty array isn't portable.
>
Ok, how do I do this then?
The array is not actually empty, but it's size is dependent on the
length field.
> +typedef int (snd_kcontrol_info2_t) (struct snd_kcontrol * kcontrol, int type, struct snd_ctl_elem_info2 * uinfo);
>
> The type should be non-numeric but constants (bitwise define or
> enum).
>
I don't understand. This is almost an exact copy of:
typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol,
struct snd_ctl_elem_info * uinfo);
That is already in the code.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
