From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CD2DA132132 for ; Tue, 27 Aug 2024 19:04:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724785480; cv=none; b=QuzEgxP6xnE0xUVuJHao/DV5iOC70hmmGubRSmIlmmC6HUfB44yZkFouVNp6Jiir5goUkHhm9CpsQ/A3I9eE8HKnorHtoKjdizBI+NOOGOHGBSn7MWGKUno0C5WpDgfwJo0eBI1E2RuFOCVbsGMHMQ57YGE5AHJL5Nzho0PWPt8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724785480; c=relaxed/simple; bh=xB+tRyst4h/JkZ6zWo+HspsZwigVOpyOfVR2Meo+9zo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=VYh7kpdnYRK9yReBeKIhOmjx3Hid4XTqdSI8iPrGNrLtMBWk/GClqLKjk2uLs+5exkobBlaDArjOXOSWkFC3iMqFW7ICuHf/+0gCHSYgvoAG0uuLN+RdFtcwYNqC4hK1+yVoy7uM1GyupyLh+jN8eZjmWibPyFi9U0E2BT5x41I= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=Bpkp72ik; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Bpkp72ik" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1724785477; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HkWCAaDqu2XZmQMkoLU93xiUV6PoV/L38xqUed5LNOs=; b=Bpkp72ikXTdmmgpP2Pdo238sWRgFj4sHQe3o8RJWL84c/z+reS/6LlsFZr7/lJmV/J/xU5 ZcZwzdG0KM3+cSiS7UQL8RAQhSB4f4uG8AQHfsR1g1UimHMRTdREfj8TN9BBmC1mwWdvyj z+pOvYYR1VryTXa/vdEKbK/dR/xZdFc= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-569-j11tp4xJMN6UD1__FEUnlQ-1; Tue, 27 Aug 2024 15:04:29 -0400 X-MC-Unique: j11tp4xJMN6UD1__FEUnlQ-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-42817980766so53332925e9.3 for ; Tue, 27 Aug 2024 12:04:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724785463; x=1725390263; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HkWCAaDqu2XZmQMkoLU93xiUV6PoV/L38xqUed5LNOs=; b=OPUOl64PyT+nzzSfek0eJzZLQN5Eyu/9sikTTZPnEAqUovZhdcSsjGfwAWggZyhlKL 2p+6HITTaDQ0Zr0VRf+1dk+VHmvYW0nTwDqJVFW2PVbX6ilJq7huu9o94CFVaZ+400Ge jEJVA1p6H8J8ELDz1Se82pNASb5KK1xlktvVKHDRpYDh/HVO9gOp3jbzTCpX0QiGctMh vw8SxAk+yHQ5TWUBc7+WxFrQ1RCXOXpEOE/3O6CCVPsjAUDg+laf8h8zGXztktdKXa5s suDrVXTRc/+y+MwMqmdJOou/Z5oNO8HlYW+PMdWs+pm1R3JLaPr0mhPWxTGBq0skJgK3 JO7A== X-Gm-Message-State: AOJu0Yx2iwVu5rx5/gIy52wEVptiy3xm44dnnW7TfAfBdC8CV5CAPQEY uUfCk6jtpzlXeGScK58s4EhyC4EmXOcA8zH1U9ucJrCvP5O8Np78bBTGIerMEjHJkG6FGb1KTvu wCDW/HksBW+xqurQBV8OsEFXoMKRPxfLoueH1C0wqBvK11dI6DB7zW4huHCsghw== X-Received: by 2002:a05:6000:400f:b0:371:8f19:bff5 with SMTP id ffacd0b85a97d-37311857877mr11933153f8f.3.1724785463433; Tue, 27 Aug 2024 12:04:23 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF1i/xHwfH62KeEf74CXSi1HrJpaaxieMRJplttOqZqnN5NHZ0E2WbImpYcSF+/j2WB6c89uQ== X-Received: by 2002:a05:6000:400f:b0:371:8f19:bff5 with SMTP id ffacd0b85a97d-37311857877mr11933128f8f.3.1724785462851; Tue, 27 Aug 2024 12:04:22 -0700 (PDT) Received: from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec? (2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl. [2001:1c00:c32:7800:5bfa:a036:83f0:f9ec]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c0bb20a5a4sm1305368a12.42.2024.08.27.12.04.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Aug 2024 12:04:22 -0700 (PDT) Message-ID: Date: Tue, 27 Aug 2024 21:04:20 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings To: Andres Salomon Cc: linux-kernel@vger.kernel.org, =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= , =?UTF-8?Q?Pali_Roh=C3=A1r?= , platform-driver-x86@vger.kernel.org, Matthew Garrett , Sebastian Reichel , =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= , linux-pm@vger.kernel.org, Dell.Client.Kernel@dell.com References: <20240820033005.09e03af1@5400> <04d48a7c-cad1-4490-bbcd-ceb332c740bd@redhat.com> <20240827142408.0748911f@5400> Content-Language: en-US, nl From: Hans de Goede In-Reply-To: <20240827142408.0748911f@5400> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi, On 8/27/24 8:24 PM, Andres Salomon wrote: > On Mon, 26 Aug 2024 16:44:35 +0200 > Hans de Goede wrote: > >> Hi Andres, >> >> On 8/20/24 9:30 AM, Andres Salomon wrote: > [...] >>> + >>> +static ssize_t charge_type_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + ssize_t count = 0; >>> + int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) { >>> + bool active; >>> + >>> + if (!(battery_supported_modes & BIT(i))) >>> + continue; >>> + >>> + active = dell_battery_mode_is_active(battery_modes[i].token); >>> + count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ", >>> + battery_modes[i].label); >>> + } >> >> If you look at the way how charge_type is shown by the power_supply_sysfs.c >> file which is used for power-supply drivers which directly register >> a power-supply themselves rather then extending an existing driver, this >> is not the correct format. >> >> drivers/power/supply/power_supply_sysfs.c >> >> lists charge_type as: >> >> POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE), >> >> and ENUM type properties use the following for show() : >> >> default: >> if (ps_attr->text_values_len > 0 && >> value.intval < ps_attr->text_values_len && value.intval >= 0) { >> ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); >> } else { >> ret = sysfs_emit(buf, "%d\n", value.intval); >> } >> } >> >> with in this case text_values pointing to: >> >> static const char * const POWER_SUPPLY_CHARGE_TYPE_TEXT[] = { >> [POWER_SUPPLY_CHARGE_TYPE_UNKNOWN] = "Unknown", >> [POWER_SUPPLY_CHARGE_TYPE_NONE] = "N/A", >> [POWER_SUPPLY_CHARGE_TYPE_TRICKLE] = "Trickle", >> [POWER_SUPPLY_CHARGE_TYPE_FAST] = "Fast", >> [POWER_SUPPLY_CHARGE_TYPE_STANDARD] = "Standard", >> [POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE] = "Adaptive", >> [POWER_SUPPLY_CHARGE_TYPE_CUSTOM] = "Custom", >> [POWER_SUPPLY_CHARGE_TYPE_LONGLIFE] = "Long Life", >> [POWER_SUPPLY_CHARGE_TYPE_BYPASS] = "Bypass", >> }; >> >> So value.intval will be within the expected range hitting: >> >> ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]); >> >> IOW instead of outputting something like this: >> >> Fast [Standard] Long Life >> >> which is what your show() function does it outputs only >> the active value as a string, e.g.: >> >> Standard >> >> Yes not being able to see the supported values is annoying I actually >> wrote an email about that earlier today: >> >> https://lore.kernel.org/linux-pm/49993a42-aa91-46bf-acef-4a089db4c2db@redhat.com/ >> >> but we need to make sure that the output is consistent between drivers otherwise >> userspace can never know how to use the API, so for charge_type the dell >> driver should only output the active type, not all the options. > > So should I just wait to make any changes until you hear back in that > thread? Yes that might be best. > I'm not overly excited about changing it to use the current > charge_type API, given that the only way to get a list of modes that the > hardware supports is to try setting them all and seeing what fails. > > I suppose another option is to rename it to charge_types in the dell > driver under the assumption that your proposed charge_types API (or > something like it) will be added.. Right, if we get a favorable reaction to my charge_types suggestion then we can go ahead with the dell-laptop changes using charge_types instead of charge_type. I was already thinking along those lines myself too. So if my RFC gets a favorable response lets do that. In that case you don't even need to send a new version just renaming charge_type to charge_types is something which I can do while merging this. >> This reminds me that there was a patch-series to allow battery extension drivers >> like this one to actually use the power-supply core code for show()/store() >> Thomas IIRC that series was done by you ? What is the status of that ? >> >> Also looking at the userspace API parts of this again I wonder >> if mapping BAT_PRI_AC_MODE_TOKEN -> "Trickle" is the right thing do >> maybe "Long Life" would be a better match ? That depends on what the option >> actually does under the hood I guess. Is this known ? >> > > I originally thought to use Long Life rather than Trickle. We discussed > it here: > > https://lore.kernel.org/linux-pm/5cfe4c42-a003-4668-8c3a-f18fb6b7fba6@gmx.de/ > > Based on the existing documentation and the fact that the wilco driver > already mapped it, it was decided to stick with the existing precedent > of using Trickle. Ok, I was just wondering if this was discussed already, since it was lets stick with "Trickle". > That said, Armin at first suggested creating a new "Primarily AC" entry. > That's personally my favorite option, though I understand if we don't > have to have 50 CHARGE_TYPE entries that just slightly different > variations. :) Right. Regards, Hans