On Wed, Jul 25, 2018 at 11:43 AM, Vitor Soares <Vitor.Soares@xxxxxxxxxxxx> wrote: > On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares > <Vitor.Soares@xxxxxxxxxxxx> wrote: > Thanks for answers, my comments below. > This patch add driver for Synopsys DesignWare IP on top of > I3C subsystem patchset proposal V6 ... > +#include <linux/reset.h> > Reset API. > > All of them required? Why? Thanks, got it. > There is some header files that are already included by others header files. > Should I add them too? it there any rule for that? No need. Usually we drop some "wired" headers (when we sure that one will always include the other one, like module.h vs. init.h) > + writel(cmd->cmd_hi, master->regs + COMMAND_QUEUE_PORT); > + writel(cmd->cmd_lo, master->regs + COMMAND_QUEUE_PORT); > > hmm... writesl()? > Is there any advantage here? Here maybe not. Just a material to think about. If you can refactor code to utilize them, good. > + info->pid = (u64)readl(master->regs + SLV_PID_VALUE); > > Why explicit casting? > info->pid is u64 size. In C standard there is an integer promotion which allows you not to use explicit casting in such cases. > + u32 r; > + > + > + core_rate = clk_get_rate(master->core_clk); > > Too many blank lines in between. > > > For me in that way it's better to filter code parts. Do you think that is > not readable? The point is it's useless. On the other hand, you have a lot of inconsistency with that style. > + p = (ret >> 6) ^ (ret >> 5) ^ (ret >> 4) ^ (ret >> 3) ^ > + (ret >> 2) ^ (ret >> 1) ^ ret ^ 1; > + p = p & 1; > > Is it parity calculus? Do we have something implemented in kernel already? > > Btw, > https://urldefense.proofpoint.com/v2/url?u=https-3A__graphics.stanford.edu_-7Eseander_bithacks.html-23ParityNaive&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=5FpGHBbT8tYA6PB4RT_9O6PJk3v-wYcy1MV59xoqK4I&s=FSJ3EcuoxPtRJWmsk9Yt4s_UH9kxFBam01Xvas2ZFdo&e= > offered this > > v ^= v >> 4; > v &= 0xf; > v = (0x6996 >> v) & 1; > > > I search into the kernel and I didn't find any function for that. In your > opinion what shoud I use? If license of the piece above is okay to use in kernel, then definitely it would be better (even we might create a helper out of it). -- With Best Regards, Andy Shevchenko
