Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP

[Vitor Soares]

Hi Andy,

Ãs 4:35 PM de 7/21/2018, Andy Shevchenko escreveu:

On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares
<Vitor.Soares@xxxxxxxxxxxx> wrote:

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

Some of comments below related to style.
But unaligned helpers I think is good to use.

+#include <linux/bitops.h>

Bit operations API eg: GENMASK...

+#include <linux/clk.h>

Clock API eg: master->core_clk = devm_clk_get(&pdev->dev, "core_clk");

+#include <linux/completion.h>

Completion API eg: struct completion

+#include <linux/err.h>

Check kernel pointer eg: return PTR_ERR(master->regs);

+#include <linux/errno.h>

Error codes eg: return -ENOTSUPP;

+#include <linux/i3c/master.h>

I3C Master API eg: i3c_master_register()

+#include <linux/init.h>

Not needed.

+#include <linux/interrupt.h>

Interrupt API eg: devm_request_irq().

+#include <linux/io.h>
+#include <linux/ioport.h>

Used to get io resource.

+#include <linux/iopoll.h>

Âthis function: readl_poll_timeout_atomic().

+#include <linux/module.h>

Module API.

+#include <linux/platform_device.h>

Platform driver API.

+#include <linux/reset.h>

Reset API.

All of them required? Why?

There is some header files that are already included by others header files. Should I add them too? it there any rule for that?
Thank for the advice.

+       default:

Just return false here?

Yes, it makes more sense.

+               break;
+       }
+
+       return false;

+       for (i = 0; i < nbytes; i += 4) {
+               u32 data = ""
+

+               for (j = 0; j < 4 && (i + j) < nbytes; j++)
+                       data |= (u32)bytes[i + j] << (j * 8);

NIH of get_unaligned_le32()

+
+               writel(data, master->regs + RX_TX_DATA_PORT);
+       }
+}
+
+static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
+                                      u8 *bytes, int nbytes)
+{
+       int i, j;
+
+       for (i = 0; i < nbytes; i += 4) {
+               u32 data;
+
+               data = "" + RX_TX_DATA_PORT);
+

+               for (j = 0; j < 4 && (i + j) < nbytes; j++)
+                       bytes[i + j] = data >> (j * 8);

Ditto put_unaligned_le32() ?

+       }
+}

I'm wondering what else you open coded instead of using helpers we already have.

I will see how it works to implement.

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

Probably I can use it to fill the TX buffer with this.

+       info->pid = (u64)readl(master->regs + SLV_PID_VALUE);

Why explicit casting?

info->pid is u64 size.

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

+
+

Ditto.

+       /* Prepare DAT before launching DAA. */
+       for (pos = 0; pos < master->maxdevs; pos++) {
+               if (olddevs & BIT(pos))
+                       continue;
+
+               ret = i3c_master_get_free_addr(m, last_addr + 1);
+               if (ret < 0)
+                       return -ENOSPC;
+               master->addrs[pos] = ret;

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


Thanks for your feedback.

Regards,
Vitor Soares