From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751495AbdFGIds (ORCPT ); Wed, 7 Jun 2017 04:33:48 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:40195 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751441AbdFGIdq (ORCPT ); Wed, 7 Jun 2017 04:33:46 -0400 Date: Wed, 7 Jun 2017 10:33:34 +0200 From: Sascha Hauer To: Jiada Wang Cc: broonie@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, kernel@pengutronix.de, fabio.estevam@nxp.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-spi@vger.kernel.org Subject: Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode Message-ID: <20170607083334.uk6pmgzdpgzfkn2i@pengutronix.de> References: <1496222398-3192-1-git-send-email-jiada_wang@mentor.com> <1496222398-3192-4-git-send-email-jiada_wang@mentor.com> <20170531121037.pgs5yysveiu54nwo@pengutronix.de> <59316463.9030209@mentor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <59316463.9030209@mentor.com> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 10:27:28 up 65 days, 1:09, 29 users, load average: 0.06, 0.02, 0.00 User-Agent: Mutt/1.6.2-neo (2016-06-11) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 02, 2017 at 06:13:07AM -0700, Jiada Wang wrote: > Hi Sascha > > On 05/31/2017 05:10 AM, Sascha Hauer wrote: > >> + > >> +static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx) > >> +{ > >> + u32 val = 0; > >> + int shift = spi_imx->count % sizeof(val); > >> + > >> + if (spi_imx->tx_buf) { > >> + if (shift) { > >> + memcpy(((u8 *)&val) + sizeof(val) - shift, > >> + spi_imx->tx_buf, shift); > >> + } else { > >> + val = *((u32 *)spi_imx->tx_buf); > >> + shift = sizeof(val); > >> + } > >> + val = cpu_to_be32(val); > >> + spi_imx->tx_buf += shift; > >> + } > >> + > >> + if (!shift) > >> + shift = sizeof(val); > > A better name for 'shift' is n_bytes. Its value should be calculated > > before the if (spi_imx->tx_buf) block. Then you can use memcpy for the > > four byte case aswell. > > > > > > But anyway, have you tested this function for anything with > > spi_imx->count % sizeof(val) != 0? In this case you have to put the fill > > the FIFO only partly somewhere. I would assume you have to do this at > > the end of the transfer, but you do this at the beginning. Can this > > work? > Yes, I have tested the case when "spi_imx->count % sizeof(val) != 0", it > works. For the record: Yes, this seems to be right. I misread the datasheet. When the controller is programmed to transfer 5 bytes, then it seemed fairly logical to me that it first expects four bytes in its 32bit FIFO and then the remaining single byte. In fact it's the other way round, how the example of the BURST_LENGTH register description says: 0x021 A SPI burst contains the 2 LSB in first word and all 32 bits in second word. So the part that doesn't fill a whole FIFO width goes out first, not last. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |