Hi Andy,
On 10/11/2020 05:26, Andy Shevchenko wrote:
On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@xxxxxxxxxx> wrote:
This is the initial implementation of the platform-profile feature.
It provides the details discussed and outlined in the
sysfs-platform_profile document.
Many modern systems have the ability to modify the operating profile to
control aspects like fan speed, temperature and power levels. This
module provides a common sysfs interface that platform modules can register
against to control their individual profile options.
...
+config ACPI_PLATFORM_PROFILE
+ tristate "ACPI Platform Profile Driver"
+ default y
+ help
+ This driver adds support for platform-profiles on platforms that
+ support it.
+
+ Platform-profiles can be used to control the platform behaviour. For
+ example whether to operate in a lower power mode, in a higher
+ power performance mode or between the two.
+
+ This driver provides the sysfs interface and is used as the registration
+ point for platform specific drivers.
+
+ Which profiles are supported is determined on a per-platform basis and
+ should be obtained from the platform specific driver.
+
+
None of the blank lines is enough. But can you consider to find
perhaps better place (I imply some logical group of options in the
file).
Will do.
...
obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
obj-$(CONFIG_ACPI_PPTT) += pptt.o
+obj-$(CONFIG_ACPI_PLATFORM_PROFILE) += platform_profile.o
...yes, and this becomes consistent with the above.
...
+/*
+ * platform_profile.c - Platform profile sysfs interface
+ */
One line. PLease, don't put the file name into the file. If we want to
rename it, it will give additional churn and as shown in practice
people often forget this change to follow.
Ack.
...
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/string.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/mutex.h>
+#include <acpi/acpi_bus.h>
+#include <linux/platform_profile.h>
Perhaps sorted?
Why do you need a specific acpi_bus.h? I thought acpi.h includes it already, no?
Sure - I hadn't realised sorted was a requirement :)
I'll check on acpi_bus.h - I think I initially added acpi_bus.h thinking
it was all I needed and then added acpi.h later. I'll clean up if I can.
...
+struct platform_profile *cur_profile;
Better naming since it's a global variable.
Is it supposed to be exported to modules?
It's internal only, but agreed on improving the naming.
(Barabas pointed out that it should be a static too)
...
+DEFINE_MUTEX(profile_lock);
No static?
Yes, it should be.
...
+/* Ensure the first char of each profile is unique */
+static char *profile_str[] = {
static const char * const profile_names[]
Also naming (perhaps like I proposed above?).
Ack.
+ "Low-power",
+ "Cool",
+ "Quiet",
+ "Balance",
+ "Performance",
+ "Unknown"
Leave the comma here.
Ack.
+};
...
+ int i;
+ int ret, count = 0;
count AFAICS should be size_t (or ssize_t).
Can you make them in reversed xmas tree order?
Sure - will fix.
...
+ return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);
Nowadays we have sysfs_emit(), use it.
OK - that was a new one to me. Will update.
...
+ /* Scan for a matching profile */
+ for (profile = profile_low; profile < profile_unknown; profile++) {
+ if (toupper(buf[0]) == profile_str[profile][0])
+ break;
+ }
match_string() / sysfs_match_string() ?
Yes, Barnabas picked up on this too. I'll correct.
...
+static struct attribute *platform_profile_attributes[] = {
+ &dev_attr_platform_profile_choices.attr,
+ &dev_attr_platform_profile.attr,
+ NULL,
Drop comma in terminator line.
Ack.
+};
...
+module_init(platform_profile_init);
+module_exit(platform_profile_exit);
Attach them to respective functions.
OK.
...
+/*
+ * platform_profile.h - platform profile sysfs interface
No file name.
Ack.
+ *
+ * See Documentation/ABI/testing/sysfs-platform_profile for more information.
+ */
...
+/*
+ * If more options are added please update profile_str
+ * array in platform-profile.c
+ */
Kernel doc?
OK.
Just in case - I assume this should go int the sysfs-platform-profile
doc (patch 1 of the series). Let me know if that's not the expectation.
+enum profile_option {
+ profile_low,
+ profile_cool,
+ profile_quiet,
+ profile_balance,
+ profile_perform,
+ profile_unknown /* Must always be last */
Comment is semi-useless. Comma at the end (or its absence) is usually
enough to give a clue, but okay, comment makes this explicit.
I prefer explicit but let me know if you feel strongly about this one.
...
+struct platform_profile {
+ unsigned int choices; /* bitmap of available choices */
+ int cur_profile; /* Current active profile */
Kernel doc?
Sure
+ int (*profile_get)(void);
+ int (*profile_set)(int profile);
+};
...
+extern int platform_profile_register(struct platform_profile *pprof);
+extern int platform_profile_unregister(void);
+extern int platform_profile_notify(void);
extern is not needed.
I will remove.
Thanks for the detailed review - very much appreciated.
Mark
