Hi Pierre-Louis,
Some nit picks for reducing LOC :)
[snip]
+static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *uinfo)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct hdmi_spec *spec;
- struct hdmi_spec_per_pin *per_pin;
- struct hdmi_eld *eld;
- int pin_idx;
- spec = codec->spec;
- uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
- pin_idx = kcontrol->private_value;
- if (snd_BUG_ON(pin_idx < 0))
return -EINVAL;- per_pin = &spec->pins[pin_idx];
- eld = &per_pin->sink_eld;
- uinfo->count = eld->eld_size;
The local variables @per_pin and @eld are referenced only and hence looks unnecessary.
- return 0;
+}
+static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct hdmi_spec *spec;
- struct hdmi_spec_per_pin *per_pin;
- struct hdmi_eld *eld;
- int pin_idx;
- int size;
- spec = codec->spec;
- pin_idx = kcontrol->private_value;
- if (snd_BUG_ON(pin_idx < 0))
return -EINVAL;
That check looks unnecessary since the pin_idx originates from the below hdmi_create_eld_ctl() which already checked it against 0.
- memset(ucontrol->value.bytes.data, 0, ELD_MAX_SIZE);
- per_pin = &spec->pins[pin_idx];
- eld = &per_pin->sink_eld;
- size = eld->eld_size;
@per_pin and @eld seems not necessary.
- memcpy(ucontrol->value.bytes.data, eld->eld_buffer, size);
The memset/memcpy could be merged into a single
memcpy(ucontrol->value.bytes.data, eld->eld_buffer, ELD_MAX_SIZE);
- return 0;
+}
+static struct snd_kcontrol_new eld_bytes_ctl = {
- .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
- .iface = SNDRV_CTL_ELEM_IFACE_PCM,
- .name = "ELD",
- .info = hdmi_eld_ctl_info,
- .get = hdmi_eld_ctl_get,
+};
+static int hdmi_create_eld_ctl(struct hda_codec *codec, int pin_idx,
int device)+{
- struct snd_kcontrol *kctl;
- struct hdmi_spec *spec = codec->spec;
- int err;
- if (pin_idx < 0)
return -EINVAL;
Even that check looks unnecessary.. see below
- kctl = snd_ctl_new1(&eld_bytes_ctl, codec);
- if (!kctl)
return -ENOMEM;- kctl->private_value = pin_idx;
- kctl->id.device = device;
- err = snd_hda_ctl_add(codec, spec->pins[pin_idx].pin_nid, kctl);
- if (err < 0)
return err;- return 0;
+}
#ifdef BE_PARANOID static void hdmi_get_dip_index(struct hda_codec *codec, hda_nid_t pin_nid, int *packet_index, int *byte_index) @@ -1193,6 +1273,14 @@ static int generic_hdmi_build_controls(struct hda_codec *codec) if (err < 0) return err; snd_hda_spdif_ctls_unassign(codec, pin_idx);
/* add control for ELD Bytes */err = hdmi_create_eld_ctl(codec,pin_idx,spec->pcm_rec[pin_idx].device);
The above "pcm_rec[pin_idx]" will likely oops if ever pin_idx is an invalid value, before hdmi_create_eld_ctl() checks pin_idx.