Re: [PATCH v3 1/8] cpufreq: CPPC: Add generic helpers for sysfs show/store

[Jie Zhan <zhanjie9@hisilicon.com>]

Hi Sumit,

On 10/1/2025 11:00 PM, Sumit Gupta wrote:
> Add generic show/store helper functions for u64 sysfs attributes:
> - cppc_cpufreq_sysfs_show_u64()
> - cppc_cpufreq_sysfs_store_u64()
> 
> Refactor auto_act_window and energy_performance_preference_val
> attributes to use these helpers, eliminating code duplication.
> 
> No functional changes.
> 
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Nice cleanup in general.  Some minor bits inline.
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 87 ++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 12de0ac7bbaf..732f35096991 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -781,6 +781,36 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>  	return 0;
>  }
>  
> +static ssize_t cppc_cpufreq_sysfs_show_u64(unsigned int cpu, int (*get_func)(int, u64 *), char *buf)
Wrap a bit into 80 chars?

BTW, trivial but I would prefer a symmetric param order, like:
show(buf, get_func, cpu)
store(buf, count, set_func, cpu)
> +{
> +	u64 val;
> +	int ret = get_func(cpu, &val);
> +
> +	if (ret == -EOPNOTSUPP)
> +		return sysfs_emit(buf, "<unsupported>\n");
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%llu\n", val);
> +}
> +
> +static ssize_t cppc_cpufreq_sysfs_store_u64(const char *buf, size_t count,
> +					    int (*set_func)(int, u64), unsigned int cpu)
> +{
> +	u64 val;
> +	int ret;
> +
> +	if (!buf || !set_func)
> +		return -EINVAL;
No need.
> +
> +	ret = kstrtou64(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = set_func((int)cpu, val);
> +	return ret ? ret : count;
I suppose it's preferred to avoid using ternary operators like this.
> +}
> +
Would be nicer to move cppc_cpufreq_sysfs_show/store_u64() to just above
where they are used, i.e. just before show_auto_act_window().
>  static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>  {
>  	struct cppc_cpudata *cpu_data = policy->driver_data;
> @@ -824,70 +854,23 @@ static ssize_t store_auto_select(struct cpufreq_policy *policy,
>  
>  static ssize_t show_auto_act_window(struct cpufreq_policy *policy, char *buf)
>  {
> -	u64 val;
> -	int ret;
> -
> -	ret = cppc_get_auto_act_window(policy->cpu, &val);
> -
> -	/* show "<unsupported>" when this register is not supported by cpc */
> -	if (ret == -EOPNOTSUPP)
> -		return sysfs_emit(buf, "<unsupported>\n");
> -
> -	if (ret)
> -		return ret;
> -
> -	return sysfs_emit(buf, "%llu\n", val);
> +	return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_auto_act_window, buf);
>  }
>  
> -static ssize_t store_auto_act_window(struct cpufreq_policy *policy,
> -				     const char *buf, size_t count)
> +static ssize_t store_auto_act_window(struct cpufreq_policy *policy, const char *buf, size_t count)
>  {
> -	u64 usec;
> -	int ret;
> -
> -	ret = kstrtou64(buf, 0, &usec);
> -	if (ret)
> -		return ret;
> -
> -	ret = cppc_set_auto_act_window(policy->cpu, usec);
> -	if (ret)
> -		return ret;
> -
> -	return count;
> +	return cppc_cpufreq_sysfs_store_u64(buf, count, cppc_set_auto_act_window, policy->cpu);
>  }
>  
>  static ssize_t show_energy_performance_preference_val(struct cpufreq_policy *policy, char *buf)
>  {
> -	u64 val;
> -	int ret;
> -
> -	ret = cppc_get_epp_perf(policy->cpu, &val);
> -
> -	/* show "<unsupported>" when this register is not supported by cpc */
> -	if (ret == -EOPNOTSUPP)
> -		return sysfs_emit(buf, "<unsupported>\n");
> -
> -	if (ret)
> -		return ret;
> -
> -	return sysfs_emit(buf, "%llu\n", val);
> +	return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_epp_perf, buf);
>  }
>  
>  static ssize_t store_energy_performance_preference_val(struct cpufreq_policy *policy,
>  						       const char *buf, size_t count)
>  {
> -	u64 val;
> -	int ret;
> -
> -	ret = kstrtou64(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	ret = cppc_set_epp(policy->cpu, val);
> -	if (ret)
> -		return ret;
> -
> -	return count;
> +	return cppc_cpufreq_sysfs_store_u64(buf, count, cppc_set_epp, policy->cpu);
>  }
>  
>  cpufreq_freq_attr_ro(freqdomain_cpus);