From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D92BC46462 for ; Mon, 30 Jul 2018 01:16:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D79E420881 for ; Mon, 30 Jul 2018 01:16:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D79E420881 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=socionext.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728708AbeG3Crm convert rfc822-to-8bit (ORCPT ); Sun, 29 Jul 2018 22:47:42 -0400 Received: from mx.socionext.com ([202.248.49.38]:54164 "EHLO mx.socionext.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728625AbeG3Crm (ORCPT ); Sun, 29 Jul 2018 22:47:42 -0400 Received: from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54]) by mx.socionext.com with ESMTP; 30 Jul 2018 10:15:10 +0900 Received: from mail.mfilter.local (m-filter-2 [10.213.24.62]) by iyokan-ex.css.socionext.com (Postfix) with ESMTP id 6247D60034; Mon, 30 Jul 2018 10:15:10 +0900 (JST) Received: from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP; Mon, 30 Jul 2018 10:15:10 +0900 Received: from yuzu.css.socionext.com (yuzu [172.31.8.45]) by iyokan.css.socionext.com (Postfix) with ESMTP id DBE95403C7; Mon, 30 Jul 2018 10:15:09 +0900 (JST) Received: from DESKTOP0FARE34 (unknown [10.213.134.218]) by yuzu.css.socionext.com (Postfix) with ESMTP id 8E8CE120424; Mon, 30 Jul 2018 10:15:09 +0900 (JST) From: "Keiji Hayashibara" To: "'Radu Pirea'" , "'Andy Shevchenko'" Cc: "Mark Brown" , "Rob Herring" , "Mark Rutland" , =?utf-8?B?WWFtYWRhLCBNYXNhaGlyby/lsbHnlLAg55yf5byY?= , "linux-spi" , "linux-arm Mailing List" , "devicetree" , "Masami Hiramatsu" , "Jassi Brar" , "Linux Kernel Mailing List" , =?utf-8?Q?Hayashi=2C_Kunihiko/=E6=9E=97_=E9=82=A6=E5=BD=A6?= References: <1532588943-19481-1-git-send-email-hayashibara.keiji@socionext.com> <1532588943-19481-3-git-send-email-hayashibara.keiji@socionext.com> <001b01d424c4$66e04230$34a0c690$@socionext.com> In-Reply-To: Subject: RE: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC Date: Mon, 30 Jul 2018 10:15:06 +0900 Message-ID: <002001d427a2$c05ca7f0$4115f7d0$@socionext.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Outlook 15.0 Thread-Index: AQHUJK+Lb4PtBS+ckEWxak8O5zFtw6SgmfYAgACbT7D//4lngIAGO07Q Content-Language: ja x-securitypolicycheck: OK by SHieldMailChecker v2.5.2 x-shieldmailcheckerpolicyversion: POLICY180220 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Radu, > From: Radu Pirea [mailto:radu.pirea@microchip.com] > Sent: Thursday, July 26, 2018 7:58 PM > To: Hayashibara, Keiji/林原 啓二 ; 'Andy Shevchenko' > > 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/林原 啓二 > >> 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 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 > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >> > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Keiji Hayashibara" Subject: RE: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC Date: Mon, 30 Jul 2018 10:15:06 +0900 Message-ID: <002001d427a2$c05ca7f0$4115f7d0$@socionext.com> References: <1532588943-19481-1-git-send-email-hayashibara.keiji@socionext.com> <1532588943-19481-3-git-send-email-hayashibara.keiji@socionext.com> <001b01d424c4$66e04230$34a0c690$@socionext.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: Content-Language: ja List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: 'Radu Pirea' , 'Andy Shevchenko' Cc: Mark Rutland , devicetree , =?utf-8?Q?Hayashi=2C_Kunihiko/=E6=9E=97_=E9=82=A6=E5=BD=A6?= , Masami Hiramatsu , Jassi Brar , Linux Kernel Mailing List , Rob Herring , linux-spi , =?utf-8?B?WWFtYWRhLCBNYXNhaGlyby/lsbHnlLAg55yf5byY?= , Mark Brown , linux-arm Mailing List List-Id: devicetree@vger.kernel.org SGVsbG8gUmFkdSwKCj4gRnJvbTogUmFkdSBQaXJlYSBbbWFpbHRvOnJhZHUucGlyZWFAbWljcm9j aGlwLmNvbV0KPiBTZW50OiBUaHVyc2RheSwgSnVseSAyNiwgMjAxOCA3OjU4IFBNCj4gVG86IEhh eWFzaGliYXJhLCBLZWlqaS/mnpfljp8g5ZWT5LqMIDxoYXlhc2hpYmFyYS5rZWlqaUBzb2Npb25l eHQuY29tPjsgJ0FuZHkgU2hldmNoZW5rbycKPiA8YW5keS5zaGV2Y2hlbmtvQGdtYWlsLmNvbT4K PiBTdWJqZWN0OiBSZTogW1BBVENIIHYyIDIvMl0gc3BpOiBhZGQgU1BJIGNvbnRyb2xsZXIgZHJp dmVyIGZvciBVbmlQaGllciBTb0MKPiAKPiAKPiAKPiBPbiAwNy8yNi8yMDE4IDEyOjM4IFBNLCBL ZWlqaSBIYXlhc2hpYmFyYSB3cm90ZToKPiA+IEhlbGxvIEFuZHksCj4gPgo+ID4gVGhhbmsgeW91 IGZvciB5b3VyIGNoZWNrIQo+ID4KPiA+Cj4gPj4gRnJvbTogQW5keSBTaGV2Y2hlbmtvIFttYWls dG86YW5keS5zaGV2Y2hlbmtvQGdtYWlsLmNvbV0KPiA+PiBTZW50OiBUaHVyc2RheSwgSnVseSAy NiwgMjAxOCA1OjQ2IFBNCj4gPj4gVG86IEhheWFzaGliYXJhLCBLZWlqaS/mnpfljp8g5ZWT5LqM IDxoYXlhc2hpYmFyYS5rZWlqaUBzb2Npb25leHQuY29tPgo+ID4+IFN1YmplY3Q6IFJlOiBbUEFU Q0ggdjIgMi8yXSBzcGk6IGFkZCBTUEkgY29udHJvbGxlciBkcml2ZXIgZm9yCj4gPj4gVW5pUGhp ZXIgU29DCj4gPj4KPiA+PiBPbiBUaHUsIEp1bCAyNiwgMjAxOCBhdCAxMDowOSBBTSwgS2Vpamkg SGF5YXNoaWJhcmEgPGhheWFzaGliYXJhLmtlaWppQHNvY2lvbmV4dC5jb20+IHdyb3RlOgo+ID4+ PiBBZGQgU1BJIGNvbnRyb2xsZXIgZHJpdmVyIGltcGxlbWVudGVkIGluIFNvY2lvbmV4dCBVbmlQ aGllciBTb0NzLgo+ID4+Pgo+ID4+PiBVbmlQaGllciBTb0NzIGhhdmUgdHdvIHR5cGVzIFNQSSBj b250cm9sbGVyczsgU0NTU0kgc3VwcG9ydHMgYQo+ID4+PiBzaW5nbGUgY2hhbm5lbCwgYW5kIE1D U1NJIHN1cHBvcnRzIG11bHRpcGxlIGNoYW5uZWxzLgo+ID4+PiBUaGlzIGRyaXZlciBzdXBwb3J0 cyBTQ1NTSSBvbmx5Lgo+ID4+Pgo+ID4+PiBUaGlzIGNvbnRyb2xsZXIgaGFzIDMyYml0IFRYL1JY IEZJRk8gd2l0aCBkZXB0aCBvZiBlaWdodCBlbnRyeSwgYW5kCj4gPj4+IHN1cHBvcnRzIHRoZSBT UEkgbWFzdGVyIG1vZGUgb25seS4KPiA+Pj4KPiA+Pj4gVGhpcyBjb21taXQgaXMgaW1wbGVtZW50 ZWQgaW4gUElPIHRyYW5zZmVyIG1vZGUsIG5vdCBETUEgdHJhbnNmZXIuCj4gPj4KPiA+PiBGZXcg c3R5bGUgcmVhbHRlZCBjb21tZW50cy4KPiA+Pgo+ID4+PiArI2luY2x1ZGUgPGFzbS91bmFsaWdu ZWQuaD4KPiA+Pj4gKyNpbmNsdWRlIDxsaW51eC9rZXJuZWwuaD4KPiA+Pj4gKyNpbmNsdWRlIDxs aW51eC9iaXRmaWVsZC5oPgo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L2JpdG9wcy5oPgo+ID4+PiAr I2luY2x1ZGUgPGxpbnV4L2Nsay5oPgo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L2ludGVycnVwdC5o Pgo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L2lvLmg+Cj4gPj4+ICsjaW5jbHVkZSA8bGludXgvbW9k dWxlLmg+Cj4gPj4+ICsjaW5jbHVkZSA8bGludXgvb2YuaD4KPiA+Pj4gKyNpbmNsdWRlIDxsaW51 eC9vZl9wbGF0Zm9ybS5oPgo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L3BsYXRmb3JtX2RldmljZS5o Pgo+ID4+PiArI2luY2x1ZGUgPGxpbnV4L3NwaS9zcGkuaD4KPiA+Pgo+ID4+IFNsaWdodGx5IGJl dHRlciB0byBrZWVwIHRoZW0gaW4gb3JkZXIgYW5kIHB1dCBhc20vKiBhdCB0aGUgbGFzdC4KPiA+ Cj4gPiBJIHNlZS4gSSB3aWxsIG1vZGlmeSB0aGlzLgo+ID4KPiA+Cj4gPj4+ICsjZGVmaW5lIFNT SV9USU1FT1VUICAgICAgICAgICAgMjAwMCAgICAvKiBtcyAqLwo+ID4+Cj4gPj4gU1NJX1RJTUVP VVRfTVMgPwo+ID4+Cj4gPj4+ICsjZGVmaW5lIFNTSV9DVEwgICAgICAgICAgICAgICAgICAgICAg ICAweDAKPiA+Pgo+ID4+IFNsaWdodGx5IGJldHRlciB0byBrZWVwIHNhbWUgd2lkdGggZm9yIHRo ZSBhZGRyZXNzZXMsIGxpa2UgMHgwMCBoZXJlLgo+ID4+Cj4gPj4+ICsjZGVmaW5lIFNTSV9DS1Mg ICAgICAgICAgICAgICAgICAgICAgICAweDQKPiA+Pgo+ID4+PiArI2RlZmluZSBTU0lfVFhXRFMg ICAgICAgICAgICAgIDB4OAo+ID4+Cj4gPj4+ICsjZGVmaW5lIFNTSV9SWFdEUyAgICAgICAgICAg ICAgMHhjCj4gPj4KPiA+PiBEaXR0by4KPiA+Cj4gPiBJIHdpbGwgbW9kaWZ5IGFib3V0IGFib3Zl Lgo+ID4KPiA+Pgo+ID4+PiArc3RhdGljIGludCB1bmlwaGllcl9zcGlfc2V0X2JhdWRyYXRlKHN0 cnVjdCBzcGlfZGV2aWNlICpzcGksCj4gPj4+ICt1bnNpZ25lZCBpbnQgc3BlZWQpIHsKPiA+Pj4g KyAgICAgICBzdHJ1Y3QgdW5pcGhpZXJfc3BpX3ByaXYgKnByaXYgPSBzcGlfbWFzdGVyX2dldF9k ZXZkYXRhKHNwaS0+bWFzdGVyKTsKPiA+Pj4gKyAgICAgICB1MzIgdmFsLCBja3JhdDsKPiA+Pj4g Kwo+ID4+PiArICAgICAgIC8qCj4gPj4+ICsgICAgICAgICogdGhlIHN1cHBvcnRlZCByYXRlcyBh cmUgZXZlbiBudW1iZXJzIGZyb20gNCB0byAyNTQuICg0LDYsOC4uLjI1NCkKPiA+Pj4gKyAgICAg ICAgKiByb3VuZCB1cCBhcyB3ZSBsb29rIGZvciBlcXVhbCBvciBsZXNzIHNwZWVkCj4gPj4+ICsg ICAgICAgICovCj4gPj4+ICsgICAgICAgY2tyYXQgPSBESVZfUk9VTkRfVVAoY2xrX2dldF9yYXRl KHByaXYtPmNsayksIHNwZWVkKTsKPiA+Pgo+ID4+PiArICAgICAgIGNrcmF0ID0gcm91bmR1cChj a3JhdCwgMik7Cj4gPj4KPiA+PiBja3JhdCArPSBja3JhdCAmIDE7Cj4gPj4KPiA+PiA/Cj4gPgo+ ID4gSXQncyBzaW1wbGUuIEkgd2lsbCBtb2RpZnkuCj4gPgo+ID4KPiA+Pj4gKyAgICAgICAvKiBj aGVjayBpZiByZXF1ZXN0ZWQgc3BlZWQgaXMgdG9vIHNtYWxsICovCj4gPj4+ICsgICAgICAgaWYg KGNrcmF0ID4gU1NJX01BWF9DTEtfRElWSURFUikKPiA+Pgo+ID4+PiArICAgICAgICAgICAgICAg cmV0dXJuIC1FSU5WQUw7Cj4gPj4KPiA+PiBTbywgZG9lcyB0aGlzIGNyaXRpY2FsPwo+ID4KPiA+ IElmIHNldCB0aGUgdmFsdWUgdG8gU1NJX01BWF9DTEtfRElWSURFUiwgdGhlIGNsb2NrIGZyZXF1 ZW5jeSB3aWxsIGJlIHNldCBoaWdoLgo+ID4gSSBkb24ndCBjaGFuZ2UgaXQgdG8gaGlnaCBmcmVx dWVuY3ksIGFuZCBpdCBpcyBkYXJpbmdseSBhbiBlcnJvci4KPiA+IE9uIHRoZSBvdGhlciBoYW5k LCB3aGVuIGNoYW5naW5nIHRvIGxvdyBmcmVxdWVuY3ksIEkgd2lsbCBjaGFuZ2UgaXQgYXV0b21h dGljYWxseS4KPiA+Cj4gPj4+ICsKPiA+Pj4gKyAgICAgICBpZiAoY2tyYXQgPCBTU0lfTUlOX0NM S19ESVZJREVSKQo+ID4+PiArICAgICAgICAgICAgICAgY2tyYXQgPSBTU0lfTUlOX0NMS19ESVZJ REVSOwo+IAo+IEluIGZhY3QgeW91IGRvbid0IG5lZWQgdGhpcyBjaGVja3MuIFlvdSBhbHJlYWR5 IHNldCBpbiBwcm9iZSBmdW5jdGlvbiB0aGlzLgo+IG1hc3Rlci0+bWF4X3NwZWVkX2h6ID0gRElW X1JPVU5EX1VQKGNsa3NyYywgU1NJX01JTl9DTEtfRElWSURFUik7Cj4gbWFzdGVyLT5taW5fc3Bl ZWRfaHogPSBESVZfUk9VTkRfVVAoY2xrc3JjLCBTU0lfTUFYX0NMS19ESVZJREVSKTsKPiAKPiBU aGUgU1BJIGNvcmUgd2lsbCBjaGVjayBpZiB0cmFuc2ZlciBzcGVlZCBpcyBoaWdoZXIgdGhhbiBj b250cm9sbGVyIHNwZWVkIGFuZCBpZiBpcywgd2lsbCBzZXQgdGhlIHRyYW5zZmVyIHNwZWVkCj4g dG8gbWFzdGVyLT5tYXhfc3BlZWRfaHouIEluIGNhc2Ugb2YgbWFzdGVyLT5taW5fc3BlZWRfaHos IGlmIHRyYW5zZmVyIHNwZWVkIGlzIGxvd2VyIHRoYW4KPiBtYXN0ZXItPm1pbl9zcGVlZF9oeiBf X3NwaV92YWxpZGF0ZShkcml2ZXJzL3NwaS9zcGkuYykgd2lsbCByZXR1cm4gLUVJTlZBTC4KCkkg c2VlLgpJIGNvbmZpcm1lZCBfX3NwaV92YWxpZGF0ZSgpIGFuZCB1bmRlcnN0b29kIHRoYXQgdGhp cyBjaGVjayBjb2RlIGlzIHVubmVjZXNzYXJ5LgpJIHdpbGwgcmVtb3ZlIHRoaXMgY2hlY2sgY29k ZS4KClRoYW5rIHlvdS4KCj4gPj4KPiA+PiBjbGFtcF92YWwoKSAgLyBtYXgoKSA/Cj4gPgo+ID4g SSB3aWxsIG1vZGlmeSBpdCB0byB1c2UgbWF4KCkuCj4gPgo+ID4+Cj4gPj4+ICsgICAgICAgdmFs ID0gcmVhZGwocHJpdi0+YmFzZSArIFNTSV9DS1MpOwo+ID4+PiArICAgICAgIHZhbCAmPSB+U1NJ X0NLU19DS1JBVF9NQVNLOwo+ID4+PiArICAgICAgIHZhbCB8PSBja3JhdCAmIFNTSV9DS1NfQ0tS QVRfTUFTSzsKPiA+Pj4gKyAgICAgICB3cml0ZWwodmFsLCBwcml2LT5iYXNlICsgU1NJX0NLUyk7 Cj4gPj4+ICsKPiA+Pj4gKyAgICAgICByZXR1cm4gMDsKPiA+Pj4gK30KPiA+Pgo+ID4+PiArICAg ICAgIHByaXYtPmlycSA9IHBsYXRmb3JtX2dldF9pcnEocGRldiwgMCk7Cj4gPj4+ICsgICAgICAg aWYgKHByaXYtPmlycSA8IDApIHsKPiA+Pj4gKyAgICAgICAgICAgICAgIGRldl9lcnIoJnBkZXYt PmRldiwgImZhaWxlZCB0byBnZXQgSVJRXG4iKTsKPiA+Pgo+ID4+PiArICAgICAgICAgICAgICAg cmV0ID0gLUVOWElPOwo+ID4+Cj4gPj4gV2hhdCdzIHdyb25nIHdpdGgKPiA+Pgo+ID4+IHJldCA9 IHByaXYtPmlycTsKPiA+Pgo+ID4+ID8KPiA+Cj4gPiBJIHdpbGwgbW9kaWZ5IGl0Lgo+ID4KPiA+ Pj4gKyAgICAgICAgICAgICAgIGdvdG8gb3V0X2Rpc2FibGVfY2xrOwo+ID4+PiArICAgICAgIH0K PiA+Pgo+ID4+PiArc3RhdGljIGNvbnN0IHN0cnVjdCBvZl9kZXZpY2VfaWQgdW5pcGhpZXJfc3Bp X21hdGNoW10gPSB7Cj4gPj4+ICsgICAgICAgeyAuY29tcGF0aWJsZSA9ICJzb2Npb25leHQsdW5p cGhpZXItc2Nzc2kiLCB9LAo+ID4+Cj4gPj4+ICsgICAgICAgeyAvKiBzZW50aW5lbCAqLyB9LAo+ ID4+Cj4gPj4gU2xpZ2h0bHkgYmV0dGVyIHdpdGhvdXQgY29tbWEuCj4gPgo+ID4gT0suIEkgd2ls bCBtb2RpZnkgdGhpcy4KPiA+Cj4gPiAtLS0tLS0tLS0tLS0tLS0tLQo+ID4gQmVzdCBSZWdhcmRz LAo+ID4gS2VpamkgSGF5YXNoaWJhcmEKPiA+Cj4gPgo+ID4KPiA+Pj4gK307Cj4gPj4+ICtNT0RV TEVfREVWSUNFX1RBQkxFKG9mLCB1bmlwaGllcl9zcGlfbWF0Y2gpOwo+ID4+Cj4gPj4gLS0KPiA+ PiBXaXRoIEJlc3QgUmVnYXJkcywKPiA+PiBBbmR5IFNoZXZjaGVua28KPiA+CgoKLS0tLS0tLS0t LS0tLS0tLS0KQmVzdCBSZWdhcmRzLApLZWlqaSBIYXlhc2hpYmFyYQoKCgpfX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxp bmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3Rz LmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: hayashibara.keiji@socionext.com (Keiji Hayashibara) Date: Mon, 30 Jul 2018 10:15:06 +0900 Subject: [PATCH v2 2/2] spi: add SPI controller driver for UniPhier SoC In-Reply-To: References: <1532588943-19481-1-git-send-email-hayashibara.keiji@socionext.com> <1532588943-19481-3-git-send-email-hayashibara.keiji@socionext.com> <001b01d424c4$66e04230$34a0c690$@socionext.com> Message-ID: <002001d427a2$c05ca7f0$4115f7d0$@socionext.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Radu, > From: Radu Pirea [mailto:radu.pirea at microchip.com] > Sent: Thursday, July 26, 2018 7:58 PM > To: Hayashibara, Keiji/?? ?? ; 'Andy Shevchenko' > > 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 at gmail.com] > >> Sent: Thursday, July 26, 2018 5:46 PM > >> To: Hayashibara, Keiji/?? ?? > >> 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 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 > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >> > >> 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