On Wed, Feb 10, 2016 at 07:23:28AM +0000, Mark Brown wrote:
On Wed, Feb 10, 2016 at 10:03:58AM +0530, Vinod Koul wrote:
On Tue, Feb 09, 2016 at 07:42:06PM +0000, Mark Brown wrote:
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?
Not really, we print when we get the hotplug notification. Usually that wont happen too often unless someone is moneky testing :)
That sounds like it might get old very quickly, we presumably already have the graphics stack doing the same thing.
Last I did not see that, but yes lets make it less noisy, will move this to debug
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).
ELD is quite often used acronym in HDMI world, It is already there in drivers (did a quick grep :)
I know it's widely used, I'm saying you should write it properly.
Okay, I think your concern is on writing, sorry I though about the term, will fix it now :)
} 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?
There is no way to know if this was IO or garbage from display. So we just throw up error and retry few times.
It seems implausible that there is no way of identifying I/O errors, and even if that is the case the point about the message not being terribly helpful still stands - moving the reporting to the point where we decide there is an error would be a lot more helpful.
Hrmmm, looking back and checking the get_eld does print more verbose information on the failure, so this one is really not required, we will remove this
Thanks