[vRFC,3/8] treewide: rename firmware_request_platform()
Commit Message
Rename firmware_request_platform() to request_firmware_platform()
to be more concrete and align with the name of other request
firmware family functions.
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Documentation/driver-api/firmware/fallback-mechanisms.rst | 4 ++--
Documentation/driver-api/firmware/lookup-order.rst | 2 +-
Documentation/driver-api/firmware/request_firmware.rst | 4 ++--
drivers/base/firmware_loader/main.c | 6 +++---
drivers/input/touchscreen/chipone_icn8505.c | 2 +-
drivers/input/touchscreen/silead.c | 2 +-
include/linux/firmware.h | 4 ++--
lib/test_firmware.c | 2 +-
8 files changed, 13 insertions(+), 13 deletions(-)
Comments
On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> Rename firmware_request_platform() to request_firmware_platform()
> to be more concrete and align with the name of other request
> firmware family functions.
Sorry, but no, it should be "noun_verb" for public functions.
Yes, we mess this up a lot, but keeping the namespace this way works out
better for global symbols, so "firmware_*" is best please.
thanks,
greg k-h
On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > Rename firmware_request_platform() to request_firmware_platform()
> > to be more concrete and align with the name of other request
> > firmware family functions.
>
> Sorry, but no, it should be "noun_verb" for public functions.
News to me, do we have this documented somewhere?
> Yes, we mess this up a lot, but keeping the namespace this way works out
> better for global symbols, so "firmware_*" is best please.
We should certainly stick to *one* pattern, for the better, and it
occurs to me we could further review this with a coccinelle python
script for public functions, checking the first two elements of a public
function for noun and verb.
Luis
On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
> On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> > On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > > Rename firmware_request_platform() to request_firmware_platform()
> > > to be more concrete and align with the name of other request
> > > firmware family functions.
> >
> > Sorry, but no, it should be "noun_verb" for public functions.
>
> News to me, do we have this documented somewhere?
Not really, but searching makes it nicer.
And yes, I violated this in the past in places, and have regretted it...
> > Yes, we mess this up a lot, but keeping the namespace this way works out
> > better for global symbols, so "firmware_*" is best please.
>
> We should certainly stick to *one* pattern, for the better, and it
> occurs to me we could further review this with a coccinelle python
> script for public functions, checking the first two elements of a public
> function for noun and verb.
Changing the existing function names for no real reason isn't probably a
good idea, nor worth it. The firmware_* function prefix is good, let's
keep it please.
If you really wanted to be picky, we should make them part of a module
namespace too :)
thanks,
greg k-h
On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
> On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
> > On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> > > On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > > > Rename firmware_request_platform() to request_firmware_platform()
> > > > to be more concrete and align with the name of other request
> > > > firmware family functions.
> > >
> > > Sorry, but no, it should be "noun_verb" for public functions.
> >
> > News to me, do we have this documented somewhere?
>
> Not really, but searching makes it nicer.
>
> And yes, I violated this in the past in places, and have regretted it...
Care to share a few examples of regret?
> > > Yes, we mess this up a lot, but keeping the namespace this way works out
> > > better for global symbols, so "firmware_*" is best please.
> >
> > We should certainly stick to *one* pattern, for the better, and it
> > occurs to me we could further review this with a coccinelle python
> > script for public functions, checking the first two elements of a public
> > function for noun and verb.
>
> Changing the existing function names for no real reason isn't probably a
> good idea, nor worth it. The firmware_* function prefix is good, let's
> keep it please.
>
> If you really wanted to be picky, we should make them part of a module
> namespace too :)
Sticking to *any* convention is good, so longa as we decide on one, it
used to be hard to have tidy rules to do this sort of stuff but with
coccinelle we should be able to grow these rules and ensure we stick
to it for the good for new stuff.
Luis
On Fri, Feb 23, 2024 at 11:42:35AM -0800, Luis Chamberlain wrote:
> On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
> > On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
> > > On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> > > > On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > > > > Rename firmware_request_platform() to request_firmware_platform()
> > > > > to be more concrete and align with the name of other request
> > > > > firmware family functions.
> > > >
> > > > Sorry, but no, it should be "noun_verb" for public functions.
> > >
> > > News to me, do we have this documented somewhere?
> >
> > Not really, but searching makes it nicer.
> >
> > And yes, I violated this in the past in places, and have regretted it...
>
> Care to share a few examples of regret?
get_device()
put_device()
kill_device()
vs. a saner:
kobject_get()
kobject_put()
kobject_del()
Learn from the mistakes of my youth please :)
thanks,
greg k-h
On 2/24/2024 11:06 AM, Greg KH wrote:
> On Fri, Feb 23, 2024 at 11:42:35AM -0800, Luis Chamberlain wrote:
>> On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
>>> On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
>>>> On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
>>>>> On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
>>>>>> Rename firmware_request_platform() to request_firmware_platform()
>>>>>> to be more concrete and align with the name of other request
>>>>>> firmware family functions.
>>>>>
>>>>> Sorry, but no, it should be "noun_verb" for public functions.
>>>>
>>>> News to me, do we have this documented somewhere?
>>>
>>> Not really, but searching makes it nicer.
>>>
>>> And yes, I violated this in the past in places, and have regretted it...
>>
>> Care to share a few examples of regret?
>
> get_device()
> put_device()
> kill_device()
>
> vs. a saner:
> kobject_get()
> kobject_put()
> kobject_del()
>
> Learn from the mistakes of my youth please :)
Thanks for the history.,
In that case, should we fix this verb_noun cases ?
request_firmware()
request_firmware_into_buf()
request_firmware_nowarn()
request_firmware_direct()
request_firmware_cache()
request_partial_firmware_into_buf()
release_firmware()
-Mukesh
>
> thanks,
>
> greg k-h
On Mon, Feb 26, 2024 at 04:22:09PM +0530, Mukesh Ojha wrote:
>
>
> On 2/24/2024 11:06 AM, Greg KH wrote:
> > On Fri, Feb 23, 2024 at 11:42:35AM -0800, Luis Chamberlain wrote:
> > > On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
> > > > On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
> > > > > On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> > > > > > On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > > > > > > Rename firmware_request_platform() to request_firmware_platform()
> > > > > > > to be more concrete and align with the name of other request
> > > > > > > firmware family functions.
> > > > > >
> > > > > > Sorry, but no, it should be "noun_verb" for public functions.
> > > > >
> > > > > News to me, do we have this documented somewhere?
> > > >
> > > > Not really, but searching makes it nicer.
> > > >
> > > > And yes, I violated this in the past in places, and have regretted it...
> > >
> > > Care to share a few examples of regret?
> >
> > get_device()
> > put_device()
> > kill_device()
> >
> > vs. a saner:
> > kobject_get()
> > kobject_put()
> > kobject_del()
> >
> > Learn from the mistakes of my youth please :)
>
> Thanks for the history.,
> In that case, should we fix this verb_noun cases ?
>
> request_firmware()
> request_firmware_into_buf()
> request_firmware_nowarn()
> request_firmware_direct()
> request_firmware_cache()
> request_partial_firmware_into_buf()
> release_firmware()
That would provide consistency, right?
thanks,
greg k-h
On 2/26/2024 6:39 PM, Greg KH wrote:
> On Mon, Feb 26, 2024 at 04:22:09PM +0530, Mukesh Ojha wrote:
>>
>>
>> On 2/24/2024 11:06 AM, Greg KH wrote:
>>> On Fri, Feb 23, 2024 at 11:42:35AM -0800, Luis Chamberlain wrote:
>>>> On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
>>>>> On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
>>>>>> On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
>>>>>>> On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
>>>>>>>> Rename firmware_request_platform() to request_firmware_platform()
>>>>>>>> to be more concrete and align with the name of other request
>>>>>>>> firmware family functions.
>>>>>>>
>>>>>>> Sorry, but no, it should be "noun_verb" for public functions.
>>>>>>
>>>>>> News to me, do we have this documented somewhere?
>>>>>
>>>>> Not really, but searching makes it nicer.
>>>>>
>>>>> And yes, I violated this in the past in places, and have regretted it...
>>>>
>>>> Care to share a few examples of regret?
>>>
>>> get_device()
>>> put_device()
>>> kill_device()
>>>
>>> vs. a saner:
>>> kobject_get()
>>> kobject_put()
>>> kobject_del()
>>>
>>> Learn from the mistakes of my youth please :)
>>
>> Thanks for the history.,
>> In that case, should we fix this verb_noun cases ?
>>
>> request_firmware()
>> request_firmware_into_buf()
>> request_firmware_nowarn()
>> request_firmware_direct()
>> request_firmware_cache()
>> request_partial_firmware_into_buf()
>> release_firmware()
>
> That would provide consistency, right?
Yes, Below names look better..
firmware_request()
firmware_request_into_buf()
firmware_request_nowarn()
firmware_request_direct()
firmware_request_cache()
firmware_request_partial_into_buf()
firmware_release()
@Luis/Others, Can we do this change ?
-Mukesh
>
> thanks,
>
> greg k-h
On Mon, Feb 26, 2024 at 06:52:49PM +0530, Mukesh Ojha wrote:
>
>
> On 2/26/2024 6:39 PM, Greg KH wrote:
> > On Mon, Feb 26, 2024 at 04:22:09PM +0530, Mukesh Ojha wrote:
> > >
> > >
> > > On 2/24/2024 11:06 AM, Greg KH wrote:
> > > > On Fri, Feb 23, 2024 at 11:42:35AM -0800, Luis Chamberlain wrote:
> > > > > On Fri, Feb 23, 2024 at 04:33:40PM +0100, Greg KH wrote:
> > > > > > On Fri, Feb 23, 2024 at 07:15:45AM -0800, Luis Chamberlain wrote:
> > > > > > > On Fri, Feb 23, 2024 at 07:21:31AM +0100, Greg KH wrote:
> > > > > > > > On Thu, Feb 22, 2024 at 11:30:28PM +0530, Mukesh Ojha wrote:
> > > > > > > > > Rename firmware_request_platform() to request_firmware_platform()
> > > > > > > > > to be more concrete and align with the name of other request
> > > > > > > > > firmware family functions.
> > > > > > > >
> > > > > > > > Sorry, but no, it should be "noun_verb" for public functions.
> > > > > > >
> > > > > > > News to me, do we have this documented somewhere?
> > > > > >
> > > > > > Not really, but searching makes it nicer.
> > > > > >
> > > > > > And yes, I violated this in the past in places, and have regretted it...
> > > > >
> > > > > Care to share a few examples of regret?
> > > >
> > > > get_device()
> > > > put_device()
> > > > kill_device()
> > > >
> > > > vs. a saner:
> > > > kobject_get()
> > > > kobject_put()
> > > > kobject_del()
> > > >
> > > > Learn from the mistakes of my youth please :)
> > >
> > > Thanks for the history.,
> > > In that case, should we fix this verb_noun cases ?
> > >
> > > request_firmware()
> > > request_firmware_into_buf()
> > > request_firmware_nowarn()
> > > request_firmware_direct()
> > > request_firmware_cache()
> > > request_partial_firmware_into_buf()
> > > release_firmware()
> >
> > That would provide consistency, right?
>
> Yes, Below names look better..
>
> firmware_request()
> firmware_request_into_buf()
> firmware_request_nowarn()
> firmware_request_direct()
> firmware_request_cache()
> firmware_request_partial_into_buf()
> firmware_release()
>
> @Luis/Others, Can we do this change ?
Go for it. I just also think we might as well document from the learnt
lessons, and our preference, instead of making this just one developer's
personal preference because the moon made them feel a different way than
two years ago. From my part it is best we *strive* to stick to one
convention, whatever it is. As for the *why* to document this, I suspect
it allows easier namespace grep'ing for symbols related to one thing or
another, as to why it shoudl go first, I'll let Greg chime in.
Long term I see value in having anything we decide to stick to, to make it
easier for debugging heuristics.
Luis
@@ -212,7 +212,7 @@ of firmware for some of the system's integrated peripheral devices and
the peripheral's Linux device-driver needs to access this firmware.
Device drivers which need such firmware can use the
-firmware_request_platform() function for this, note that this is a
+request_firmware_platform() function for this, note that this is a
separate fallback mechanism from the other fallback mechanisms and
this does not use the sysfs interface.
@@ -245,7 +245,7 @@ To register this array with the efi-embedded-fw code, a driver needs to:
4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
-The firmware_request_platform() function will always first try to load firmware
+The request_firmware_platform() function will always first try to load firmware
with the specified name directly from the disk, so the EFI embedded-fw can
always be overridden by placing a file under /lib/firmware.
@@ -13,7 +13,7 @@ a driver issues a firmware API call.
* The ''Direct filesystem lookup'' is performed next, if found we
return it immediately
* The ''Platform firmware fallback'' is performed next, but only when
- firmware_request_platform() is used, if found we return it immediately
+ request_firmware_platform() is used, if found we return it immediately
* If no firmware has been found and the fallback mechanism was enabled
the sysfs interface is created. After this either a kobject uevent
is issued or the custom firmware loading is relied upon for firmware
@@ -25,10 +25,10 @@ request_firmware_nowarn
.. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware_nowarn
-firmware_request_platform
+request_firmware_platform
-------------------------
.. kernel-doc:: drivers/base/firmware_loader/main.c
- :functions: firmware_request_platform
+ :functions: request_firmware_platform
request_firmware_direct
-----------------------
@@ -1016,7 +1016,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
EXPORT_SYMBOL_GPL(request_firmware_direct);
/**
- * firmware_request_platform() - request firmware with platform-fw fallback
+ * request_firmware_platform() - request firmware with platform-fw fallback
* @firmware: pointer to firmware image
* @name: name of firmware file
* @device: device for which firmware is being loaded
@@ -1025,13 +1025,13 @@ EXPORT_SYMBOL_GPL(request_firmware_direct);
* direct filesystem lookup fails, it will fallback to looking for a copy of the
* requested firmware embedded in the platform's main (e.g. UEFI) firmware.
**/
-int firmware_request_platform(const struct firmware **firmware,
+int request_firmware_platform(const struct firmware **firmware,
const char *name, struct device *device)
{
return _request_firmware(firmware, name, device, NULL, 0, 0,
FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
}
-EXPORT_SYMBOL_GPL(firmware_request_platform);
+EXPORT_SYMBOL_GPL(request_firmware_platform);
/**
* firmware_request_cache() - cache firmware for suspend so resume can use it
@@ -288,7 +288,7 @@ static int icn8505_upload_fw(struct icn8505_data *icn8505)
* we may need it at resume. Having loaded it once will make the
* firmware class code cache it at suspend/resume.
*/
- error = firmware_request_platform(&fw, icn8505->firmware_name, dev);
+ error = request_firmware_platform(&fw, icn8505->firmware_name, dev);
if (error) {
dev_err(dev, "Firmware request error %d\n", error);
return error;
@@ -428,7 +428,7 @@ static int silead_ts_load_fw(struct i2c_client *client)
*/
error = request_firmware_nowarn(&fw, data->fw_name, dev);
if (error) {
- error = firmware_request_platform(&fw, data->fw_name, dev);
+ error = request_firmware_platform(&fw, data->fw_name, dev);
if (error) {
dev_err(dev, "Firmware request error %d\n", error);
return error;
@@ -99,7 +99,7 @@ int request_firmware(const struct firmware **fw, const char *name,
struct device *device);
int request_firmware_nowarn(const struct firmware **fw, const char *name,
struct device *device);
-int firmware_request_platform(const struct firmware **fw, const char *name,
+int request_firmware_platform(const struct firmware **fw, const char *name,
struct device *device);
int request_firmware_nowait(
struct module *module, bool uevent,
@@ -129,7 +129,7 @@ static inline int request_firmware_nowarn(const struct firmware **fw,
return -EINVAL;
}
-static inline int firmware_request_platform(const struct firmware **fw,
+static inline int request_firmware_platform(const struct firmware **fw,
const char *name,
struct device *device)
{
@@ -730,7 +730,7 @@ static ssize_t trigger_request_platform_store(struct device *dev,
efi_embedded_fw_checked = true;
pr_info("loading '%s'\n", name);
- rc = firmware_request_platform(&firmware, name, dev);
+ rc = request_firmware_platform(&firmware, name, dev);
if (rc) {
pr_info("load of '%s' failed: %d\n", name, rc);
goto out;