From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Delvare Subject: Re: [PATCH V4] drm: edid: add support for E-DDC Date: Wed, 29 Aug 2012 15:44:17 +0200 Message-ID: <1346247857.21586.17.camel@amber.site> References: <1345887836-15314-2-git-send-email-s.shirish@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id 3686D9ED48 for ; Wed, 29 Aug 2012 06:44:33 -0700 (PDT) In-Reply-To: <1345887836-15314-2-git-send-email-s.shirish@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: s.shirish@samsung.com, Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 -- Jean Delvare Suse L3