On 4/21/21 6:15 PM, Jonas Malaco wrote: > On Wed, Apr 21, 2021 at 05:15:37PM -0700, Guenter Roeck wrote: >> On Wed, Apr 21, 2021 at 08:31:27PM -0300, Jonas Malaco wrote: >>> On Wed, Apr 21, 2021 at 10:21:36AM -0700, Guenter Roeck wrote: >>>> On Wed, Apr 21, 2021 at 01:48:03PM -0300, Jonas Malaco wrote: >>>>> On Tue, Apr 13, 2021 at 09:45:29AM -0300, Jonas Malaco wrote: >>>>>> Guenter (and others on this list), >>>>> >>>>> Very gentle ping. >>>>> >>>>> I also thought posting these questions first would be less disruptive >>>>> than a RFC patch, but please let me know if you prefer the latter. >>>>> >>>> >>>> It is a difficult subject, and I am struggling myself with the situations >>>> you are presenting. >>> >>> I am somewhat relieved that these issues are not so silly. And I really >>> appropriate your comments. >>> >>> Please take a look at a few more comments bellow. >>> >>>> >>>>> Thanks again, >>>>> Jonas >>>>> >>>>>> >>>>>> I am getting ready to submit a driver for NZXT Grid+ V3 and Smart Device >>>>>> (V1) fan controllers, but I am having trouble deciding how to expose >>>>>> their PWM control due to some device limitations. >>>>>> >>>>>> Before getting into those, let me first give some very basic context... >>>>>> >>>>>> These devices are USB HIDs, and asynchronously send "status" reports >>>>>> every 200 ms to communicate speed, current, voltage and control mode for >>>>>> their channels (one channel per report). >>>>>> >>>>>> Fans can be controlled by sending HID output reports to the device, and >>>>>> both DC and PWM modes are supported. The device features a special >>>>>> initialization routine (that must be requested during probe) which >>>>>> automatically detects the appropriate control mode for each channel. >>>>>> >>>>>> Back to the device limitations... >>>>>> >>>>>> The first is that PWM values can be set, but not read back. And neither >>>>>> hwmon[1] nor lm-sensors' pwmconfig/fancontrol expect pmw* attributes to >>>>>> be WO. One solution is to have the driver track the PWM values that are >>>>>> set through it and return those, but is this acceptable? >>>> >>>> I have seen a couple of those recently. I think returning -ENODATA >>>> if the value isn't known (yet) is the best possible solution. I thought >>>> about adding that to the ABI, actually. >>> >>> We can never read the pwm[1-*] attributes, not even for detected and >>> controllable fans after the initialization procedure. >>> >>> And returning -ENODATA for pwm[1-*] reads makes pwmconfig/fancontrol >>> unhappy: >>> >> >> Seems to me that pwmconfig is then maybe not appropriate to use, >> and maybe there should be no driver for this device in the kernel >> in the first place. > > I see your point. Although, apart from this rather serious quirk, these > devices work really well with pwmconfig/fancontrol. > >> >> Returning a random value after setting the pwm value to 255, removing, >> and re-inserting the driver is, in my opinion, even worse than >> returning -ENODATA. After all, the driver doesn't know the pwm value, >> and it is really a bad idea to report data which doesn't reflect >> reality. > > Well, in this particular case, the driver must reinitialize the device > anyway, since it cannot know whether the device was already initialized. > Maybe the driver was removed because the device itself was disconnected > and powered off (it's a USB device, not a chip on a board). > > So in the work-in-progress driver I take this opportunity to also make > sure that its state matches the device's, and force all fans to their > default PWM values after powering on. > > Specifically, besides requesting the device to initialize itself, I also > (re)set all fans to 40% PWM. (This does not require waiting for the > initialization to be complete). > > Is this egregious? > SGTM, just make sure this is well documented, and make sure that the initial pwm value is reported as 40%, ie 102. Thanks, Guenter
