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

Groups > linux.kernel > #1516615

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

Show all headers | View raw

Hi Joel,

On Mon, Nov 7, 2016 at 11:02 AM, Joel Holdsworth
<joel@airwebreathe.org.uk> 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

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