Re: [PATCH 2/3] ACPI: platform-profile: Add platform profile support

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Nov 10, 2020 at 5:35 AM Mark Pearson <markpearson@lenovo.com> 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).

...

>  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.

...

> +#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?

...

> +struct platform_profile *cur_profile;

Better naming since it's a global variable.
Is it supposed to be exported to modules?

...

> +DEFINE_MUTEX(profile_lock);

No static?

...

> +/* 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?).

> +       "Low-power",
> +       "Cool",
> +       "Quiet",
> +       "Balance",
> +       "Performance",

> +       "Unknown"

Leave the comma here.

> +};

...

> +       int i;
> +       int ret, count = 0;

count AFAICS should be size_t (or ssize_t).
Can you make them in reversed xmas tree order?

...

> +       return snprintf(buf, PAGE_SIZE, "%s", profile_str[profile]);

Nowadays we have sysfs_emit(), use it.

...

> +       /* 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() ?

...

> +static struct attribute *platform_profile_attributes[] = {
> +       &dev_attr_platform_profile_choices.attr,
> +       &dev_attr_platform_profile.attr,

> +       NULL,

Drop comma in terminator line.

> +};

...

> +module_init(platform_profile_init);
> +module_exit(platform_profile_exit);

Attach them to respective functions.

...

> +/*
> + * platform_profile.h - platform profile sysfs interface

No file name.

> + *
> + * 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?

> +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.

...

> +struct platform_profile {
> +       unsigned int choices; /* bitmap of available choices */
> +       int cur_profile;      /* Current active profile */

Kernel doc?

> +       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.

-- 
With Best Regards,
Andy Shevchenko