On Wed, Apr 21, 2021 at 06:30:42PM -0700, Guenter Roeck wrote: > 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. Of course! :) I'll send a v1 in the next few days. Thanks, Jonas > > Thanks, > Guenter
