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

[Werner Sembach <wse@tuxedocomputers.com>]

Am 27.02.26 um 10:27 schrieb Armin Wolf:
> Am 26.02.26 um 16:55 schrieb Werner Sembach:
>
>>
>> Am 26.02.26 um 02:11 schrieb Armin Wolf:
>>> 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.
>> ack
>>>
>>>> +    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.
>> ack
>>>
>>>> + 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.
>>
>> It does give context on which values are available. It it is otherwise not 
>> easily deduceable without referring to the documentation.
>>
>> Also USB_C_POWER_PRIORITY_OPTIONS_VALUE[USB_C_POWER_PRIORITY_OPTIONS_PERFORMANCE] 
>> is BIT(7) which is a kinda random number when expressed as an integer.
>>
> Sorry, i meant USB_C_POWER_PRIORITY_OPTIONS_TEXT[].
>
> I do not think that using the "option1 [option2]" syntax provides any real 
> benefit here. The users need to read the documentation
> anyway in order to even find this attribute, and the set of available options 
> is always the same.
>
> Please drop this.
ok
>
>>>
>>>> +
>>>> +    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.
>> ack
>>>
>>> Maybe you also need to add the appropriate suspend/resume functions for this?
>> tuxedo-drivers doesn't have it for this, so it's probably not necessary
>
> Please add the appropriate suspend/resume functions anyway, just in case the 
> user has
> connected the charger while the system was suspended.
ok, suspend is actually not necessary because last value is already saved on write
>
> Thanks,
> Armin Wolf
>
>>>
>>>> +}
>>>> +
>>>>   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.
>> ack
>>>
>>> Thanks,
>>> Armin Wolf
>>>
>>>>             return NOTIFY_OK;
>>>>       case UNIWILL_OSD_FN_LOCK:
>>