Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp547887imm; Wed, 25 Jul 2018 01:45:08 -0700 (PDT) X-Google-Smtp-Source: AAOMgpd6qPDCYq6T52Oi3/u4zWwbQEpt70fRvyI9SrO0cvzDgFSQF24Ni+C7+heOVnw+8Y1/qgOW X-Received: by 2002:a62:d10b:: with SMTP id z11-v6mr21214270pfg.255.1532508308192; Wed, 25 Jul 2018 01:45:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532508308; cv=none; d=google.com; s=arc-20160816; b=kmmnOkNU198mABHPguo7Z5qZ3yi4taIAm7ciY625WAYyeMnWBFcRR6bWWSyVa1PMb0 pry+UNZAcarQMTYqeCGzad9QtcuvpyfPK8AQ3mSj0G5KW38VMZoydOJAhEfB+6xmutJ6 UExhZKV6AswxsPzjI7rG6XJ+/KgJfvo/l6NfGMeFyx0jxrK3SllRNR1A0ieftD+CsFxd ksHFnwkbB3f0j3CKBPQS6hTTtO8HQEZU9DGv4A+NxwiDVr4fWy6rKICkYnjIl52TGBqQ fRzVUvEVUTJeC5yITSRv8a0MW24dF7v+326vVxPUrcKVARb3RqK9codVTQXt/L3ThJI6 RMOQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-language:in-reply-to:mime-version :user-agent:date:message-id:references:cc:to:subject:from :dkim-signature:arc-authentication-results; bh=3MK+LdPpVO+NZDMXehduw/aCjye2+79kgtTDvCpW5Hw=; b=TsHk8LGoILVPmQxmlWBVGdhXH8ilQswMy6NR+00JwVZ0OvsxaFqI/bel00Q3cVnsvw pU6R9OGm8gXXOL9FSwOWEJhVglVdXN+v1q+FqjrWfmQsoARrBx1+3uEZq1IqBcbIk6qn p0mx5xrGf2UNiT04EQClJGiJNg7raplBatFPlKi+gUXIEOkwjcaFbqRFhV4NOOal26Ey 9/4a4JiG57e+odL9lOc0NlPJJhfk0KJyfQlmB+J+PHRuDkqPhsa35fWDw3mH2KVuGxoi UxEI1nYWtZBoIIZrJ+vSf4YweZE7e/rjJvGL61gipOC6XM7ykPOJneJGy2rLB1pz6Ntx fMXw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@synopsys.com header.s=mail header.b=hWdWClzn; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=synopsys.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id v127-v6si12184933pgv.89.2018.07.25.01.44.52; Wed, 25 Jul 2018 01:45:08 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@synopsys.com header.s=mail header.b=hWdWClzn; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=synopsys.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728652AbeGYJyj (ORCPT + 99 others); Wed, 25 Jul 2018 05:54:39 -0400 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:53766 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728222AbeGYJyi (ORCPT ); Wed, 25 Jul 2018 05:54:38 -0400 Received: from mailhost.synopsys.com (mailhost1.synopsys.com [10.12.238.239]) by smtprelay.synopsys.com (Postfix) with ESMTP id A169C10C0F00; Wed, 25 Jul 2018 01:43:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=synopsys.com; s=mail; t=1532508238; bh=zRex2I7G+BUDfqQjn3xv+mysdbo1m0+RAwiIcmNxdsY=; h=From:Subject:To:CC:References:Date:In-Reply-To:From; b=hWdWClznbbQ8DcTxRIjf+64XJt7Jvel7FP7NvDKkTqRm4luzvIMvYDsecUg0vcyjs FvAaYSf3kYz66joFMazhEt5YMKmHnBcmTXFC4RrOcXZdGXv3oOP8feZBsJ2g8xsrYZ 2yMHGiYDvgKwWmlaE3MIqwi5Ac8SF30kB0rokErE1LXjBNMnjAavxrczzl7L57D1vl Wt6KXJ6Z1JqBWuqdgg5CbSvU/1mhnJyFJYWWsCCzxHdwDmjuVXPJnWAh/QbSWUNo4O 6nW/dNvMKsWmJpLCiKMwc+UpkDqgiR7g99HMDKM5/Z2ynJ9sQqPRGIJloJRjysgmq2 A8MlZy8o9Q0Tg== Received: from US01WXQAHTC1.internal.synopsys.com (us01wxqahtc1.internal.synopsys.com [10.12.238.230]) by mailhost.synopsys.com (Postfix) with ESMTP id E6F1A55A7; Wed, 25 Jul 2018 01:43:54 -0700 (PDT) Received: from DE02WEHTCB.internal.synopsys.com (10.225.19.94) by US01WXQAHTC1.internal.synopsys.com (10.12.238.230) with Microsoft SMTP Server (TLS) id 14.3.361.1; Wed, 25 Jul 2018 01:43:53 -0700 Received: from DE02WEHTCA.internal.synopsys.com (10.225.19.92) by DE02WEHTCB.internal.synopsys.com (10.225.19.94) with Microsoft SMTP Server (TLS) id 14.3.361.1; Wed, 25 Jul 2018 10:43:52 +0200 Received: from [10.225.2.53] (10.225.2.53) by DE02WEHTCA.internal.synopsys.com (10.225.19.80) with Microsoft SMTP Server (TLS) id 14.3.361.1; Wed, 25 Jul 2018 10:43:51 +0200 From: Vitor Soares Subject: Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP To: Andy Shevchenko , Vitor soares CC: Boris Brezillon , Wolfram Sang , linux-i2c , Jonathan Corbet , Linux Documentation List , Greg Kroah-Hartman , Arnd Bergmann , Przemyslaw Sroka , Arkadiusz Golec , Alan Douglas , Bartosz Folta , Damian Kos , Alicja Jurasik-Urbaniak , Cyprian Wronka , Suresh Punnoose , Rafal Ciepiela , Thomas Petazzoni , Nishanth Menon , Rob Herring , Pawel Moll , Mark Rutland , ijc+devicetree , Kumar Gala , devicetree , Linux Kernel Mailing List , "Geert Uytterhoeven" , Linus Walleij , Xiang Lin , "open list:GPIO SUBSYSTEM" , Sekhar Nori , Przemyslaw Gaj , Peter Rosin , Joao Pinto References: <1532120272-17157-1-git-send-email-soares@synopsys.com> <1532120272-17157-2-git-send-email-soares@synopsys.com> Message-ID: <9fdfaf4e-5a20-af81-82ef-0d9e327f9133@synopsys.com> Date: Wed, 25 Jul 2018 09:43:48 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------99CD8D8374B6CE31411AECC4" Content-Language: pt-PT X-Originating-IP: [10.225.2.53] Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --------------99CD8D8374B6CE31411AECC4 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Hi Andy, Às 4:35 PM de 7/21/2018, Andy Shevchenko escreveu: > On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares > 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 Bit operations API eg: GENMASK... >> +#include Clock API eg: master->core_clk = devm_clk_get(&pdev->dev, "core_clk"); >> +#include Completion API eg: struct completion >> +#include Check kernel pointer eg: return PTR_ERR(master->regs); >> +#include Error codes eg: return -ENOTSUPP; >> +#include I3C Master API eg: i3c_master_register() >> +#include Not needed. >> +#include Interrupt API eg: devm_request_irq(). >> +#include >> +#include Used to get io resource. >> +#include  this function: readl_poll_timeout_atomic(). >> +#include Module API. >> +#include Platform driver API. >> +#include 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 --------------99CD8D8374B6CE31411AECC4 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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

    

  


--------------99CD8D8374B6CE31411AECC4--