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