lkml.org 
[lkml]   [2018]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC
Date
Hello Radu,

> From: Radu Pirea [mailto:radu.pirea@microchip.com]
> Sent: Thursday, July 26, 2018 7:58 PM
> To: Hayashibara, Keiji/林原 啓二 <hayashibara.keiji@socionext.com>; 'Andy Shevchenko'
> <andy.shevchenko@gmail.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@gmail.com]
> >> Sent: Thursday, July 26, 2018 5:46 PM
> >> To: Hayashibara, Keiji/林原 啓二 <hayashibara.keiji@socionext.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@socionext.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.

I see.
I confirmed __spi_validate() and understood that this check code is unnecessary.
I will remove this check code.

Thank you.

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


-----------------
Best Regards,
Keiji Hayashibara


\
 
 \ /
  Last update: 2018-07-30 03:17    [W:0.051 / U:4.752 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site