Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP
From: Andy Shevchenko
Date: Wed Jul 25 2018 - 12:56:42 EST
On Wed, Jul 25, 2018 at 11:43 AM, Vitor Soares
> 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?
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?
> 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
With Best Regards,