| Message ID | 20180329153658.11614-5-mdf@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Mar 29, 2018 at 08:36:56AM -0700, Moritz Fischer wrote: > From: Alan Tull <atull@kernel.org> > > Change fpga_region_register to only take one parameter: > > int fpga_region_register(struct fpga_region *region) > > The parent dev is added to struct fpga_region. > > This make it similar to fpga_bridge_register and fpga_mgr_register > which also just take their respective struct. > > The one caller of fpga_region_register is changed to alloc the > fpga_region struct, fill it in, and pass it to the register > function. > > Signed-off-by: Alan Tull <atull@kernel.org> > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > Documentation/fpga/fpga-region.txt | 3 +-- > drivers/fpga/fpga-region.c | 8 +++++++- > drivers/fpga/of-fpga-region.c | 3 ++- > include/linux/fpga/fpga-region.h | 4 +++- > 4 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt > index 139a02ba1ff6..d38fa3b4154a 100644 > --- a/Documentation/fpga/fpga-region.txt > +++ b/Documentation/fpga/fpga-region.txt > @@ -42,8 +42,7 @@ The FPGA region API > To register or unregister a region: > ----------------------------------- > > - int fpga_region_register(struct device *dev, > - struct fpga_region *region); > + int fpga_region_register(struct fpga_region *region); > int fpga_region_unregister(struct fpga_region *region); > > An example of usage can be seen in the probe function of [3] > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index ebe1f872810d..660a91b9e246 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -162,10 +162,16 @@ int fpga_region_program_fpga(struct fpga_region *region) > } > EXPORT_SYMBOL_GPL(fpga_region_program_fpga); > > -int fpga_region_register(struct device *dev, struct fpga_region *region) > +int fpga_region_register(struct fpga_region *region) > { > + struct device *dev = region->parent; > int id, ret = 0; > > + if (!dev) { > + pr_err("Attempt to register fpga region without parent\n"); > + return -EINVAL; > + } Are you sure you don't want a virtual device? That is what will happen if you do not have a parent, right? Or do you always want to have "real" devices? > + > id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL); > if (id < 0) > return id; > diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c > index 35e7e8c4a0cb..a7b38aafeaa7 100644 > --- a/drivers/fpga/of-fpga-region.c > +++ b/drivers/fpga/of-fpga-region.c > @@ -428,12 +428,13 @@ static int of_fpga_region_probe(struct platform_device *pdev) > goto eprobe_mgr_put; > } > > + region->parent = dev; > region->mgr = mgr; > > /* Specify how to get bridges for this type of region. */ > region->get_bridges = of_fpga_region_get_bridges; > > - ret = fpga_region_register(dev, region); > + ret = fpga_region_register(region); > if (ret) > goto eprobe_mgr_put; > > diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h > index b6520318ab9c..423c87e3e29a 100644 > --- a/include/linux/fpga/fpga-region.h > +++ b/include/linux/fpga/fpga-region.h > @@ -8,6 +8,7 @@ > /** > * struct fpga_region - FPGA Region structure > * @dev: FPGA Region device > + * @parent: parent device > * @mutex: enforces exclusive reference to region > * @bridge_list: list of FPGA bridges specified in region > * @mgr: FPGA manager > @@ -18,6 +19,7 @@ > */ > struct fpga_region { > struct device dev; > + struct device *parent; Why doesn't your dev parent pointer point to this, why do you need to have a separate pointer? That feels really wrong. Pass in the parent pointer when you create the struct device, otherwise it will be registered incorrectly anyway. Then you always have the correct pointer, no need to keep a "spare" copy. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote: Hi Greg, >> -int fpga_region_register(struct device *dev, struct fpga_region *region) >> +int fpga_region_register(struct fpga_region *region) >> { >> + struct device *dev = region->parent; >> int id, ret = 0; >> >> + if (!dev) { >> + pr_err("Attempt to register fpga region without parent\n"); >> + return -EINVAL; >> + } > > Are you sure you don't want a virtual device? That is what will happen > if you do not have a parent, right? Or do you always want to have > "real" devices? I don't want to restrict this to "real" devices, so yes, I'll be removing this check. >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h >> index b6520318ab9c..423c87e3e29a 100644 >> --- a/include/linux/fpga/fpga-region.h >> +++ b/include/linux/fpga/fpga-region.h >> @@ -8,6 +8,7 @@ >> /** >> * struct fpga_region - FPGA Region structure >> * @dev: FPGA Region device >> + * @parent: parent device >> * @mutex: enforces exclusive reference to region >> * @bridge_list: list of FPGA bridges specified in region >> * @mgr: FPGA manager >> @@ -18,6 +19,7 @@ >> */ >> struct fpga_region { >> struct device dev; >> + struct device *parent; > > Why doesn't your dev parent pointer point to this, why do you need to > have a separate pointer? That feels really wrong. Pass in the parent > pointer when you create the struct device, otherwise it will be > registered incorrectly anyway. Then you always have the correct > pointer, no need to keep a "spare" copy. I'll add a fpga_mgr_create function and let it set the parent. No need to save it. Thanks again for the review, Alan > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 03:42:51PM -0500, Alan Tull wrote: > On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > Hi Greg, > > >> -int fpga_region_register(struct device *dev, struct fpga_region *region) > >> +int fpga_region_register(struct fpga_region *region) > >> { > >> + struct device *dev = region->parent; > >> int id, ret = 0; > >> > >> + if (!dev) { > >> + pr_err("Attempt to register fpga region without parent\n"); > >> + return -EINVAL; > >> + } > > > > Are you sure you don't want a virtual device? That is what will happen > > if you do not have a parent, right? Or do you always want to have > > "real" devices? > > I don't want to restrict this to "real" devices, so yes, I'll be > removing this check. > > >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h > >> index b6520318ab9c..423c87e3e29a 100644 > >> --- a/include/linux/fpga/fpga-region.h > >> +++ b/include/linux/fpga/fpga-region.h > >> @@ -8,6 +8,7 @@ > >> /** > >> * struct fpga_region - FPGA Region structure > >> * @dev: FPGA Region device > >> + * @parent: parent device > >> * @mutex: enforces exclusive reference to region > >> * @bridge_list: list of FPGA bridges specified in region > >> * @mgr: FPGA manager > >> @@ -18,6 +19,7 @@ > >> */ > >> struct fpga_region { > >> struct device dev; > >> + struct device *parent; > > > > Why doesn't your dev parent pointer point to this, why do you need to > > have a separate pointer? That feels really wrong. Pass in the parent > > pointer when you create the struct device, otherwise it will be > > registered incorrectly anyway. Then you always have the correct > > pointer, no need to keep a "spare" copy. > > I'll add a fpga_mgr_create function and let it set the parent. No > need to save it. I think we had discussed this in the first round of the patchset. How about fpga_mgr_alloc(...) and fpga_mgr_register(...) as suggested back then? Thanks for the review, Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 29, 2018 at 4:39 PM, Moritz Fischer <mdf@kernel.org> wrote: > On Thu, Mar 29, 2018 at 03:42:51PM -0500, Alan Tull wrote: >> On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> Hi Greg, >> >> >> -int fpga_region_register(struct device *dev, struct fpga_region *region) >> >> +int fpga_region_register(struct fpga_region *region) >> >> { >> >> + struct device *dev = region->parent; >> >> int id, ret = 0; >> >> >> >> + if (!dev) { >> >> + pr_err("Attempt to register fpga region without parent\n"); >> >> + return -EINVAL; >> >> + } >> > >> > Are you sure you don't want a virtual device? That is what will happen >> > if you do not have a parent, right? Or do you always want to have >> > "real" devices? >> >> I don't want to restrict this to "real" devices, so yes, I'll be >> removing this check. >> >> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h >> >> index b6520318ab9c..423c87e3e29a 100644 >> >> --- a/include/linux/fpga/fpga-region.h >> >> +++ b/include/linux/fpga/fpga-region.h >> >> @@ -8,6 +8,7 @@ >> >> /** >> >> * struct fpga_region - FPGA Region structure >> >> * @dev: FPGA Region device >> >> + * @parent: parent device >> >> * @mutex: enforces exclusive reference to region >> >> * @bridge_list: list of FPGA bridges specified in region >> >> * @mgr: FPGA manager >> >> @@ -18,6 +19,7 @@ >> >> */ >> >> struct fpga_region { >> >> struct device dev; >> >> + struct device *parent; >> > >> > Why doesn't your dev parent pointer point to this, why do you need to >> > have a separate pointer? That feels really wrong. Pass in the parent >> > pointer when you create the struct device, otherwise it will be >> > registered incorrectly anyway. Then you always have the correct >> > pointer, no need to keep a "spare" copy. >> >> I'll add a fpga_mgr_create function and let it set the parent. No >> need to save it. > > I think we had discussed this in the first round of the patchset. Yup! :) > How about fpga_mgr_alloc(...) and fpga_mgr_register(...) as suggested > back then? I'm cool with either name. The alloc/create function will be doing more than alloc. It's going to fill in some struct members, allocate an id #, and init the dev. There's precedent for similar functions named either way, even 'alloc' functions that do plenty of initialization. Alan > > Thanks for the review, > > Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt index 139a02ba1ff6..d38fa3b4154a 100644 --- a/Documentation/fpga/fpga-region.txt +++ b/Documentation/fpga/fpga-region.txt @@ -42,8 +42,7 @@ The FPGA region API To register or unregister a region: ----------------------------------- - int fpga_region_register(struct device *dev, - struct fpga_region *region); + int fpga_region_register(struct fpga_region *region); int fpga_region_unregister(struct fpga_region *region); An example of usage can be seen in the probe function of [3] diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index ebe1f872810d..660a91b9e246 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -162,10 +162,16 @@ int fpga_region_program_fpga(struct fpga_region *region) } EXPORT_SYMBOL_GPL(fpga_region_program_fpga); -int fpga_region_register(struct device *dev, struct fpga_region *region) +int fpga_region_register(struct fpga_region *region) { + struct device *dev = region->parent; int id, ret = 0; + if (!dev) { + pr_err("Attempt to register fpga region without parent\n"); + return -EINVAL; + } + id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL); if (id < 0) return id; diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c index 35e7e8c4a0cb..a7b38aafeaa7 100644 --- a/drivers/fpga/of-fpga-region.c +++ b/drivers/fpga/of-fpga-region.c @@ -428,12 +428,13 @@ static int of_fpga_region_probe(struct platform_device *pdev) goto eprobe_mgr_put; } + region->parent = dev; region->mgr = mgr; /* Specify how to get bridges for this type of region. */ region->get_bridges = of_fpga_region_get_bridges; - ret = fpga_region_register(dev, region); + ret = fpga_region_register(region); if (ret) goto eprobe_mgr_put; diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index b6520318ab9c..423c87e3e29a 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -8,6 +8,7 @@ /** * struct fpga_region - FPGA Region structure * @dev: FPGA Region device + * @parent: parent device * @mutex: enforces exclusive reference to region * @bridge_list: list of FPGA bridges specified in region * @mgr: FPGA manager @@ -18,6 +19,7 @@ */ struct fpga_region { struct device dev; + struct device *parent; struct mutex mutex; /* for exclusive reference to region */ struct list_head bridge_list; struct fpga_manager *mgr; @@ -34,7 +36,7 @@ struct fpga_region *fpga_region_class_find( int (*match)(struct device *, const void *)); int fpga_region_program_fpga(struct fpga_region *region); -int fpga_region_register(struct device *dev, struct fpga_region *region); +int fpga_region_register(struct fpga_region *region); int fpga_region_unregister(struct fpga_region *region); #endif /* _FPGA_REGION_H */