Re: [PATCH v5 01/14] ASoC: hdac_hdmi: Add hotplug notification and read eld

From: Mark Brown <broonie@kernel.org>
To: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Cc: tiwai@suse.de, patches.audio@intel.com,
	alsa-devel@alsa-project.org, Vinod Koul <vinod.koul@intel.com>,
	lgirdwood@gmail.com
Subject: Re: [PATCH v5 01/14] ASoC: hdac_hdmi: Add hotplug notification and read eld
Date: Tue, 9 Feb 2016 19:42:06 +0000	[thread overview]
Message-ID: <20160209194206.GO13270@sirena.org.uk> (raw)
In-Reply-To: <1454161428-7896-2-git-send-email-subhransu.s.prusty@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 1061 bytes --]

On Sat, Jan 30, 2016 at 07:13:35PM +0530, Subhransu S. Prusty wrote:

> +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> +
> +		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
> +						__func__, pin->nid);
> +		goto put_hdac_device;

Will this get noisy?

> +		/* TODO: use i915 component for reading ELD later */
> +		if (hdac_hdmi_get_eld(&edev->hdac, pin->nid,
> +				pin->eld.eld_buffer,
> +				&pin->eld.eld_size) == 0) {
> +
> +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> +					pin->eld.eld_buffer, pin->eld.eld_size);

ELD is an acronym, please write it properly (there's some other examples
in the patch).

> +		} else {
> +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> +			pin->eld.monitor_present = false;
> +			pin->eld.eld_valid = false;
> +		}

Is it really invalid or did we fail to read it?  There's a difference as
far as debugging is concerned (one means there's an I/O problem, the
other means that the display is reporting nonsense).  Perhaps this
should be silent and let the read function be more specific?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]