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

[Vitor Soares <Vitor.Soares@synopsys.com>]

[-- Attachment #1: Type: text/plain, Size: 4251 bytes --]

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@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


[-- Attachment #2: Type: text/html, Size: 10545 bytes --]