From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933793AbcKGTCQ (ORCPT ); Mon, 7 Nov 2016 14:02:16 -0500 Received: from b.painless.aa.net.uk ([81.187.30.52]:53695 "EHLO b.painless.aa.net.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933675AbcKGTCL (ORCPT ); Mon, 7 Nov 2016 14:02:11 -0500 Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs To: Moritz Fischer References: <1478486962-26794-1-git-send-email-joel@airwebreathe.org.uk> <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> Cc: Alan Tull , Geert Uytterhoeven , Rob Herring , Devicetree List , Linux Kernel Mailing List , linux-spi@vger.kernel.org, =?UTF-8?Q?Marek_Va=c5=a1ut?= , clifford@clifford.at From: Joel Holdsworth Message-ID: Date: Mon, 7 Nov 2016 12:02:01 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Painless-Spam-Score: -0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Moritz - thanks for your comments. >> An example of such a device is the icoBoard; a RaspberryPI HAT which >> features an iCE40HX8K with a 1 or 8 MBit SRAM and ports for >> Digilent-compatible PMOD modules. A PMOD module may contain a device >> with which the kernel communicates, via the FPGA. > > Personally I find this a bit verbose, but that's just me. I could cut it down a bit if you want. I was just trying to make the case for why this *has* to be in the kernel rather than just being done from user-space. >> + struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1, >> + .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}; > > Formatting looks odd, can you move the .cs_change to the next line? Marek made the same comment. I'll make the change. >> +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,}; >> + >> + /* 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)); > > I'd move all of these into the write() callback. Is that correct? I was under the impression that write might be called multiple times to load firmware in multiple chunks. >> + /* Enter reset */ >> + gpiod_set_value(priv->reset, 1); > > I know Marek had suggested this, none of the other drivers behave like that. > I'm not sure this is expected behavior for most people. For me it's not a big deal either way, but I think it's quite good for the driver to reset the device. When you remove most other drivers, you expect the driver to shut the device down, so isn't this just the same? ...but then the FPGA manager is a unique best with its own conventions, so perhaps it should behave differently. How can we get a consensus about this? Joel