linux-kernel - Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 26 Jul 2018 13:57:35 +0300
From:   Radu Pirea <radu.pirea@...rochip.com>
To:     Keiji Hayashibara <hayashibara.keiji@...ionext.com>,
        'Andy Shevchenko' <andy.shevchenko@...il.com>
CC:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Yamada, Masahiro/山田 真弘 
        <yamada.masahiro@...ionext.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hayashi, Kunihiko/林 邦彦 
        <hayashi.kunihiko@...ionext.com>
Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC



On 07/26/2018 12:38 PM, Keiji Hayashibara wrote:
> Hello Andy,
> 
> Thank you for your check!
> 
> 
>> From: Andy Shevchenko [mailto:andy.shevchenko@...il.com]
>> Sent: Thursday, July 26, 2018 5:46 PM
>> To: Hayashibara, Keiji/林原 啓二 <hayashibara.keiji@...ionext.com>
>> Subject: Re: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
>>
>> On Thu, Jul 26, 2018 at 10:09 AM, Keiji Hayashibara <hayashibara.keiji@...ionext.com> wrote:
>>> Add SPI controller driver implemented in Socionext UniPhier SoCs.
>>>
>>> UniPhier SoCs have two types SPI controllers; SCSSI supports a single
>>> channel, and MCSSI supports multiple channels.
>>> This driver supports SCSSI only.
>>>
>>> This controller has 32bit TX/RX FIFO with depth of eight entry, and
>>> supports the SPI master mode only.
>>>
>>> This commit is implemented in PIO transfer mode, not DMA transfer.
>>
>> Few style realted comments.
>>
>>> +#include <asm/unaligned.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/spi/spi.h>
>>
>> Slightly better to keep them in order and put asm/* at the last.
> 
> I see. I will modify this.
> 
> 
>>> +#define SSI_TIMEOUT            2000    /* ms */
>>
>> SSI_TIMEOUT_MS ?
>>
>>> +#define SSI_CTL                        0x0
>>
>> Slightly better to keep same width for the addresses, like 0x00 here.
>>
>>> +#define SSI_CKS                        0x4
>>
>>> +#define SSI_TXWDS              0x8
>>
>>> +#define SSI_RXWDS              0xc
>>
>> Ditto.
> 
> I will modify about above.
> 
>>
>>> +static int uniphier_spi_set_baudrate(struct spi_device *spi, unsigned
>>> +int speed) {
>>> +       struct uniphier_spi_priv *priv = spi_master_get_devdata(spi->master);
>>> +       u32 val, ckrat;
>>> +
>>> +       /*
>>> +        * the supported rates are even numbers from 4 to 254. (4,6,8...254)
>>> +        * round up as we look for equal or less speed
>>> +        */
>>> +       ckrat = DIV_ROUND_UP(clk_get_rate(priv->clk), speed);
>>
>>> +       ckrat = roundup(ckrat, 2);
>>
>> ckrat += ckrat & 1;
>>
>> ?
> 
> It's simple. I will modify.
> 
> 
>>> +       /* check if requested speed is too small */
>>> +       if (ckrat > SSI_MAX_CLK_DIVIDER)
>>
>>> +               return -EINVAL;
>>
>> So, does this critical?
> 
> If set the value to SSI_MAX_CLK_DIVIDER, the clock frequency will be set high.
> I don't change it to high frequency, and it is daringly an error.
> On the other hand, when changing to low frequency, I will change it automatically.
> 
>>> +
>>> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
>>> +               ckrat = SSI_MIN_CLK_DIVIDER;

In fact you don't need this checks. You already set in probe function this.
master->max_speed_hz = DIV_ROUND_UP(clksrc, SSI_MIN_CLK_DIVIDER);
master->min_speed_hz = DIV_ROUND_UP(clksrc, SSI_MAX_CLK_DIVIDER);

The SPI core will check if transfer speed is higher than controller 
speed and if is, will set the transfer speed to master->max_speed_hz. In 
case of master->min_speed_hz, if transfer speed is lower than 
master->min_speed_hz __spi_validate(drivers/spi/spi.c) will return -EINVAL.

>>
>> clamp_val()  / max() ?
> 
> I will modify it to use max().
> 
>>
>>> +       val = readl(priv->base + SSI_CKS);
>>> +       val &= ~SSI_CKS_CKRAT_MASK;
>>> +       val |= ckrat & SSI_CKS_CKRAT_MASK;
>>> +       writel(val, priv->base + SSI_CKS);
>>> +
>>> +       return 0;
>>> +}
>>
>>> +       priv->irq = platform_get_irq(pdev, 0);
>>> +       if (priv->irq < 0) {
>>> +               dev_err(&pdev->dev, "failed to get IRQ\n");
>>
>>> +               ret = -ENXIO;
>>
>> What's wrong with
>>
>> ret = priv->irq;
>>
>> ?
> 
> I will modify it.
> 
>>> +               goto out_disable_clk;
>>> +       }
>>
>>> +static const struct of_device_id uniphier_spi_match[] = {
>>> +       { .compatible = "socionext,uniphier-scssi", },
>>
>>> +       { /* sentinel */ },
>>
>> Slightly better without comma.
> 
> OK. I will modify this.
> 
> -----------------
> Best Regards,
> Keiji Hayashibara
> 
> 
> 
>>> +};
>>> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>