Re: [PATCH v2 2/4] platform/x86/amd/pmf: Add sysfs to toggle CnQF

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 9/12/2022 04:06, Shyam Sundar S K wrote:
> Whether to turn CnQF on/off by default upon driver load would be decided
> by a BIOS flag. Add a sysfs node to provide a way to the user whether to
> use static slider or CnQF .
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/cnqf.c | 57 +++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index aebcef778a0b..8d0c1eff1659 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -294,9 +294,64 @@ void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
>   		config_store.trans_prio[i] = i;
>   }
>   
> +static ssize_t feat_store(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
> +	int mode = amd_pmf_get_pprof_modes(pdev);
> +	int result, src;
> +	bool input;
> +
> +	result = kstrtobool(buf, &input);
> +	if (result)
> +		return result;
> +
> +	src = amd_pmf_get_power_source();
> +	pdev->cnqf_enabled = input;
> +
> +	if (mode < 0)
> +		return mode;
> +
> +	if (pdev->cnqf_enabled) {
> +		amd_pmf_set_cnqf(pdev, src, config_store.current_mode, NULL);
> +	} else {
> +		if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) {
> +			amd_pmf_init_sps(pdev);
> +			amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL);
> +		}
> +	}
> +
> +	dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off");
> +	return count;
> +}
> +
> +static ssize_t feat_show(struct device *dev,
> +			 struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off");
> +}
> +
> +static DEVICE_ATTR_RW(feat);
> +
> +static struct attribute *cnqf_feature_attrs[] = {
> +	&dev_attr_feat.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group cnqf_feature_attribute_group = {
> +	.attrs = cnqf_feature_attrs,
> +	.name = "cnqf"

Perhaps you should have a "is_visible" controlled by a function that 
checks "is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)".

This will then let you adjust "feat_store" to not have to check this and 
also only expose the attribute on supported systems.

> +};
> +
>   void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev)
>   {
>   	cancel_delayed_work_sync(&dev->work_buffer);
> +	sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group);
> +	kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE);
>   }
>   
>   void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
> @@ -316,4 +371,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev)
>   	/* update the thermal for CnQF */
>   	src = amd_pmf_get_power_source();
>   	amd_pmf_set_cnqf(dev, src, config_store.current_mode, NULL);
> +	ret = sysfs_create_group(&dev->dev->kobj, &cnqf_feature_attribute_group);
> +	kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE);
>   }