Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

['Greg KH']

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