On Wed, 22 Jul 2015, Moritz Fischer wrote: Hi Miritz, > Hi Alan, > > a couple of small things I found while reworking the Zynq version to > match the v9 patchset: > > On Fri, Jul 17, 2015 at 8:51 AM, <atull@xxxxxxxxxxxxxxxxxxxxx> wrote: > > From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> > > ... > > + ret = mgr->mops->write_complete(mgr); > > Could we pass in flags here? This way the driver wouldn't have to keep > track of the flags. For my case it would simplify the partial reconfig > case. Yes, that change will be very simple and makes sense to me. > > +/* > > + * FPGA Manager flags > > + * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported > > + */ > > +#define FPGA_MGR_PARTIAL_RECONFIG (1) > Could this be BIT(0) instead? Yes, ha. That's what it should have been. > > + > > +/** > > + * struct fpga_manager_ops - ops for low level fpga manager drivers > > + * @state: returns an enum value of the FPGA's state > > + * @write_init: prepare the FPGA to receive confuration data > > + * @write: write count bytes of configuration data to the FPGA > > + * @write_complete: set FPGA to operating state after writing is done > > + * @fpga_remove: optional: Set FPGA into a specific state during driver remove > > + * > > + * fpga_manager_ops are the low level functions implemented by a specific > > + * fpga manager driver. The optional ones are tested for NULL before being > > + * called, so leaving them out is fine. > > + */ > > +struct fpga_manager_ops { > > + enum fpga_mgr_states (*state)(struct fpga_manager *mgr); > > + int (*write_init)(struct fpga_manager *mgr, u32 flags, > > + const char *buf, size_t count); > > + int (*write)(struct fpga_manager *mgr, const char *buf, size_t count); > > + int (*write_complete)(struct fpga_manager *mgr); > See comment above, having the flags here would be very convenient for > my usecase. Yes > > + void (*fpga_remove)(struct fpga_manager *mgr); > > +}; > > + ... > > Overall looks pretty good. I still need to look at the bridge part, > currently I have the resets and level shifters in the zynq-fpga > driver, > but maybe breaking them out makes sense. > > Cheers, > > Moritz > Thanks for the review. I'll probably send out a v10 next week including your recommendations. Alan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html