On Mon, Apr 15, 2024 at 07:11:03PM +0200, Marco Pagani wrote:
> On 2024-04-15 14:19, Marco Pagani wrote:
> >
> >
> > On 2024-04-13 04:35, Xu Yilun wrote:
> >>> /**
> >>> - * fpga_region_register_full - create and register an FPGA Region device
> >>> + * __fpga_region_register_full - create and register an FPGA Region device
> >>> * @parent: device parent
> >>> * @info: parameters for FPGA Region
> >>> + * @owner: owner module containing the get_bridges function
> >>
> >> This is too specific and easily get unaligned if we add another
> >> callback. How about "module containing the region ops"?
> >
> > I had some concerns about using the name "region ops" in the kernel-doc
> > comment since it was not supported by a struct definition nor referenced
> > in the documentation. However, since the name is now referred to in the
> > ops_owner pointer, making the change makes sense to me.
> >
>
> On second thought, I think it would be better to leave the @owner
> description to "module containing the get_bridges function" for the
> moment. Otherwise, it could confuse the user by blurring the connection
> between the @owner and @get_bridges parameters.
>
> * __fpga_region_register - create and register an FPGA Region device
> * [...]
> * @get_bridges: optional function to get bridges to a list
> * @owner: owner module containing get_bridges function
>
> We can always modify the @owner description later, together with all the
> necessary changes to add a new op, like grouping all ops in a structure
> and changing the registration function signature.
OK, it's good to me. I'll apply this patch to for-next.
Acked-by: Xu Yilun <yilun.xu@xxxxxxxxx>
>
> Thanks,
> Marco
>
> >
> >>
> >>> *
> >>> * Return: struct fpga_region or ERR_PTR()
> >>> */
> >>> struct fpga_region *
> >>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
> >>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
> >>> + struct module *owner)
> >>> {
> >>> struct fpga_region *region;
> >>> int id, ret = 0;
> >>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >>> region->compat_id = info->compat_id;
> >>> region->priv = info->priv;
> >>> region->get_bridges = info->get_bridges;
> >>> + region->ops_owner = owner;
> >>>
> >>> mutex_init(®ion->mutex);
> >>> INIT_LIST_HEAD(®ion->bridge_list);
> >>> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >>>
> >>> return ERR_PTR(ret);
> >>> }
> >>> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >>> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
> >>>
> >>> /**
> >>> - * fpga_region_register - create and register an FPGA Region device
> >>> + * __fpga_region_register - create and register an FPGA Region device
> >>> * @parent: device parent
> >>> * @mgr: manager that programs this region
> >>> * @get_bridges: optional function to get bridges to a list
> >>> + * @owner: owner module containing get_bridges function
> >>
> >> ditto
> >>
> >>> *
> >>> * This simple version of the register function should be sufficient for most users.
> >>> * The fpga_region_register_full() function is available for users that need to
> >>> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >>> * Return: struct fpga_region or ERR_PTR()
> >>> */
> >>> struct fpga_region *
> >>> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >>> - int (*get_bridges)(struct fpga_region *))
> >>> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >>> + int (*get_bridges)(struct fpga_region *), struct module *owner)
> >>> {
> >>> struct fpga_region_info info = { 0 };
> >>>
> >>> info.mgr = mgr;
> >>> info.get_bridges = get_bridges;
> >>>
> >>> - return fpga_region_register_full(parent, &info);
> >>> + return __fpga_region_register_full(parent, &info, owner);
> >>> }
> >>> -EXPORT_SYMBOL_GPL(fpga_region_register);
> >>> +EXPORT_SYMBOL_GPL(__fpga_region_register);
> >>>
> >>> /**
> >>> * fpga_region_unregister - unregister an FPGA region
> >>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> >>> index 9d4d32909340..5fbc05fe70a6 100644
> >>> --- a/include/linux/fpga/fpga-region.h
> >>> +++ b/include/linux/fpga/fpga-region.h
> >>> @@ -36,6 +36,7 @@ struct fpga_region_info {
> >>> * @mgr: FPGA manager
> >>> * @info: FPGA image info
> >>> * @compat_id: FPGA region id for compatibility check.
> >>> + * @ops_owner: module containing the get_bridges function
> >>
> >> ditto
> >>
> >> Thanks,
> >> Yilun
> >>
> >>> * @priv: private data
> >>> * @get_bridges: optional function to get bridges to a list
> >>> */
> >>> @@ -46,6 +47,7 @@ struct fpga_region {
> >>> struct fpga_manager *mgr;
> >>> struct fpga_image_info *info;
> >>> struct fpga_compat_id *compat_id;
> >>> + struct module *ops_owner;
> >>> void *priv;
> >>> int (*get_bridges)(struct fpga_region *region);
> >>> };
> >>
>
