On Mon, Nov 13, 2017 at 11:51:59AM -0600, Alan Tull wrote: > On Tue, Oct 31, 2017 at 9:22 PM, Alan Tull <at...@kernel.org> wrote: > > Any further comments on v5? I'm getting ready to send v6. If I do it > today, most of these patches will have no changes (again), the only > changes will be in the patches that move drvdata out of the common > code. > > I've gone to a lot of trouble to break out functional patches to make > them easy to review and to keep what I was trying to accomplish clear > here. I think this stuff is necessary going forward. If this stuff > doesn't have errors, let's move forward and make whatever changes we > want to make on top of this.
Sounds good, haven't found anything else. Sorry for dropping the ball. > > > On Tue, Oct 31, 2017 at 8:34 PM, Moritz Fischer <m...@kernel.org> wrote: > >> On Tue, Oct 31, 2017 at 04:45:54PM -0500, Alan Tull wrote: > >>> On Tue, Oct 31, 2017 at 3:59 PM, Moritz Fischer <m...@kernel.org> wrote: > >>> > On Tue, Oct 31, 2017 at 08:42:14PM +0000, Alan Tull wrote: > >>> >> Changes to the fpga manager code to not use drvdata in common > >>> >> code. > >>> >> > >>> >> Change fpga_mgr_register to not set or use drvdata. > >>> >> > >>> >> Change the register/unregister function parameters to take the mgr > >>> >> struct: > >>> >> * int fpga_mgr_register(struct device *dev, > >>> >> struct fpga_manager *mgr); > >>> >> * void fpga_mgr_unregister(struct fpga_manager *mgr); > >>> >> > >>> >> Change the drivers that call fpga_mgr_register to alloc the struct > >>> >> fpga_manager (using devm_kzalloc) and partly fill it, adding name, > >>> >> ops, and priv. > >>> >> > >>> >> The rationale is that setting drvdata is fine for DT based devices > >>> >> that will have one manager, bridge, or region per platform device. > >>> >> However PCIe based devices may have multiple FPGA mgr/bridge/regions > >>> >> under one pcie device. Without these changes, the PCIe solution has > >>> >> to create an extra device for each child mgr/bridge/region to hold > >>> >> drvdata. > >>> > > >>> > This looks very common, in fact several subsystems provide this two step > >>> > way of registering things. > >>> > > >>> > - Allocate fpga_mgr via fpga_mgr_alloc() where you pass in the size of > >>> > the private data > >>> > - Fill in some fields > >>> > - fpga_mgr_register() where you pass in the fpga_mgr as suggested > >>> > > >>> > See for example the alloc_etherdev() for ethernet devices. > >>> > > >>> > >>> Yes, I considered it when I was writing this. I've seen it both ways. > >>> In the case you mention, they've got reasons they absolutely needed to > >>> do it that way. alloc_etherdev() calls eventually to > >>> alloc_netdev_mqs() which is about 100 lines of alloc'ing and > >>> initializing a network device struct. > >> > >> Yeah, sure. I looked around some more. Other subsystems just alloc > >> manually, seems fine with me. > >>> > >>> > The benefit of the API you proposed is that one could embed the fpga_mgr > >>> > struct inside of another struct and get to the container via > >>> > container_of() I guess ... > >>> > >>> Yes, let's keep it simple for now, as that gives us greater > >>> flexibility. We can add alloc functions when it becomes clear that it > >>> won't get in the way of someone's use. > >> > >> Agreed. > >>> > >>> Alan > >>> > >>> > > >>> >> > >>> >> Signed-off-by: Alan Tull <at...@kernel.org> > >>> >> Reported-by: Jiuyue Ma <majiu...@huawei.com> > >>> >> --- > >>> >> Documentation/fpga/fpga-mgr.txt | 23 ++++++++++++++++------- > >>> >> drivers/fpga/altera-cvp.c | 17 +++++++++++++---- > >>> >> drivers/fpga/altera-pr-ip-core.c | 16 ++++++++++++++-- > >>> >> drivers/fpga/altera-ps-spi.c | 17 ++++++++++++++--- > >>> >> drivers/fpga/fpga-mgr.c | 28 +++++++--------------------- > >>> >> drivers/fpga/ice40-spi.c | 19 +++++++++++++++---- > >>> >> drivers/fpga/socfpga-a10.c | 15 ++++++++++++--- > >>> >> drivers/fpga/socfpga.c | 17 ++++++++++++++--- > >>> >> drivers/fpga/ts73xx-fpga.c | 17 ++++++++++++++--- > >>> >> drivers/fpga/xilinx-spi.c | 17 ++++++++++++++--- > >>> >> drivers/fpga/zynq-fpga.c | 15 ++++++++++++--- > >>> >> include/linux/fpga/fpga-mgr.h | 6 ++---- > >>> >> 12 files changed, 147 insertions(+), 60 deletions(-) > >>> >> > >>> >> diff --git a/Documentation/fpga/fpga-mgr.txt > >>> >> b/Documentation/fpga/fpga-mgr.txt > >>> >> index cc6413e..7e5e5c8 100644 > >>> >> --- a/Documentation/fpga/fpga-mgr.txt > >>> >> +++ b/Documentation/fpga/fpga-mgr.txt > >>> >> @@ -67,11 +67,9 @@ fpga_mgr_unlock when done programming the FPGA. > >>> >> To register or unregister the low level FPGA-specific driver: > >>> >> ------------------------------------------------------------- > >>> >> > >>> >> - int fpga_mgr_register(struct device *dev, const char *name, > >>> >> - const struct fpga_manager_ops *mops, > >>> >> - void *priv); > >>> >> + int fpga_mgr_register(struct device *dev, struct fpga_manager > >>> >> *mgr); > >> > >> At that point you could also just give the struct fpga_manager a > >> ->parent or ->dev that you populate with &pdev->dev or &spi->dev etc > >> instead of > >> making it a separate parameter, this makes an odd mix of half and half > >> here. > > > > Yes, I'd have to call it parent as dev is taken. I also noticed that > > I forgot to edit the function parameter documentation in the c file. > > > >>> >> > >>> >> - void fpga_mgr_unregister(struct device *dev); > >>> >> + void fpga_mgr_unregister(struct fpga_manager *mgr); > >>> >> > >>> >> Use of these two functions is described below in "How To Support a > >>> >> new FPGA > >>> >> device." > >>> >> @@ -148,8 +146,13 @@ static int socfpga_fpga_probe(struct > >>> >> platform_device *pdev) > >>> >> { > >>> >> struct device *dev = &pdev->dev; > >>> >> struct socfpga_fpga_priv *priv; > >>> >> + struct fpga_manager *mgr; > >>> >> int ret; > >>> >> > >>> >> + mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL); > >>> >> + if (!mgr) > >>> >> + return -ENOMEM; > >>> >> + > >>> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > >>> >> if (!priv) > >>> >> return -ENOMEM; > >>> >> @@ -157,13 +160,19 @@ static int socfpga_fpga_probe(struct > >>> >> platform_device *pdev) > >>> >> /* ... do ioremaps, get interrupts, etc. and save > >>> >> them in priv... */ > >>> >> > >>> >> - return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager", > >>> >> - &socfpga_fpga_ops, priv); > >>> >> + mgr->name = "Altera SOCFPGA FPGA Manager"; > >>> >> + mgr->mops = &socfpga_fpga_ops; > >>> >> + mgr->priv = priv; > >> Like here: mgr->dev = &pdev->dev > > > > Yes, good catch. By the way, I pushed these patches on a branch to > > linux-fpga (branch name is review-v4.14-rc7-non-dt-support-v5.1). > > > > Thanks for reviewing, > > Alan > > > >> > >>> >> + platform_set_drvdata(pdev, mgr); > >>> >> + > >>> >> + return fpga_mgr_register(dev, mgr); > >>> >> } Cheers, Moritz
signature.asc
Description: PGP signature