Messages in this thread | | | Subject | Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP | From | vitor <> | Date | Wed, 8 Aug 2018 18:01:55 +0100 |
| |
Hi Andy,
On 25-07-2018 17:56, Andy Shevchenko wrote: > On Wed, Jul 25, 2018 at 11:43 AM, Vitor Soares > <Vitor.Soares@synopsys.com> wrote: >> On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares >> <Vitor.Soares@synopsys.com> 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). > Thanks for your comments I will take them into account for the next version.
Best regards, Vitor Soares
| |