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 11:46:12 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Keiji Hayashibara <hayashibara.keiji@...ionext.com>
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Masahiro Yamada <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>,
        Kunihiko Hayashi <hayashi.kunihiko@...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.

> +#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.


> +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;

?

> +       /* check if requested speed is too small */
> +       if (ckrat > SSI_MAX_CLK_DIVIDER)

> +               return -EINVAL;

So, does this critical?

> +
> +       if (ckrat < SSI_MIN_CLK_DIVIDER)
> +               ckrat = SSI_MIN_CLK_DIVIDER;

clamp_val()  / 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;

?

> +               goto out_disable_clk;
> +       }

> +static const struct of_device_id uniphier_spi_match[] = {
> +       { .compatible = "socionext,uniphier-scssi", },

> +       { /* sentinel */ },

Slightly better without comma.

> +};
> +MODULE_DEVICE_TABLE(of, uniphier_spi_match);

-- 
With Best Regards,
Andy Shevchenko