Re: [PATCH 5/6] platform/x86: uniwill-laptop: Mark EC_ADDR_OEM_4 as volatile

[Werner Sembach <wse@tuxedocomputers.com>]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> It turned out that EC_ADDR_OEM_4 also contains bits with a volatile
> nature. Mark the whole register as volatile to prepare for the usage
> of said bits. This also means that we now have to save/restore the
> touchpad toggle state ourself.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
looks good

Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 70 ++++++++++++++++++---
>   1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 4d149b39d94e..a116e2ec6453 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -343,6 +343,7 @@ struct uniwill_data {
>   	struct mutex battery_lock;	/* Protects the list of currently registered batteries */
>   	bool last_fn_lock_state;
>   	bool last_super_key_enable_state;
> +	bool last_touchpad_toggle_enable_state;
>   	struct mutex super_key_lock;	/* Protects the toggling of the super key lock state */
>   	struct list_head batteries;
>   	struct mutex led_lock;		/* Protects writes to the lightbar registers */
> @@ -598,6 +599,7 @@ static bool uniwill_volatile_reg(struct device *dev, unsigned int reg)
>   	case EC_ADDR_PWM_2:
>   	case EC_ADDR_TRIGGER:
>   	case EC_ADDR_SWITCH_STATUS:
> +	case EC_ADDR_OEM_4:
>   	case EC_ADDR_CHARGE_CTRL:
>   	case EC_ADDR_USB_C_POWER_PRIORITY:
>   		return true;
> @@ -747,11 +749,22 @@ static ssize_t super_key_enable_show(struct device *dev, struct device_attribute
>   
>   static DEVICE_ATTR_RW(super_key_enable);
>   
> +static int uniwill_write_touchpad_toggle_enable(struct uniwill_data *data, bool status)
> +{
> +	unsigned int value;
> +
> +	if (status)
> +		value = 0;
> +	else
> +		value = TOUCHPAD_TOGGLE_OFF;
> +
> +	return regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
> +}
> +
>   static ssize_t touchpad_toggle_enable_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;
>   	bool enable;
>   	int ret;
>   
> @@ -759,30 +772,39 @@ static ssize_t touchpad_toggle_enable_store(struct device *dev, struct device_at
>   	if (ret < 0)
>   		return ret;
>   
> -	if (enable)
> -		value = 0;
> -	else
> -		value = TOUCHPAD_TOGGLE_OFF;
> -
> -	ret = regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
> +	ret = uniwill_write_touchpad_toggle_enable(data, enable);
>   	if (ret < 0)
>   		return ret;
>   
>   	return count;
>   }
>   
> +static int uniwill_read_touchpad_toggle_enable(struct uniwill_data *data, bool *status)
> +{
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = !(value & TOUCHPAD_TOGGLE_OFF);
> +
> +	return 0;
> +}
> +
>   static ssize_t touchpad_toggle_enable_show(struct device *dev, struct device_attribute *attr,
>   					   char *buf)
>   {
>   	struct uniwill_data *data = dev_get_drvdata(dev);
> -	unsigned int value;
> +	bool status;
>   	int ret;
>   
> -	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
> +	ret = uniwill_read_touchpad_toggle_enable(data, &status);
>   	if (ret < 0)
>   		return ret;
>   
> -	return sysfs_emit(buf, "%d\n", !(value & TOUCHPAD_TOGGLE_OFF));
> +	return sysfs_emit(buf, "%d\n", status);
>   }
>   
>   static DEVICE_ATTR_RW(touchpad_toggle_enable);
> @@ -1774,6 +1796,18 @@ static int uniwill_suspend_super_key(struct uniwill_data *data)
>   	return uniwill_read_super_key_enable(data, &data->last_super_key_enable_state);
>   }
>   
> +static int uniwill_suspend_touchpad_toggle(struct uniwill_data *data)
> +{
> +	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
> +		return 0;
> +
> +	/*
> +	 * EC_ADDR_OEM_4 is marked as volatile, so we have to restore it
> +	 * ourselves.
> +	 */
> +	return uniwill_read_touchpad_toggle_enable(data, &data->last_touchpad_toggle_enable_state);
> +}
> +
>   static int uniwill_suspend_battery(struct uniwill_data *data)
>   {
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
> @@ -1809,6 +1843,10 @@ static int uniwill_suspend(struct device *dev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = uniwill_suspend_touchpad_toggle(data);
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = uniwill_suspend_battery(data);
>   	if (ret < 0)
>   		return ret;
> @@ -1839,6 +1877,14 @@ static int uniwill_resume_super_key(struct uniwill_data *data)
>   	return uniwill_write_super_key_enable(data, data->last_super_key_enable_state);
>   }
>   
> +static int uniwill_resume_touchpad_toggle(struct uniwill_data *data)
> +{
> +	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
> +		return 0;
> +
> +	return uniwill_write_touchpad_toggle_enable(data, data->last_touchpad_toggle_enable_state);
> +}
> +
>   static int uniwill_resume_battery(struct uniwill_data *data)
>   {
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
> @@ -1884,6 +1930,10 @@ static int uniwill_resume(struct device *dev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = uniwill_resume_touchpad_toggle(data);
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = uniwill_resume_battery(data);
>   	if (ret < 0)
>   		return ret;