Hello Radu, > From: Radu Pirea [mailto:radu.pirea@xxxxxxxxxxxxx] > Sent: Thursday, July 26, 2018 7:58 PM > To: Hayashibara, Keiji/林原 啓二 <hayashibara.keiji@xxxxxxxxxxxxx>; 'Andy Shevchenko' > <andy.shevchenko@xxxxxxxxx> > 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@xxxxxxxxx] > >> Sent: Thursday, July 26, 2018 5:46 PM > >> To: Hayashibara, Keiji/林原 啓二 <hayashibara.keiji@xxxxxxxxxxxxx> > >> 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@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
