Thanks for the comments, will upload patch set<div>with your comments. <div class="gmail_quote">On Fri, Aug 24, 2012 at 11:20 AM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Aug 24, 2012 at 02:56:50PM +0530, Shirish S wrote: > The current logic for probing ddc is limited to > 2 blocks (256 bytes), this patch adds support > for the 4 block (512) data. > > To do this, a single 8-bit segment index is > passed to the display via the I2C address 30h. > Data from the selected segment is then immediately > read via the regular DDC2 address using a repeated > I2C 'START' signal. > > Signed-off-by: Shirish S <<a href="mailto:s.shirish@samsung.com">s.shirish@samsung.com</a>> > --- > drivers/gpu/drm/drm_edid.c | 83 ++++++++++++++++++++++++++++++++----------- > 1 files changed, 62 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a8743c3..a10a130 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > int block, int len) > { > unsigned char start = block * EDID_LENGTH; > + unsigned char segment = block >> 1; > + unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK; </div>You can inline segFlags to the only place it's used. <div><div class="h5"> > int ret, retries = 5; > > /* The core i2c driver will automatically retry the transfer if the > @@ -262,29 +264,68 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > * generate spurious NAKs and timeouts. Retrying the transfer > * of the individual block a few times seems to overcome this. > */ > - do { > - struct i2c_msg msgs[] = { > - { > - .addr = DDC_ADDR, > - .flags = 0, > - .len = 1, > - .buf = &start, > - }, { > - .addr = DDC_ADDR, > - .flags = I2C_M_RD, > - .len = len, > - .buf = buf, > + switch (segment) { > + /* To reduce traffic on I2C, we request the i2c xfer > + * for segment addr only if 4 block edid data is > + * present. > + * 0 : DDC > + * 1 : E-DDC > + */ </div></div>This comment is a bit misleading: We don't avoid the segment xfer to reduce i2c traffic, but to avoid upsetting any non-compliant screens. <div class="im"> > + case 0: > + do { </div>Keeping the loop as the outermost would imo be clearer and avoid duplicating it (it also would make the patch a bit smaller). You could even do a trick with the msgs array like this: msgs[] = { full i2c xfer including segment addr} xfers = 3; /* Avoid sending the segment addr to not upset non-compliant ddc * monitors. */ if (!segments) msgs++, len--; i2c_transefer(adapter, msgs, len); Also, I think it'd be good to set the IGNORE_NAK on the start addre transfer, too (but in a separate patch). At least I think we've seen screens out there that need this :( Yours, Daniel <div class="HOEnZb"><div class="h5"> > + struct i2c_msg msgs[] = { > + { > + .addr = DDC_ADDR, > + .flags = 0, > + .len = 1, > + .buf = &start, > + }, { > + .addr = DDC_ADDR, > + .flags = I2C_M_RD, > + .len = len, > + .buf = buf, > + } > + }; > + ret = i2c_transfer(adapter, msgs, 2); > + if (ret == -ENXIO) { > + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > + adapter->name); > + break; > } > - }; > - ret = i2c_transfer(adapter, msgs, 2); > - if (ret == -ENXIO) { > - DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > - adapter->name); > - break; > - } > - } while (ret != 2 && --retries); > + } while (ret != 2 && --retries); > + > + return ret == 2 ? 0 : -1; > + case 1: > + do { > + struct i2c_msg msgs[] = { > + { /*set segment pointer */ > + .addr = DDC_SEGMENT_ADDR, > + .flags = segFlags, > + .len = 1, > + .buf = &segment, > + }, { /*set offset */ > + .addr = DDC_ADDR, > + .flags = 0, > + .len = 1, > + .buf = &start, > + }, { /*set data */ > + .addr = DDC_ADDR, > + .flags = I2C_M_RD, > + .len = len, > + .buf = buf, > + } > + }; > + ret = i2c_transfer(adapter, msgs, 3); > + if (ret == -ENXIO) { > + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > + adapter->name); > + break; > + } > + } while (ret != 3 && --retries); > > - return ret == 2 ? 0 : -1; > + return ret == 3 ? 0 : -1; > + } > + return -1; > } > > static bool drm_edid_is_zero(u8 *in_edid, int length) > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </div></div>-- Daniel Vetter Mail: <a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a> Mobile: +41 (0)79 365 57 48 <div class="HOEnZb"><div class="h5">_______________________________________________ dri-devel mailing list <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </div></div></blockquote></div> </div>