Am 16.12.24 um 20:42 schrieb Joshua Grisham:
Den sön 15 dec. 2024 kl 18:16 skrev Joshua Grisham <josh@xxxxxxxxxxxxxxxxx>:
+static int charge_control_end_threshold_acpi_set(struct samsung_galaxybook *galaxybook,
+ const u8 value)
While certainly not forbidden, using const on plain integer types is not
extremely useful. In fact, if it wouldn't be const, you could do the 100
-> 0 mapping for it separately and not do it twice below.
[...]
Put comment on line before it so it's easier to read.
"off" -> "no threshold" ?
[...]
Good idea, now I have handled this in the v2 of the patch as follows:
if (value > 100)
return -EINVAL;
/* if setting to 100, should be set to 0 (no threshold) */
if (value == 100)
value = 0;
Does this make sense now or do you see anything that should be adjusted here?
Do you want to differentiate 0 from 0? Should this function actually
return -EINVAL if somebody attempts to set 0 threshold?
And regarding this, the device requires that you send 0 to represent
that the feature is "turned off", so to speak (no threshold is enabled
and the battery will charge all the way to 100%). So yes, in my mind,
we want to send 0 to the device if you are attempting to set either 0
or 100. Also I seem to recall that I tried to dig into how this is
handled in upower and the coming features in GNOME, and have a vague
memory that I saw somewhere in there that they were also converting
100 to a 0, but now I am having a bit of trouble finding this again.
Do you know if it would be better to have this driver provide an
interface where "100" means "no threshold" and that it should be
translated within the driver (that samsung_galaxybook sends a 0 to the
ACPI in case the user has requested "100" ?) or is it better if "0"
means "no threshold/charge to 100%" (or both?)?
I can also do some testing with the device to see if it accepts the
value 100 anyway, and how it behaves, though I would be a little
concerned with this longer term as it is not how the driver and
settings applications work in Windows (they are hard-coded with a
toggle and it always sets either 0 (off) or 80 (on)), and I could see
where even if it works today, sending the value of 100 to mean "off"
could be altered by potential BIOS updates?
Just wanted to follow up on one thing here -- I tested a bit more with
the device and how it handles trying to set 100 vs 0. Basically, if
you just directly call the ACPI method and set the value 100, the
device actually changes it to 0 and stores it as 0 anyway. The next
fetch of the sysfs show attribute it will return 0 (so basically the
device itself behaves exactly the same as the current logic in the
driver).
So the question is, how should this behave as regards to a "standard
interface" for battery charge_control_end_threshold ? Should it prefer
to report 100 or 0 if there is "no threshold" ? And if it should
prefer 100, should this driver handle the translation of 0 => 100 => 0
? (i.e. users set the value to 100 but the driver sends 0 to the
device, and when the device reports 0, the driver reports 100)
Thank you again!
Best regards,
Joshua
I think in this case the driver should transparently handle the 100 -> 0 translation
and report 100 if there is no threshold.
Thanks,
Armin Wolf
[...]
