Re: [PATCH] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

[Armin Wolf <W_Armin@gmx.de>]

Am 16.12.24 um 20:42 schrieb Joshua Grisham:

> Den sön 15 dec. 2024 kl 18:16 skrev Joshua Grisham <josh@joshuagrisham.com>:
>>>> +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

>>> [...]