From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Cyrus-Session-Id: sloti22d1t05-3496022-1516349027-2-5056891786275067413 X-Sieve: CMU Sieve 3.0 X-Spam-known-sender: yes ("Address greg@kroah.com in From header is in addressbook"); in-addressbook; shared/fdfaecbe-d8f0-4518-a17e-0d89bf6dc529 ("Greg") X-Spam-score: 0.0 X-Spam-hits: BAYES_00 -1.9, HEADER_FROM_DIFFERENT_DOMAINS 0.25, RCVD_IN_DNSWL_HI -5, T_RP_MATCHES_RCVD -0.01, LANGUAGES en, BAYES_USED global, SA_VERSION 3.4.0 X-Spam-source: IP='209.132.180.67', Host='vger.kernel.org', Country='US', FromHeader='com', MailFrom='org' X-Spam-charsets: to='utf-8', cc='utf-8', plain='utf-8' X-Resolved-to: greg@kroah.com X-Delivered-to: greg@kroah.com X-Mail-from: linux-usb-owner@vger.kernel.org ARC-Seal: i=1; a=rsa-sha256; cv=none; d=messagingengine.com; s=arctest; t=1516349026; b=lR656AIVu0gWg3Xr4j5yPfqJPeRcq6oU3vQmsEQVSdILzzY Ae/8YywdQJXIMLDP9R3jpZ3D4zfjvaBCDV1fJ/3PXbq1bQQH0pCK7MUesbsvGiX9 GItxn1qtuGebQMBXoNGJqjOQ64O+eDTVSAfgNW97vWkUVf/6SXDpAjIQ53865v6c QHmojQ0pGyRd/M08H/NHoG2yci8JMbFWiTx/m25UrtT63kWIiElFNBsBHwXtqi6P HInhNdRuIM67KsVva/S0M/Vr7enDF+bzkixD4E7JtWxAjsqFkdEryMwuAL4Tu9jJ a94HBGi5pXm6mYMLwKenLzEuagalqtO6/OLqWig== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=date:from:to:cc:subject:message-id :references:mime-version:content-type:content-transfer-encoding :in-reply-to:sender:list-id; s=arctest; t=1516349026; bh=W7KXcvq 9iA6c5hVRGKZBKjSAkarCT57YN3jLaH97Z5Y=; b=FUJqg0QBNAtSFBaE+zfK1hf vLG2EQJ6rWMvsGXQL7W9QDr3jq05Ts1f5ELn7FA1AUSyDHZ2GDzxR7Vrv2TlM8wP WiQWKkrDGZstxcUFSYEEOx0qalyf8Wm91keFoyaWbUT9jA+3Gz7lFZmKh2LCbeKg sDmV4Ox0Wzvdq2huR9Gc4f22jDRH2IF1J6pSz/5pknTCQ8JMIibRrcvsK3bWV70c 3k/emGtH+Y1bENLajaJqTxQPIPXFVoDvrVWh/6mgpmH8htpuEUHi6cM7W2TxG2JD W6YYdfEsXFCYv71bf30Kfnacd8Wvb/sHVeDMAzjayURlzM8FQxZTzZhVtLOClow= = ARC-Authentication-Results: i=1; mx3.messagingengine.com; arc=none (no signatures found); dkim=fail (body has been altered; 2048-bit rsa key sha256) header.d=messagingengine.com header.i=@messagingengine.com header.b=PRCGY5XD x-bits=2048 x-keytype=rsa x-algorithm=sha256 x-selector=fm1; dmarc=none (p=none,has-list-id=yes,d=none) header.from=kroah.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-usb-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=kroah.com header.result=pass header_is_org_domain=yes Authentication-Results: mx3.messagingengine.com; arc=none (no signatures found); dkim=fail (body has been altered; 2048-bit rsa key sha256) header.d=messagingengine.com header.i=@messagingengine.com header.b=PRCGY5XD x-bits=2048 x-keytype=rsa x-algorithm=sha256 x-selector=fm1; dmarc=none (p=none,has-list-id=yes,d=none) header.from=kroah.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-usb-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=kroah.com header.result=pass header_is_org_domain=yes Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754759AbeASIDm (ORCPT ); Fri, 19 Jan 2018 03:03:42 -0500 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:53041 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751747AbeASIDk (ORCPT ); Fri, 19 Jan 2018 03:03:40 -0500 X-ME-Sender: Date: Fri, 19 Jan 2018 09:03:39 +0100 From: 'Greg KH' To: =?utf-8?B?c2h1ZmFuX2xlZSjmnY7mm7jluIYp?= Cc: ShuFanLee , "heikki.krogerus@linux.intel.com" , =?utf-8?B?Y3lfaHVhbmco6buD5ZWf5Y6fKQ==?= , "linux-kernel@vger.kernel.org" , "linux-usb@vger.kernel.org" Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver Message-ID: <20180119080339.GB31079@kroah.com> References: <1515567552-7692-1-git-send-email-leechu729@gmail.com> <20180117134219.GE3188@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-usb-owner@vger.kernel.org X-Mailing-List: linux-usb@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Thu, Jan 18, 2018 at 01:13:15PM +0000, shufan_lee(李書帆) wrote: > > + > > +#include "rt1711h.h" > > Why a .h file for a single .c file? > > Is the suggestion to move all content in rt1711h.h into rt1711h.c? If it can be, sure, you only need a .h file for things that are shared among other .c files. > > +/* I2C */ > > +atomic_t i2c_busy; > > +atomic_t pm_suspend; > > Why are these atomic? You know that doesn't mean they do not need locking, right? > > For my understanding, a single operation on atomic_t doesn't need lock, like a single atomic_set. > But two consecutive operations doesn't guarantee anything. Like atomic_set followed by an atomic_read. > This part is referenced from fusb302 used to make sure I2C is idle before system suspends. > It only needs to guarantee a single read/write on these variable is atomic operation, so atomic is used. It's atomic for read/write, yes, but that does not mean it can not be instantly changed after the value is read, right? So you might need to look and ensure you are not doing something wrong that can race. A single lock should be simpler than this type of thing, and will be correct. > > +#ifdef CONFIG_DEBUG_FS > > +struct dentry *dbgdir; > > +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX]; > > +struct dentry *dbg_files[RT1711H_DBG_MAX]; > > +int dbg_regidx; > > +struct mutex dbgops_lock; > > +/* lock for log buffer access */ > > +struct mutex logbuffer_lock; > > +int logbuffer_head; > > +int logbuffer_tail; > > +u8 *logbuffer[LOG_BUFFER_ENTRIES]; > > +#endif /* CONFIG_DEBUG_FS */ > > That's a lot of stuff jsut for debugfs. Why do you care about #define at all? The code should not. > > Is the suggestion to remove #ifdef CONFIG_DEBUG_FS? Yes. Or just move it all to another structure that you can dynamically add to this one if needed. > And another 2 locks? Ick, no. > > dbgops_lock is used to prevent user from accessing different debug files simultaneously. > Is the suggestion to use the lock of the following one? > > +/* lock for sharing chip states */ > > +struct mutex lock; Sure, why not? > ======================================================================== > > > +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev)); > > +if (!chip->dbgdir) { > > +chip->dbgdir = debugfs_create_dir(dirname, NULL); > > +if (!chip->dbgdir) > > +return -ENOMEM; > > No need to ever check the return value of debugfs_ calls, you should not care and can always use the value to any future debugfs calls, if you really need it. > > If it is NULL without checking and we use it in debugfs_create_file, all the debug files will be created in the root of the debugfs filesystem. > Is this correct? If it returns NULL then any future calls to debugfs will also not be working, so all will be fine. So there is no need to check this. > ======================================================================== > > > +for (i = 0; i < RT1711H_DBG_MAX; i++) { > > +info = &chip->dbg_info[i]; > > static array of debug info? That feels odd. > > Is the suggestion to use pointer of array and dynamically allocated it? If that makes more sense, it's up to you. Just a suggestion. thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: USB TYPEC: RT1711H Type-C Chip Driver From: Greg KH Message-Id: <20180119080339.GB31079@kroah.com> Date: Fri, 19 Jan 2018 09:03:39 +0100 To: =?utf-8?B?c2h1ZmFuX2xlZSjmnY7mm7jluIYp?= Cc: ShuFanLee , "heikki.krogerus@linux.intel.com" , =?utf-8?B?Y3lfaHVhbmco6buD5ZWf5Y6fKQ==?= , "linux-kernel@vger.kernel.org" , "linux-usb@vger.kernel.org" List-ID: T24gVGh1LCBKYW4gMTgsIDIwMTggYXQgMDE6MTM6MTVQTSArMDAwMCwgc2h1ZmFuX2xlZSjmnY7m m7jluIYpIHdyb3RlOgo+ID4gKwo+ID4gKyNpbmNsdWRlICJydDE3MTFoLmgiCj4gCj4gV2h5IGEg LmggZmlsZSBmb3IgYSBzaW5nbGUgLmMgZmlsZT8KPiAKPiBJcyB0aGUgc3VnZ2VzdGlvbiB0byBt b3ZlIGFsbCBjb250ZW50IGluIHJ0MTcxMWguaCBpbnRvIHJ0MTcxMWguYz8KCklmIGl0IGNhbiBi ZSwgc3VyZSwgeW91IG9ubHkgbmVlZCBhIC5oIGZpbGUgZm9yIHRoaW5ncyB0aGF0IGFyZSBzaGFy ZWQKYW1vbmcgb3RoZXIgLmMgZmlsZXMuCgo+ID4gKy8qIEkyQyAqLwo+ID4gK2F0b21pY190IGky Y19idXN5Owo+ID4gK2F0b21pY190IHBtX3N1c3BlbmQ7Cj4gCj4gV2h5IGFyZSB0aGVzZSBhdG9t aWM/ICBZb3Uga25vdyB0aGF0IGRvZXNuJ3QgbWVhbiB0aGV5IGRvIG5vdCBuZWVkIGxvY2tpbmcs IHJpZ2h0Pwo+IAo+IEZvciBteSB1bmRlcnN0YW5kaW5nLCBhIHNpbmdsZSBvcGVyYXRpb24gb24g YXRvbWljX3QgZG9lc24ndCBuZWVkIGxvY2ssIGxpa2UgYSBzaW5nbGUgYXRvbWljX3NldC4KPiBC dXQgdHdvIGNvbnNlY3V0aXZlIG9wZXJhdGlvbnMgZG9lc24ndCBndWFyYW50ZWUgYW55dGhpbmcu IExpa2UgYXRvbWljX3NldCBmb2xsb3dlZCBieSBhbiBhdG9taWNfcmVhZC4KPiBUaGlzIHBhcnQg aXMgcmVmZXJlbmNlZCBmcm9tIGZ1c2IzMDIgdXNlZCB0byBtYWtlIHN1cmUgSTJDIGlzIGlkbGUg YmVmb3JlIHN5c3RlbSBzdXNwZW5kcy4KPiBJdCBvbmx5IG5lZWRzIHRvIGd1YXJhbnRlZSBhIHNp bmdsZSByZWFkL3dyaXRlIG9uIHRoZXNlIHZhcmlhYmxlIGlzIGF0b21pYyBvcGVyYXRpb24sIHNv IGF0b21pYyBpcyB1c2VkLgoKSXQncyBhdG9taWMgZm9yIHJlYWQvd3JpdGUsIHllcywgYnV0IHRo YXQgZG9lcyBub3QgbWVhbiBpdCBjYW4gbm90IGJlCmluc3RhbnRseSBjaGFuZ2VkIGFmdGVyIHRo ZSB2YWx1ZSBpcyByZWFkLCByaWdodD8gIFNvIHlvdSBtaWdodCBuZWVkIHRvCmxvb2sgYW5kIGVu c3VyZSB5b3UgYXJlIG5vdCBkb2luZyBzb21ldGhpbmcgd3JvbmcgdGhhdCBjYW4gcmFjZS4gIEEK c2luZ2xlIGxvY2sgc2hvdWxkIGJlIHNpbXBsZXIgdGhhbiB0aGlzIHR5cGUgb2YgdGhpbmcsIGFu ZCB3aWxsIGJlCmNvcnJlY3QuCgo+ID4gKyNpZmRlZiBDT05GSUdfREVCVUdfRlMKPiA+ICtzdHJ1 Y3QgZGVudHJ5ICpkYmdkaXI7Cj4gPiArc3RydWN0IHJ0MTcxMWhfZGJnX2luZm8gZGJnX2luZm9b UlQxNzExSF9EQkdfTUFYXTsKPiA+ICtzdHJ1Y3QgZGVudHJ5ICpkYmdfZmlsZXNbUlQxNzExSF9E QkdfTUFYXTsKPiA+ICtpbnQgZGJnX3JlZ2lkeDsKPiA+ICtzdHJ1Y3QgbXV0ZXggZGJnb3BzX2xv Y2s7Cj4gPiArLyogbG9jayBmb3IgbG9nIGJ1ZmZlciBhY2Nlc3MgKi8KPiA+ICtzdHJ1Y3QgbXV0 ZXggbG9nYnVmZmVyX2xvY2s7Cj4gPiAraW50IGxvZ2J1ZmZlcl9oZWFkOwo+ID4gK2ludCBsb2di dWZmZXJfdGFpbDsKPiA+ICt1OCAqbG9nYnVmZmVyW0xPR19CVUZGRVJfRU5UUklFU107Cj4gPiAr I2VuZGlmIC8qIENPTkZJR19ERUJVR19GUyAqLwo+IAo+IFRoYXQncyBhIGxvdCBvZiBzdHVmZiBq c3V0IGZvciBkZWJ1Z2ZzLiAgV2h5IGRvIHlvdSBjYXJlIGFib3V0ICNkZWZpbmUgYXQgYWxsPyAg VGhlIGNvZGUgc2hvdWxkIG5vdC4KPiAKPiBJcyB0aGUgc3VnZ2VzdGlvbiB0byByZW1vdmUgI2lm ZGVmIENPTkZJR19ERUJVR19GUz8KClllcy4gIE9yIGp1c3QgbW92ZSBpdCBhbGwgdG8gYW5vdGhl ciBzdHJ1Y3R1cmUgdGhhdCB5b3UgY2FuIGR5bmFtaWNhbGx5CmFkZCB0byB0aGlzIG9uZSBpZiBu ZWVkZWQuCgo+IEFuZCBhbm90aGVyIDIgbG9ja3M/ICBJY2ssIG5vLgo+IAo+IGRiZ29wc19sb2Nr IGlzIHVzZWQgdG8gcHJldmVudCB1c2VyIGZyb20gYWNjZXNzaW5nIGRpZmZlcmVudCBkZWJ1ZyBm aWxlcyBzaW11bHRhbmVvdXNseS4KPiBJcyB0aGUgc3VnZ2VzdGlvbiB0byB1c2UgdGhlIGxvY2sg b2YgdGhlIGZvbGxvd2luZyBvbmU/Cj4gPiArLyogbG9jayBmb3Igc2hhcmluZyBjaGlwIHN0YXRl cyAqLwo+ID4gK3N0cnVjdCBtdXRleCBsb2NrOwoKU3VyZSwgd2h5IG5vdD8KCj4gPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Cj4gCj4gPiArc25wcmludGYoZGlybmFtZSwgbGVuICsgOSwgInJ0MTcxMWgtJXMiLCBk ZXZfbmFtZShjaGlwLT5kZXYpKTsKPiA+ICtpZiAoIWNoaXAtPmRiZ2Rpcikgewo+ID4gK2NoaXAt PmRiZ2RpciA9IGRlYnVnZnNfY3JlYXRlX2RpcihkaXJuYW1lLCBOVUxMKTsKPiA+ICtpZiAoIWNo aXAtPmRiZ2RpcikKPiA+ICtyZXR1cm4gLUVOT01FTTsKPiAKPiBObyBuZWVkIHRvIGV2ZXIgY2hl Y2sgdGhlIHJldHVybiB2YWx1ZSBvZiBkZWJ1Z2ZzXyBjYWxscywgeW91IHNob3VsZCBub3QgY2Fy ZSBhbmQgY2FuIGFsd2F5cyB1c2UgdGhlIHZhbHVlIHRvIGFueSBmdXR1cmUgZGVidWdmcyBjYWxs cywgaWYgeW91IHJlYWxseSBuZWVkIGl0Lgo+IAo+IElmIGl0IGlzIE5VTEwgd2l0aG91dCBjaGVj a2luZyBhbmQgd2UgdXNlIGl0IGluIGRlYnVnZnNfY3JlYXRlX2ZpbGUsIGFsbCB0aGUgZGVidWcg ZmlsZXMgd2lsbCBiZSBjcmVhdGVkIGluIHRoZSByb290IG9mIHRoZSBkZWJ1Z2ZzIGZpbGVzeXN0 ZW0uCj4gSXMgdGhpcyBjb3JyZWN0PwoKSWYgaXQgcmV0dXJucyBOVUxMIHRoZW4gYW55IGZ1dHVy ZSBjYWxscyB0byBkZWJ1Z2ZzIHdpbGwgYWxzbyBub3QgYmUKd29ya2luZywgc28gYWxsIHdpbGwg YmUgZmluZS4gIFNvIHRoZXJlIGlzIG5vIG5lZWQgdG8gY2hlY2sgdGhpcy4KCj4gPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Cj4gCj4gPiArZm9yIChpID0gMDsgaSA8IFJUMTcxMUhfREJHX01BWDsgaSsrKSB7Cj4g PiAraW5mbyA9ICZjaGlwLT5kYmdfaW5mb1tpXTsKPiAKPiBzdGF0aWMgYXJyYXkgb2YgZGVidWcg aW5mbz8gIFRoYXQgZmVlbHMgb2RkLgo+IAo+IElzIHRoZSBzdWdnZXN0aW9uIHRvIHVzZSBwb2lu dGVyIG9mIGFycmF5IGFuZCBkeW5hbWljYWxseSBhbGxvY2F0ZWQgaXQ/CgpJZiB0aGF0IG1ha2Vz IG1vcmUgc2Vuc2UsIGl0J3MgdXAgdG8geW91LiAgSnVzdCBhIHN1Z2dlc3Rpb24uCgp0aGFua3Ms CgpncmVnIGstaAotLS0KVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxp bmUgInVuc3Vic2NyaWJlIGxpbnV4LXVzYiIgaW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1h am9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcKTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3Zn ZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sCg==