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.soa...@synopsys.com> 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 = 0;
>> +
>> + 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 = readl(master->regs + 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
