| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9fdfaf4e-5a20-af81-82ef-0d9e327f9133@synopsys.com>
Date: Wed, 25 Jul 2018 09:43:48 +0100
From: Vitor Soares <Vitor.Soares@...opsys.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
Vitor soares <Vitor.Soares@...opsys.com>
CC: Boris Brezillon <boris.brezillon@...tlin.com>,
Wolfram Sang <wsa@...-dreams.de>,
linux-i2c <linux-i2c@...r.kernel.org>,
Jonathan Corbet <corbet@....net>,
Linux Documentation List <linux-doc@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
Przemyslaw Sroka <psroka@...ence.com>,
Arkadiusz Golec <agolec@...ence.com>,
Alan Douglas <adouglas@...ence.com>,
Bartosz Folta <bfolta@...ence.com>,
Damian Kos <dkos@...ence.com>,
Alicja Jurasik-Urbaniak <alicja@...ence.com>,
Cyprian Wronka <cwronka@...ence.com>,
Suresh Punnoose <sureshp@...ence.com>,
Rafal Ciepiela <rafalc@...ence.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Nishanth Menon <nm@...com>, Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
ijc+devicetree <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"Geert Uytterhoeven" <geert@...ux-m68k.org>,
Linus Walleij <linus.walleij@...aro.org>,
Xiang Lin <Xiang.Lin@...aptics.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Sekhar Nori <nsekhar@...com>,
Przemyslaw Gaj <pgaj@...ence.com>,
Peter Rosin <peda@...ntia.se>,
Joao Pinto <Joao.Pinto@...opsys.com>
Subject: Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP
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@...opsys.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
Content of type "text/html" skipped
Powered by blists - more mailing lists