Re: [PATCH] platform/x86: asus-wmi: store a min default for ppt options
From: Ilpo Järvinen
Date: Wed Mar 20 2024 - 09:13:54 EST
On Mon, 11 Mar 2024, Luke D. Jones wrote:
> Laptops with any of the ppt or nv tunables default to the minimum setting
> on boot so we can safely assume a stored value is correct.
>
> This patch adds storing of those values in the local struct, and enables
> reading of those values back.
>
> Secondary to the above it renames some internal variables to be more
> consistent (which makes code grepping show all related parts)
>
> Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> ---
> drivers/platform/x86/asus-wmi.c | 141 +++++++++++++++++++++++++-------
> 1 file changed, 111 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index e4341abb71e0..482e23b55e1e 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -272,12 +272,19 @@ struct asus_wmi {
>
> /* Tunables provided by ASUS for gaming laptops */
> bool ppt_pl2_sppt_available;
> + u32 ppt_pl2_sppt;
> bool ppt_pl1_spl_available;
> + u32 ppt_pl1_spl;
> bool ppt_apu_sppt_available;
> - bool ppt_plat_sppt_available;
> + u32 ppt_apu_sppt;
> + bool ppt_platform_sppt_available;
> + u32 ppt_platform_sppt;
> bool ppt_fppt_available;
> - bool nv_dyn_boost_available;
> - bool nv_temp_tgt_available;
> + u32 ppt_fppt;
> + bool nv_dynamic_boost_available;
> + u32 nv_dynamic_boost;
> + bool nv_temp_target_available;
> + u32 nv_temp_target;
>
> bool kbd_rgb_mode_available;
> u32 kbd_rgb_dev;
Can you check with pahole if this structure is now full of 31-bit holes?
The benefit of keeping bool & u32 doesn't seem that big to begin with
(in visual sense because of the 1 char variation in column).
> @@ -999,11 +1006,10 @@ static ssize_t ppt_pl2_sppt_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> int result, err;
> u32 value;
>
> - struct asus_wmi *asus = dev_get_drvdata(dev);
> -
Please put this into own patch, it's entirely unrelated (but still useful
change!).
> result = kstrtou32(buf, 10, &value);
> if (result)
> return result;
> @@ -1022,22 +1028,31 @@ static ssize_t ppt_pl2_sppt_store(struct device *dev,
> return -EIO;
> }
>
> + asus->ppt_pl2_sppt = value;
> sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl2_sppt");
>
> return count;
> }
> -static DEVICE_ATTR_WO(ppt_pl2_sppt);
> +
> +static ssize_t ppt_pl2_sppt_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
Alignment is not correct?
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", asus->ppt_pl2_sppt);
> +}
> +static DEVICE_ATTR_RW(ppt_pl2_sppt);
>
> /* Tunable: PPT, Intel=PL1, AMD=SPL ******************************************/
> static ssize_t ppt_pl1_spl_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> int result, err;
> u32 value;
>
> - struct asus_wmi *asus = dev_get_drvdata(dev);
> -
Unrelated, put to the same move patch as the other change please. I won't
mark all thse from this point on but please do them all.
--
i.