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

[Moritz Fischer]

Hi Joel,

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