From mboxrd@z Thu Jan 1 00:00:00 1970 From: moinejf@free.fr (Jean-Francois Moine) Date: Sun, 2 Feb 2014 18:45:12 +0100 Subject: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers In-Reply-To: <20140202162309.GF26684@n2100.arm.linux.org.uk> References: <5269e596e16dfe40253dce38ceb0dc4a617384c1.1390986083.git.moinejf@free.fr> <20140202162309.GF26684@n2100.arm.linux.org.uk> Message-ID: <20140202184512.7fa2e3cf@armhf> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux wrote: > On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote: > > This patch takes care of the write-only registers of the tda998x. > > > > The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits > > cleared after reset, so, they may be fully re-written. > > > > The register MAT_CONTRL is set to > > MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) > > after reset, so, it may be fully set again to this value. > > I said in v3 of this patch, which seems to remain unaddressed: > > > /* must be last register set: */ > > - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); > > + reg_write(priv, REG_TBG_CNTRL_0, 0); > > Register changes which have a potential effect shouldn't be part of a > patch which is really only trying to avoid reading from write only > registers. > > This could be a potential functional change - and it's probably one > which Rob Clark should at least be made aware of. As I commented last > time, when you're changing register values in an otherwise innocuous > patch, you should comment about them in the patch description. According to the tda9983b documentation, the register TBG_CNTRL_0 is set to 0 at reset time. I think that it is the same for all the tda998x family. In the other hand, this register is supposed to be write only, so reading it may return any value, and the reg_clear() function may set any other bits. Then, clearing one bit is less secure than clearing the full register. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Francois Moine Subject: Re: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers Date: Sun, 2 Feb 2014 18:45:12 +0100 Message-ID: <20140202184512.7fa2e3cf@armhf> References: <5269e596e16dfe40253dce38ceb0dc4a617384c1.1390986083.git.moinejf@free.fr> <20140202162309.GF26684@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from smtp3-g21.free.fr (smtp3-g21.free.fr [212.27.42.3]) by gabe.freedesktop.org (Postfix) with ESMTP id 96D3EF9E51 for ; Sun, 2 Feb 2014 09:44:54 -0800 (PST) In-Reply-To: <20140202162309.GF26684@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org T24gU3VuLCAyIEZlYiAyMDE0IDE2OjIzOjA5ICswMDAwClJ1c3NlbGwgS2luZyAtIEFSTSBMaW51 eCA8bGludXhAYXJtLmxpbnV4Lm9yZy51az4gd3JvdGU6Cgo+IE9uIFNhdCwgSmFuIDI1LCAyMDE0 IGF0IDA2OjE0OjQyUE0gKzAxMDAsIEplYW4tRnJhbmNvaXMgTW9pbmUgd3JvdGU6ICAKPiA+IFRo aXMgcGF0Y2ggdGFrZXMgY2FyZSBvZiB0aGUgd3JpdGUtb25seSByZWdpc3RlcnMgb2YgdGhlIHRk YTk5OHguCj4gPiAKPiA+IFRoZSByZWdpc3RlcnMgU09GVFJFU0VULCBUQkdfQ05UUkxfMCBhbmQg VEJHX0NOVFJMXzEgaGF2ZSBhbGwgYml0cwo+ID4gY2xlYXJlZCBhZnRlciByZXNldCwgc28sIHRo ZXkgbWF5IGJlIGZ1bGx5IHJlLXdyaXR0ZW4uCj4gPiAKPiA+IFRoZSByZWdpc3RlciBNQVRfQ09O VFJMIGlzIHNldCB0bwo+ID4gCU1BVF9DT05UUkxfTUFUX0JQIHwgTUFUX0NPTlRSTF9NQVRfU0Mo MSkKPiA+IGFmdGVyIHJlc2V0LCBzbywgaXQgbWF5IGJlIGZ1bGx5IHNldCBhZ2FpbiB0byB0aGlz IHZhbHVlLiAgCj4gCj4gSSBzYWlkIGluIHYzIG9mIHRoaXMgcGF0Y2gsIHdoaWNoIHNlZW1zIHRv IHJlbWFpbiB1bmFkZHJlc3NlZDoKPiAgIAo+ID4gICAgICAgLyogbXVzdCBiZSBsYXN0IHJlZ2lz dGVyIHNldDogKi8KPiA+IC0gICAgIHJlZ19jbGVhcihwcml2LCBSRUdfVEJHX0NOVFJMXzAsIFRC R19DTlRSTF8wX1NZTkNfT05DRSk7Cj4gPiArICAgICByZWdfd3JpdGUocHJpdiwgUkVHX1RCR19D TlRSTF8wLCAwKTsgIAo+IAo+IFJlZ2lzdGVyIGNoYW5nZXMgd2hpY2ggaGF2ZSBhIHBvdGVudGlh bCBlZmZlY3Qgc2hvdWxkbid0IGJlIHBhcnQgb2YgYQo+IHBhdGNoIHdoaWNoIGlzIHJlYWxseSBv bmx5IHRyeWluZyB0byBhdm9pZCByZWFkaW5nIGZyb20gd3JpdGUgb25seQo+IHJlZ2lzdGVycy4K PiAKPiBUaGlzIGNvdWxkIGJlIGEgcG90ZW50aWFsIGZ1bmN0aW9uYWwgY2hhbmdlIC0gYW5kIGl0 J3MgcHJvYmFibHkgb25lCj4gd2hpY2ggUm9iIENsYXJrIHNob3VsZCBhdCBsZWFzdCBiZSBtYWRl IGF3YXJlIG9mLiAgQXMgSSBjb21tZW50ZWQgbGFzdAo+IHRpbWUsIHdoZW4geW91J3JlIGNoYW5n aW5nIHJlZ2lzdGVyIHZhbHVlcyBpbiBhbiBvdGhlcndpc2UgaW5ub2N1b3VzCj4gcGF0Y2gsIHlv dSBzaG91bGQgY29tbWVudCBhYm91dCB0aGVtIGluIHRoZSBwYXRjaCBkZXNjcmlwdGlvbi4gIAoK QWNjb3JkaW5nIHRvIHRoZSB0ZGE5OTgzYiBkb2N1bWVudGF0aW9uLCB0aGUgcmVnaXN0ZXIgVEJH X0NOVFJMXzAgaXMKc2V0IHRvIDAgYXQgcmVzZXQgdGltZS4gSSB0aGluayB0aGF0IGl0IGlzIHRo ZSBzYW1lIGZvciBhbGwgdGhlIHRkYTk5OHgKZmFtaWx5LiBJbiB0aGUgb3RoZXIgaGFuZCwgdGhp cyByZWdpc3RlciBpcyBzdXBwb3NlZCB0byBiZSB3cml0ZSBvbmx5LApzbyByZWFkaW5nIGl0IG1h eSByZXR1cm4gYW55IHZhbHVlLCBhbmQgdGhlIHJlZ19jbGVhcigpIGZ1bmN0aW9uIG1heQpzZXQg YW55IG90aGVyIGJpdHMuIFRoZW4sIGNsZWFyaW5nIG9uZSBiaXQgaXMgbGVzcyBzZWN1cmUgdGhh biBjbGVhcmluZwp0aGUgZnVsbCByZWdpc3Rlci4KCi0tIApLZW4gYXIgYydoZW50YcOxCXwJICAg ICAgKiogQnJlaXpoIGhhIExpbnV4IGF0YXYhICoqCkplZgkJfAkJaHR0cDovL21vaW5lamYuZnJl ZS5mci8KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJp LWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6 Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751981AbaBBRo7 (ORCPT ); Sun, 2 Feb 2014 12:44:59 -0500 Received: from smtp3-g21.free.fr ([212.27.42.3]:52480 "EHLO smtp3-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751798AbaBBRo6 convert rfc822-to-8bit (ORCPT ); Sun, 2 Feb 2014 12:44:58 -0500 Date: Sun, 2 Feb 2014 18:45:12 +0100 From: Jean-Francois Moine To: dri-devel@lists.freedesktop.org Cc: Dave Airlie , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Rob Clark Subject: Re: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers Message-ID: <20140202184512.7fa2e3cf@armhf> In-Reply-To: <20140202162309.GF26684@n2100.arm.linux.org.uk> References: <5269e596e16dfe40253dce38ceb0dc4a617384c1.1390986083.git.moinejf@free.fr> <20140202162309.GF26684@n2100.arm.linux.org.uk> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.22; arm-unknown-linux-gnueabihf) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux wrote: > On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote: > > This patch takes care of the write-only registers of the tda998x. > > > > The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits > > cleared after reset, so, they may be fully re-written. > > > > The register MAT_CONTRL is set to > > MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) > > after reset, so, it may be fully set again to this value. > > I said in v3 of this patch, which seems to remain unaddressed: > > > /* must be last register set: */ > > - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); > > + reg_write(priv, REG_TBG_CNTRL_0, 0); > > Register changes which have a potential effect shouldn't be part of a > patch which is really only trying to avoid reading from write only > registers. > > This could be a potential functional change - and it's probably one > which Rob Clark should at least be made aware of. As I commented last > time, when you're changing register values in an otherwise innocuous > patch, you should comment about them in the patch description. According to the tda9983b documentation, the register TBG_CNTRL_0 is set to 0 at reset time. I think that it is the same for all the tda998x family. In the other hand, this register is supposed to be write only, so reading it may return any value, and the reg_clear() function may set any other bits. Then, clearing one bit is less secure than clearing the full register. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/