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 > > [...]
