Re: [PATCH v7 13/22] ACPI: platform_profile: Add profile attribute for class interface

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 19 Nov 2024, Mario Limonciello wrote:

> Reading and writing the `profile` sysfs file will use the callbacks for
> the platform profile handler to read or set the given profile.
> 
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v7:
>  * Remove extra handler set
>  * Remove err variable
> v6:
>  * Fix return
> v5:
>  * Drop recovery flow
>  * Don't get profile before setting (not needed)
>  * Simplify casting for call to _store_class_profile()
>  * Only notify legacy interface of changes
>  * Adjust mutex use
> ---
>  drivers/acpi/platform_profile.c | 100 ++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 9d6ead043994c..1530e6096cd39 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -46,6 +46,52 @@ static ssize_t _commmon_choices_show(unsigned long *choices, char *buf)
>  	return len;
>  }
>  
> +/**
> + * _store_class_profile - Set the profile for a class device
> + * @dev: The class device
> + * @data: The profile to set
> + */
> +static int _store_class_profile(struct device *dev, void *data)
> +{
> +	struct platform_profile_handler *handler;
> +	int *i = (int *)data;

I don't like using "i" as the variable name for this.

> +	lockdep_assert_held(&profile_lock);
> +	handler = dev_get_drvdata(dev);
> +	if (!test_bit(*i, handler->choices))
> +		return -EOPNOTSUPP;
> +
> +	return handler->profile_set(handler, *i);
> +}
> +
> +/**
> + * get_class_profile - Show the current profile for a class device
> + * @dev: The class device
> + * @profile: The profile to return
> + * Return: 0 on success, -errno on failure
> + */
> +static int get_class_profile(struct device *dev,
> +			     enum platform_profile_option *profile)
> +{
> +	struct platform_profile_handler *handler;
> +	enum platform_profile_option val;
> +	int err;
> +
> +	lockdep_assert_held(&profile_lock);
> +	handler = dev_get_drvdata(dev);
> +	err = handler->profile_get(handler, &val);
> +	if (err) {
> +		pr_err("Failed to get profile for handler %s\n", handler->name);
> +		return err;
> +	}
> +
> +	if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
> +		return -EINVAL;
> +	*profile = val;
> +
> +	return 0;
> +}
> +
>  /**
>   * name_show - Show the name of the profile handler
>   * @dev: The device
> @@ -77,12 +123,66 @@ static ssize_t choices_show(struct device *dev,
>  	return _commmon_choices_show(handler->choices, buf);
>  }
>  
> +/**
> + * profile_show - Show the current profile for a class device
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to write to
> + * Return: The number of bytes written
> + */
> +static ssize_t profile_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> +	int err;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		err = get_class_profile(dev, &profile);
> +		if (err)
> +			return err;
> +	}
> +
> +	return sysfs_emit(buf, "%s\n", profile_names[profile]);
> +}
> +
> +/**
> + * profile_store - Set the profile for a class device
> + * @dev: The device
> + * @attr: The attribute
> + * @buf: The buffer to read from
> + * @count: The number of bytes to read
> + * Return: The number of bytes read
> + */
> +static ssize_t profile_store(struct device *dev,
> +			     struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	int i, ret;
> +
> +	i = sysfs_match_string(profile_names, buf);

Please consider if there's a better name for the variable.

> +	if (i < 0)
> +		return -EINVAL;
> +
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
> +		ret = _store_class_profile(dev, &i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	sysfs_notify(acpi_kobj, NULL, "platform_profile");
> +
> +	return count;
> +}
> +
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(choices);
> +static DEVICE_ATTR_RW(profile);
>  
>  static struct attribute *profile_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_choices.attr,
> +	&dev_attr_profile.attr,

I started to wonder if "choices" is good name for the other attribute as 
it is the set of values "profile" accepts? Should they be bound by the 
naming too like "profile_choices" or something along those lines so the
connection between the two is very evident?

>  	NULL
>  };
>  ATTRIBUTE_GROUPS(profile);
> 

-- 
 i.