Re: [PATCH 2/4] platform/x86/uniwill: Implement USB-C power priority setting

[Armin Wolf <W_Armin@gmx.de>]

Am 26.02.26 um 01:31 schrieb Werner Sembach:

> On some devices Uniwill offers the option to set the USB-C port to
> prioritise charging or performance. This patch exposes this setting to the
> userspace via sysfs for all TUXEDO devices supporting it.
>
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 99 +++++++++++++++++++--
>   1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 7ab82cf16f388..4d9657a3b8c10 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -266,8 +266,8 @@
>   #define BATTERY_CHARGE_FULL_OVER_24H	BIT(3)
>   #define BATTERY_ERM_STATUS_REACHED	BIT(4)
>   
> -#define EC_ADDR_CHARGE_PRIO		0x07CC
> -#define CHARGING_PERFORMANCE		BIT(7)
> +#define EC_ADDR_USB_C_POWER_PRIORITY	0x07CC
> +#define USB_C_POWER_PRIORITY		BIT(7)
>   
>   /* Same bits as EC_ADDR_LIGHTBAR_AC_CTRL except LIGHTBAR_S3_OFF */
>   #define EC_ADDR_LIGHTBAR_BAT_CTRL	0x07E2
> @@ -324,6 +324,7 @@
>   #define UNIWILL_FEATURE_PRIMARY_FAN		BIT(7)
>   #define UNIWILL_FEATURE_SECONDARY_FAN		BIT(8)
>   #define UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL	BIT(9)
> +#define UNIWILL_FEATURE_USB_C_POWER_PRIORITY	BIT(10)
>   
>   struct uniwill_data {
>   	struct device *dev;
> @@ -343,6 +344,7 @@ struct uniwill_data {
>   	struct mutex input_lock;	/* Protects input sequence during notify */
>   	struct input_dev *input_device;
>   	struct notifier_block nb;
> +	unsigned int last_usb_c_power_priority;
>   };
>   
>   struct uniwill_battery_entry {
> @@ -527,6 +529,7 @@ static bool uniwill_writeable_reg(struct device *dev, unsigned int reg)
>   	case EC_ADDR_CTGP_DB_CTGP_OFFSET:
>   	case EC_ADDR_CTGP_DB_TPP_OFFSET:
>   	case EC_ADDR_CTGP_DB_DB_OFFSET:
> +	case EC_ADDR_USB_C_POWER_PRIORITY:
>   		return true;
>   	default:
>   		return false;
> @@ -565,6 +568,7 @@ static bool uniwill_readable_reg(struct device *dev, unsigned int reg)
>   	case EC_ADDR_CTGP_DB_CTGP_OFFSET:
>   	case EC_ADDR_CTGP_DB_TPP_OFFSET:
>   	case EC_ADDR_CTGP_DB_DB_OFFSET:
> +	case EC_ADDR_USB_C_POWER_PRIORITY:
>   		return true;
>   	default:
>   		return false;
> @@ -587,6 +591,7 @@ static bool uniwill_volatile_reg(struct device *dev, unsigned int reg)
>   	case EC_ADDR_TRIGGER:
>   	case EC_ADDR_SWITCH_STATUS:
>   	case EC_ADDR_CHARGE_CTRL:
> +	case EC_ADDR_USB_C_POWER_PRIORITY:
>   		return true;
>   	default:
>   		return false;
> @@ -883,6 +888,86 @@ static int uniwill_nvidia_ctgp_init(struct uniwill_data *data)
>   	return 0;
>   }
>   
> +enum usb_c_power_priority_options {
> +	USB_C_POWER_PRIORITY_OPTIONS_CHARGING = 0,
> +	USB_C_POWER_PRIORITY_OPTIONS_PERFORMANCE,
> +};
> +
> +static const char * const USB_C_POWER_PRIORITY_OPTIONS_TEXT[] = {
> +	[USB_C_POWER_PRIORITY_OPTIONS_CHARGING]		= "charging",
> +	[USB_C_POWER_PRIORITY_OPTIONS_PERFORMANCE]	= "performance",
> +};
> +
> +static const u8 USB_C_POWER_PRIORITY_OPTIONS_VALUE[] = {
> +	[USB_C_POWER_PRIORITY_OPTIONS_CHARGING]		= 0,
> +	[USB_C_POWER_PRIORITY_OPTIONS_PERFORMANCE]	= USB_C_POWER_PRIORITY,
> +};
> +
> +static ssize_t usb_c_power_priority_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct uniwill_data *data = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	ret = sysfs_match_string(USB_C_POWER_PRIORITY_OPTIONS_TEXT, buf);
> +	if (ret < 0)
> +		return ret;

Please put a blank line here.

> +	value = USB_C_POWER_PRIORITY_OPTIONS_VALUE[ret];
> +
> +	ret = regmap_update_bits(data->regmap, EC_ADDR_USB_C_POWER_PRIORITY,
> +				 USB_C_POWER_PRIORITY, value);
> +	if (ret < 0)
> +		return ret;
> +

I think you need a mutex here, so concurrent writes to the charging priority
sysfs attribute are serialized. This mutex would then also protect concurrent
access to data->last_usb_c_power_priority by the event handler.

> +	data->last_usb_c_power_priority = value;
> +
> +	return count;
> +}
> +
> +static ssize_t usb_c_power_priority_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct uniwill_data *data = dev_get_drvdata(dev);
> +	unsigned int value;
> +	ssize_t count = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, EC_ADDR_USB_C_POWER_PRIORITY, &value);
> +	if (ret < 0)
> +		return ret;
> +	value &= USB_C_POWER_PRIORITY;
> +
> +	for (int i = 0; i < ARRAY_SIZE(USB_C_POWER_PRIORITY_OPTIONS_VALUE); ++i) {
> +		if (USB_C_POWER_PRIORITY_OPTIONS_VALUE[i] == value)
> +			count += sysfs_emit_at(buf, count, "[%s] ",
> +					       USB_C_POWER_PRIORITY_OPTIONS_TEXT[i]);
> +		else
> +			count += sysfs_emit_at(buf, count, "%s ",
> +					       USB_C_POWER_PRIORITY_OPTIONS_TEXT[i]);
> +	}
> +	if (count)
> +		buf[count - 1] = '\n';

Please just emit USB_C_POWER_PRIORITY_OPTIONS_VALUE[USB_C_POWER_PRIORITY_OPTIONS_PERFORMANCE] directly
when the bit is set, and USB_C_POWER_PRIORITY_OPTIONS_VALUE[USB_C_POWER_PRIORITY_OPTIONS_CHARGING] otherwise.
This for loop provides no real value.

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(usb_c_power_priority);
> +
> +static int usb_c_power_priority_restore(struct uniwill_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, EC_ADDR_OEM_4, USB_C_POWER_PRIORITY,
> +				 data->last_usb_c_power_priority);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;

Please check the feature flag here before performing the register access. You should also
omit "ret" and instead return the result directly.

Maybe you also need to add the appropriate suspend/resume functions for this?

> +}
> +
>   static struct attribute *uniwill_attrs[] = {
>   	/* Keyboard-related */
>   	&dev_attr_fn_lock.attr,
> @@ -893,6 +978,7 @@ static struct attribute *uniwill_attrs[] = {
>   	&dev_attr_breathing_in_suspend.attr,
>   	/* Power-management-related */
>   	&dev_attr_ctgp_offset.attr,
> +	&dev_attr_usb_c_power_priority.attr,
>   	NULL
>   };
>   
> @@ -927,6 +1013,11 @@ static umode_t uniwill_attr_is_visible(struct kobject *kobj, struct attribute *a
>   			return attr->mode;
>   	}
>   
> +	if (attr == &dev_attr_usb_c_power_priority.attr) {
> +		if (uniwill_device_supports(data, UNIWILL_FEATURE_USB_C_POWER_PRIORITY))
> +			return attr->mode;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -1417,9 +1508,7 @@ static int uniwill_notifier_call(struct notifier_block *nb, unsigned long action
>   
>   		return NOTIFY_OK;
>   	case UNIWILL_OSD_DC_ADAPTER_CHANGED:
> -		/* noop for the time being, will change once charging priority
> -		 * gets implemented.
> -		 */
> +		usb_c_power_priority_restore(data);

Please use notifier_from_errno() here.

Thanks,
Armin Wolf

>   
>   		return NOTIFY_OK;
>   	case UNIWILL_OSD_FN_LOCK: