| Message ID | 1345887836-15314-2-git-send-email-s.shirish@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 25, 2012 at 03:13:56PM +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 <s.shirish@samsung.com> Looks good. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- > 1 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; > int ret, retries = 5; > > /* The core i2c driver will automatically retry the transfer if the > @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > */ > do { > struct i2c_msg msgs[] = { > - { > + { /*set segment pointer */ > + .addr = DDC_SEGMENT_ADDR, > + .flags = segment ? 0 : I2C_M_IGNORE_NAK, > + .len = 1, > + .buf = &segment, > + }, { > .addr = DDC_ADDR, > .flags = 0, > .len = 1, > @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > .buf = buf, > } > }; > - ret = i2c_transfer(adapter, msgs, 2); > + /* Avoid sending the segment addr to not upset non-compliant ddc > + * monitors. > + */ > + if (!segment) > + ret = i2c_transfer(adapter, &msgs[1], xfers); > + else > + ret = i2c_transfer(adapter, msgs, xfers); > + > if (ret == -ENXIO) { > DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > adapter->name); > break; > } > - } while (ret != 2 && --retries); > + } while (ret != xfers && --retries); > > - return ret == 2 ? 0 : -1; > + return ret == xfers ? 0 : -1; > } > > static bool drm_edid_is_zero(u8 *in_edid, int length) > -- > 1.7.0.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Aug 25, 2012 at 03:13:56PM +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 <s.shirish@samsung.com> > --- > drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- > 1 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; > int ret, retries = 5; > > /* The core i2c driver will automatically retry the transfer if the > @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > */ > do { > struct i2c_msg msgs[] = { > - { > + { /*set segment pointer */ Missing whitespace after '/*'. Perhaps just drop the comment. I don't see much value in it. > + .addr = DDC_SEGMENT_ADDR, > + .flags = segment ? 0 : I2C_M_IGNORE_NAK, > + .len = 1, > + .buf = &segment, > + }, { > .addr = DDC_ADDR, > .flags = 0, > .len = 1, > @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, > .buf = buf, > } > }; > - ret = i2c_transfer(adapter, msgs, 2); > + /* Avoid sending the segment addr to not upset non-compliant ddc > + * monitors. > + */ Wrong indentation and comment style is wrong. I'm guessing this didn't go through checkpatch.pl. Otherwise looks OK to me.
Hi all, Sorry for breaking message threading but I was not included in iterations 3 and 4 of this patch. Random comments about v4: > --- 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 char xfers = segment ? 3 : 2; > int ret, retries = 5; > > /* The core i2c driver will automatically retry the transfer if the > @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > */ > do { > struct i2c_msg msgs[] = { > - { > + { /*set segment pointer */ > + .addr = DDC_SEGMENT_ADDR, > + .flags = segment ? 0 : I2C_M_IGNORE_NAK, I don't get the idea. If segment == 0, this message is never sent, so the value of field flags doesn't matter. So flags will always be 0 when this message is sent, so it can be hard-coded. But from previous discussions my understanding was an agreement on always using I2C_M_IGNORE_NAK for improved compatibility. So I2C_M_IGNORE_NAK should be hard-coded, not 0? > + .len = 1, > + .buf = &segment, > + }, { > .addr = DDC_ADDR, > .flags = 0, > .len = 1, > @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > .buf = buf, > } > }; > - ret = i2c_transfer(adapter, msgs, 2); > + /* Avoid sending the segment addr to not upset non-compliant ddc > + * monitors. > + */ s/segment addr/segment/, plus it's abot E-DCC compliance as I understand it, not DDC. > + if (!segment) > + ret = i2c_transfer(adapter, &msgs[1], xfers); > + else > + ret = i2c_transfer(adapter, msgs, xfers); > + This can be written: ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers); Which is more compact and, I suspect, faster. > if (ret == -ENXIO) { > DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > adapter->name); > break; > } > - } while (ret != 2 && --retries); > + } while (ret != xfers && --retries); > > - return ret == 2 ? 0 : -1; > + return ret == xfers ? 0 : -1; > } > > static bool drm_edid_is_zero(u8 *in_edid, int length) Other than this, your code looks reasonable, not so different from what I submitted 8 months ago actually. But ISTU you can test the code with real hardware while I couldn't. With the changes above applied, you can add: Reviewed-by: Jean Delvare <jdelvare@suse.de>
On Wed, Aug 29, 2012 at 4:08 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote: > On Sat, Aug 25, 2012 at 03:13:56PM +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 <s.shirish@samsung.com> > > --- > > drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- > > 1 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; > > int ret, retries = 5; > > > > /* The core i2c driver will automatically retry the transfer if the > > @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > > */ > > do { > > struct i2c_msg msgs[] = { > > - { > > + { /*set segment pointer */ > > Missing whitespace after '/*'. Perhaps just drop the comment. I don't > see much value in it. > > Done. > > + .addr = DDC_SEGMENT_ADDR, > > + .flags = segment ? 0 : I2C_M_IGNORE_NAK, > > + .len = 1, > > + .buf = &segment, > > + }, { > > .addr = DDC_ADDR, > > .flags = 0, > > .len = 1, > > @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > unsigned char *buf, > > .buf = buf, > > } > > }; > > - ret = i2c_transfer(adapter, msgs, 2); > > + /* Avoid sending the segment addr to not upset non-compliant ddc > > + * monitors. > > + */ > > Wrong indentation and comment style is wrong. I'm guessing this didn't go > through checkpatch.pl. > > I never got tha above as either an error or warning in the checkpatch, anyways have uploaded patch set 5 incorporating your comments. > Otherwise looks OK to me. > > -- > Ville Syrjälä > Intel OTC > - Shirish > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Jean, On Wed, Aug 29, 2012 at 6:44 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi all, > > Sorry for breaking message threading but I was not included in > iterations 3 and 4 of this patch. > > Random comments about v4: > > > --- 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 char xfers = segment ? 3 : 2; > > int ret, retries = 5; > > > > /* The core i2c driver will automatically retry the transfer if > the > > @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > > unsigned char *buf, > > */ > > do { > > struct i2c_msg msgs[] = { > > - { > > + { /*set segment pointer */ > > + .addr = DDC_SEGMENT_ADDR, > > + .flags = segment ? 0 : I2C_M_IGNORE_NAK, > > I don't get the idea. If segment == 0, this message is never sent, so the > value of field flags doesn't matter. So flags will always be 0 when this > message is sent, so it can be hard-coded. > > Agreed. > But from previous discussions my understanding was an agreement on always > using I2C_M_IGNORE_NAK for improved compatibility. So I2C_M_IGNORE_NAK > should be hard-coded, not 0? > After discussion,daniel had asked for a seprate patch for the flags modification. Will upload that later. > > > + .len = 1, > > + .buf = &segment, > > + }, { > > .addr = DDC_ADDR, > > .flags = 0, > > .len = 1, > > @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, > > unsigned char *buf, > > .buf = buf, > > } > > }; > > - ret = i2c_transfer(adapter, msgs, 2); > > + /* Avoid sending the segment addr to not upset non-compliant ddc > > + * monitors. > > + */ > > s/segment addr/segment/, plus it's abot E-DCC compliance as I understand > it, > not DDC. > > > + if (!segment) > > + ret = i2c_transfer(adapter, &msgs[1], xfers); > > + else > > + ret = i2c_transfer(adapter, msgs, xfers); > > + > > This can be written: > > ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers); > > Which is more compact and, I suspect, faster. > > Agreed. > > if (ret == -ENXIO) { > > DRM_DEBUG_KMS("drm: skipping non-existent > adapter %s\n", > > adapter->name); > > break; > > } > > - } while (ret != 2 && --retries); > > + } while (ret != xfers && --retries); > > > > - return ret == 2 ? 0 : -1; > > + return ret == xfers ? 0 : -1; > > } > > > > static bool drm_edid_is_zero(u8 *in_edid, int length) > > Other than this, your code looks reasonable, not so different from what > I submitted 8 months ago actually. But ISTU you can test the code with > real hardware while I couldn't. > > Your patch never checked for the 3 message transfer complete, it checked only 2. With the changes above applied, you can add: > > Reviewed-by: Jean Delvare <jdelvare@suse.de> > > Will add your review comments in patch set 5 and your reviewed tag. Thanks & Regards, Shirish S > -- > Jean Delvare > Suse L3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; int ret, retries = 5; /* The core i2c driver will automatically retry the transfer if the @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = { - { + { /*set segment pointer */ + .addr = DDC_SEGMENT_ADDR, + .flags = segment ? 0 : I2C_M_IGNORE_NAK, + .len = 1, + .buf = &segment, + }, { .addr = DDC_ADDR, .flags = 0, .len = 1, @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } }; - ret = i2c_transfer(adapter, msgs, 2); + /* Avoid sending the segment addr to not upset non-compliant ddc + * monitors. + */ + if (!segment) + ret = i2c_transfer(adapter, &msgs[1], xfers); + else + ret = i2c_transfer(adapter, msgs, xfers); + if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; } - } while (ret != 2 && --retries); + } while (ret != xfers && --retries); - return ret == 2 ? 0 : -1; + return ret == xfers ? 0 : -1; } static bool drm_edid_is_zero(u8 *in_edid, int length)
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 <s.shirish@samsung.com> --- drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)