On 9/21/2022 8:11 PM, Hans de Goede wrote:
> Hi,
>
> On 9/21/22 12:24, 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@xxxxxxx>
>> ---
>> drivers/platform/x86/amd/pmf/cnqf.c | 62 +++++++++++++++++++++++++++++
>> drivers/platform/x86/amd/pmf/core.c | 6 +++
>> drivers/platform/x86/amd/pmf/pmf.h | 1 +
>> 3 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
>> index fa994800692a..34021bb22064 100644
>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>> @@ -298,6 +298,68 @@ static int amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
>> return 0;
>> }
>>
>> +static ssize_t cnqf_enable_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, result, src;
>> + bool input;
>> +
>> + mode = amd_pmf_get_pprof_modes(pdev);
>> + if (mode < 0)
>> + return mode;
>> +
>> + result = kstrtobool(buf, &input);
>> + if (result)
>> + return result;
>> +
>> + src = amd_pmf_get_power_source();
>> + pdev->cnqf_enabled = input;
>> +
>> + if (pdev->cnqf_enabled && pdev->current_profile == PLATFORM_PROFILE_BALANCED) {
>> + 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);
>
> This amd_pmf_init_sps() calls is not necessary, this is already done at probe time
> when is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR) returns true.
>
> Please drop this.
>
>> + 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 cnqf_enable_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
>> +
>> + return sysfs_emit(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off");
>> +}
>> +
>> +static DEVICE_ATTR_RW(cnqf_enable);
>> +
>> +static umode_t cnqf_feature_is_visible(struct kobject *kobj,
>> + struct attribute *attr, int n)
>> +{
>> + struct device *dev = kobj_to_dev(kobj);
>> + struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
>> +
>> + return pdev->cnqf_supported ? attr->mode : 0;
>> +}
>> +
>> +static struct attribute *cnqf_feature_attrs[] = {
>> + &dev_attr_cnqf_enable.attr,
>> + NULL
>> +};
>> +
>> +const struct attribute_group cnqf_feature_attribute_group = {
>> + .is_visible = cnqf_feature_is_visible,
>> + .attrs = cnqf_feature_attrs,
>> +};
>> +
>> void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev)
>> {
>> cancel_delayed_work_sync(&dev->work_buffer);
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 99be1e6d57d3..44fe30726b62 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -391,10 +391,16 @@ static int amd_pmf_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct attribute_group *amd_pmf_driver_groups[] = {
>> + &cnqf_feature_attribute_group,
>> + NULL,
>> +};
>> +
>> static struct platform_driver amd_pmf_driver = {
>> .driver = {
>> .name = "amd-pmf",
>> .acpi_match_table = amd_pmf_acpi_ids,
>> + .dev_groups = amd_pmf_driver_groups,
>> },
>> .probe = amd_pmf_probe,
>> .remove = amd_pmf_remove,
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 24dd6fbff24c..097f3f5d33a3 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -422,5 +422,6 @@ int apmf_get_dyn_slider_def_dc(struct amd_pmf_dev *pdev, struct apmf_dyn_slider_
>> int amd_pmf_init_cnqf(struct amd_pmf_dev *dev);
>> void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
>> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>> +extern const struct attribute_group cnqf_feature_attribute_group;
>>
>> #endif /* PMF_H */
>
> Other then the remark about dropping the amd_pmf_init_sps() call this
> looks good to me, with that fixed this is:
Agreed. Will drop the call and fix that in v4.
Thanks,
Shyam
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
> Regards,
>
> Hans
>
