Hi Andy,


On 25-07-2018 17:56, Andy Shevchenko wrote:

On Wed, Jul 25, 2018 at 11:43 AM, Vitor Soares
[off-list ref] wrote:

quoted

On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares
[off-list ref] wrote:

Thanks for answers, my comments below.

quoted

This patch add driver for Synopsys DesignWare IP on top of
I3C subsystem patchset proposal V6

...

quoted

+#include <linux/reset.h>
Reset API.

All of them required? Why?

Thanks, got it.

quoted

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)

quoted

+               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.

quoted

+       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.

quoted

+       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.

quoted

+               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