On Sun, Oct 27, 2024 at 11:16 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > On 10/27/24 10:48, Derek J. Clark wrote: > > Greetings all, > > > > I am working with Cryolita to fix up the GPD driver she submitted recently: > > https://lore.kernel.org/all/20240718-gpd_fan-v4-0-116e5431a9fe@xxxxxxxxx/ > > > > We are currently having a discussion about the meaning of this part of the > > documentation and are seeking some guidance from upstream. > > >> pwm[1-*]_enable > >> Fan speed control method: > >> 0: no fan speed control (i.e. fan at full speed) > >> 1: manual fan speed control enabled (using pwm[1-*]) > >> 2+: automatic fan speed control enabled > >> Check individual chip documentation files for automatic mode > >> details. > >> RW > > > > In oxp-sensors we took 0 to mean "no kernel control" so a setting of 0 is > > technically "automatic" but fully controlled by the hardware with no > > interaction from the driver. In her original driver draft she had taken this > > That is wrong. It should be (or have been) 2 or higher. Ah yes, I can see that > the code sets fan control to automatic when oxp_pwm_disable() is called. > Again, that is wrong. Congratulations to the submitters, you sneaked that by > my review. That doesn't make it better. It is still wrong, and I call it "sneaky" > because the function is not called "oxp_pwm_automatic()" or similar, it is > called "oxp_pwm_disable(). Setting fan control to automatic does not disable > fan control. > > My bad, I should have paid closer attention, and/or maybe not have trusted > the submitters as much as I did. I guess I'll have to pay closer attention > in the future. > > > literally to have the driver set the fan speed to 100% on this setting rather > > then give back control to the hardware. My question is simply what is the > > correct interpretation here? Ideally I would like to see this interface match > > It seems to me that the above text is well defined. > > > as existing userspace software is expecting 0 as hardware controlled and 1 as > > manually controlled, but we also want to ensure this is correct before we > > submit a v5. > > > > Any such userspace expectations are simply wrong. The ABI definition is above > and, again, it is well defined. > > 0: no fan speed control (i.e. fan at full speed) > > I don't really see any ambiguity in this text. This isn't about kernel control, > it is an absolute statement. There is no "kernel" in "no fan speed control". > "fan at full speed" should make this even more obvious. > > Guenter Guenter, I'll keep this in mind in the future, and I will send a patch soon to fix the oxp_sensors driver. One final question, is a "0" setting mandatory? Thanks for the quick reply. Derek
