From: Marco Pagani <marpagan@redhat.com>
To: Xu Yilun <yilun.xu@linux.intel.com>
Cc: Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
Xu Yilun <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>,
Jonathan Corbet <corbet@lwn.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Alan Tull <atull@opensource.altera.com>,
Russ Weight <russ.weight@linux.dev>,
linux-fpga@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] fpga: region: add owner module and take its refcount
Date: Fri, 19 Apr 2024 10:13:12 +0200 [thread overview]
Message-ID: <07eb8fa4-871f-4810-ba59-6a6807d40e6e@redhat.com> (raw)
In-Reply-To: <ZiId9CnGEjwMcXeX@yilunxu-OptiPlex-7050>
On 2024-04-19 09:32, Xu Yilun wrote:
> 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@intel.com>
Thanks, I'll quickly send v6 to fix a minor typo in the kernel-doc:
- @owner: owner module containing get_bridges function
+ @owner: module containing the get_bridges function
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);
>>>>> };
>>>>
>>
>
prev parent reply other threads:[~2024-04-19 8:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 14:48 [PATCH v5] fpga: region: add owner module and take its refcount Marco Pagani
2024-04-13 2:35 ` Xu Yilun
2024-04-15 12:19 ` Marco Pagani
2024-04-15 17:11 ` Marco Pagani
2024-04-19 7:32 ` Xu Yilun
2024-04-19 8:13 ` Marco Pagani [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=07eb8fa4-871f-4810-ba59-6a6807d40e6e@redhat.com \
--to=marpagan@redhat.com \
--cc=atull@opensource.altera.com \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@intel.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mdf@kernel.org \
--cc=russ.weight@linux.dev \
--cc=trix@redhat.com \
--cc=yilun.xu@intel.com \
--cc=yilun.xu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).