Re: [PATCH V4] drm: edid: add support for E-DDC

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