lkml.org 
[lkml]   [2016]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs
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

\
 
 \ /
  Last update: 2016-11-07 22:42    [W:0.047 / U:1.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site