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.
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);
>>> };
>>
