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(-)
diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 5f04c3bcdf0c..f888fceb65ba 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -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.
diff --git a/Documentation/driver-api/firmware/lookup-order.rst b/Documentation/driver-api/firmware/lookup-order.rst
index 6064672a782e..c72bc8efb734 100644
--- a/Documentation/driver-api/firmware/lookup-order.rst
+++ b/Documentation/driver-api/firmware/lookup-order.rst
@@ -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
diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index 0201334bc308..3d6df9922c4b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -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
-----------------------
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index cb681f4069b8..9411ad7968cb 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -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
diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
index b56954830b33..be1d7530a968 100644
--- a/drivers/input/touchscreen/chipone_icn8505.c
+++ b/drivers/input/touchscreen/chipone_icn8505.c
@@ -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;
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 52477e450b02..e3fb27eeca40 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -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;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5523c9fdc92d..93e6a37352a8 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -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)
{
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 9cfdcd6d21db..4aeb25bea3b4 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -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;
--
2.43.0.254.ga26002b62827On 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
© 2016 - 2026 Red Hat, Inc.