Re: [PATCH v9 7/7] staging: fpga manager: add driver for socfpga fpga manager

[atull <atull@opensource.altera.com>]

On Fri, 17 Jul 2015, Moritz Fischer wrote:

Hi Moritz,

> Alan,
> 
> it looks pretty good so far. I have worked with Michal and developed a
> Zynq equivalent against your last
> patchset which can be found in the Xilinx tree now.
> 
> I just briefly glanced the changes below just two nits that caught my eye.
> I'll take a closer look while trying to update the zynq-fpga driver to
> work with your changes.
> 
...
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/pm.h>
> As you removed the suspend / resume part, do you still need this?
> > +#include <linux/string.h>

Yep, I can take out this include.

> > +
> > +/*
> > + * Prepare the FPGA to receive the configuration data.
> > + */
> > +static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
> > +                                          const char *buf, size_t count)
> Is there a reason buf and count need to be passed here?
> > +{
> > +       struct socfpga_fpga_priv *priv = mgr->priv;
> > +       int ret;

Its to allow .write_init to look at the image header if it needs to.
Not every FPGA manager is going to need buf and count.  This one doesn't
(cyclone5).  Your .write_init can ignore them if you don't need them.

But Arria10 does (that's a separate driver that I didn't include in this
patchset). In that case I need to parse the image header to know whether
the bitstream is compressed, etc. to know how to configure the FPGA
manager registers before the FPGA can receive image data.

Thanks for reviewing!

Alan

> >
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 
> Overall good job, and thanks for pushing this!
> 
> Cheers,
> 
> Moritz
>