From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933468AbcKGSVb (ORCPT ); Mon, 7 Nov 2016 13:21:31 -0500 Received: from mail-out.m-online.net ([212.18.0.9]:59653 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932794AbcKGSVP (ORCPT ); Mon, 7 Nov 2016 13:21:15 -0500 X-Auth-Info: 4oTCMVSreqUcNA4W1237JeujzDRw/c5hSSS645/nt9w= Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs To: Joel Holdsworth , atull@opensource.altera.com, moritz.fischer@ettus.com, geert@linux-m68k.org, robh@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, clifford@clifford.at References: <1478486962-26794-1-git-send-email-joel@airwebreathe.org.uk> <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> From: Marek Vasut Message-ID: <2255968d-b97c-2b9c-4e4d-7f3717a748e3@denx.de> Date: Mon, 7 Nov 2016 19:01:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/07/2016 03:49 AM, Joel Holdsworth wrote: > The Lattice iCE40 is a family of FPGAs with a minimalistic architecture > and very regular structure, designed for low-cost, high-volume consumer > and system applications. [...] > +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, > + const char *buf, size_t count) > +{ > + struct ice40_fpga_priv *priv = mgr->priv; > + struct spi_device *dev = priv->dev; > + struct spi_message message; > + struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1, > + .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}; Should be this way for the sake of readability, fix globally: struct spi_transfer assert_cs_then_reset_delay = { .cs_change = 1, .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY }; Also I believe this could be const. > + struct spi_transfer housekeeping_delay_then_release_cs = { > + .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY}; Don't we have some less hacky way of toggling the nCS ? Is this even nCS or is this some control pin of the FPGA ? Maybe it should be treated like a regular GPIO instead ? [...] > +static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags) > +{ > + struct ice40_fpga_priv *priv = mgr->priv; > + struct spi_device *dev = priv->dev; > + const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,}; The comma is not needed. > + > + /* Check CDONE is asserted */ > + if (!gpiod_get_value(priv->cdone)) { > + dev_err(&dev->dev, > + "CDONE was not asserted after firmware transfer\n"); > + return -EIO; > + } > + > + /* Send of zero-padding to activate the firmware */ > + return spi_write(dev, padding, sizeof(padding)); > +} [...] > + /* Check board setup data. */ > + if (spi->max_speed_hz > 25000000) { > + dev_err(dev, "Speed is too high\n"); > + return -EINVAL; > + } > + > + if (spi->max_speed_hz < 1000000) { > + dev_err(dev, "Speed is too low\n"); > + return -EINVAL; > + } Do you have some explanation for this limitation ? [...] -- Best regards, Marek Vasut