Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752131AbcKGVlc (ORCPT ); Mon, 7 Nov 2016 16:41:32 -0500 Received: from mail-qt0-f178.google.com ([209.85.216.178]:35208 "EHLO mail-qt0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751244AbcKGVl3 (ORCPT ); Mon, 7 Nov 2016 16:41:29 -0500 MIME-Version: 1.0 In-Reply-To: References: <1478486962-26794-1-git-send-email-joel@airwebreathe.org.uk> <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> From: Moritz Fischer Date: Mon, 7 Nov 2016 13:41:28 -0800 Message-ID: Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs To: Joel Holdsworth Cc: Alan Tull , Geert Uytterhoeven , Rob Herring , Devicetree List , Linux Kernel Mailing List , linux-spi@vger.kernel.org, =?UTF-8?B?TWFyZWsgVmHFoXV0?= , clifford@clifford.at Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2544 Lines: 85 Hi Joel, On Mon, Nov 7, 2016 at 11:02 AM, Joel Holdsworth wrote: > 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. Nah, you're right. Don't listen to me :) > > >>> + /* 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? Fair enough. I don't mind either way all that much, just thought I'd bring it up. Thanks, Moritz