Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1516503

Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v8 1/3] of: Add vendor prefix for Lattice Semiconductor Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-07 04:00 +0100
  [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-07 04:00 +0100
    Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Moritz Fischer <moritz.fischer@ettus.com> - 2016-11-07 19:30 +0100
      Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-07 20:10 +0100
        Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Moritz Fischer <moritz.fischer@ettus.com> - 2016-11-07 22:50 +0100
    Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Marek Vasut <marex@denx.de> - 2016-11-07 19:30 +0100
      Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-07 20:00 +0100
        Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Marek Vasut <marex@denx.de> - 2016-11-08 12:50 +0100
          Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Moritz Fischer <moritz.fischer@ettus.com> - 2016-11-08 18:10 +0100
            Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-08 18:40 +0100
              Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Marek Vasut <marex@denx.de> - 2016-11-09 19:20 +0100
                Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-09 19:40 +0100
                Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Marek Vasut <marex@denx.de> - 2016-11-09 19:50 +0100
                Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-09 20:00 +0100
                Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Marek Vasut <marex@denx.de> - 2016-11-10 18:00 +0100
          Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Joel Holdsworth <joel@airwebreathe.org.uk> - 2016-11-08 18:20 +0100

csiph-web