PWM control in NZXT Grid+ V3 and Smart Device

PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

Guenter (and others on this list),

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?

The other starts with PWM control being disabled for channels that the
device identifies as unconnected.  This is not in itself a problem, but
the initialization routine (where the detection happens) is
asynchronous, takes somewhere around 5 seconds, and we do not have any
way of directly querying its result.  We only know the control mode of
each channel (be it DC, PWM or disabled) from the regular status
reports.

These limitations make it complicated to simply use is_visible() to hide
pwm attributes of unconnected channels.  We would need to register with
the hwmon subsystem only after getting enough post-initialization status
reports for all channels, and this would essentially mean to sleep for
6+ seconds.  We would also need to unregister and re-register when going
through a suspend-reset_resume cycle, because the device may have its
state wiped, requiring reinitialization.[2]

A different approach to handle this, which I have preferred _so far,_ is
to use pwm*_enable = 0 to report the unconnected channels to user-space,
while keeping the other pwm attributes visible.  But this comes with
other problems.

First, lm-sensors' pwmconfig expects to be able to write to a
pwm*_enable attribute if it exists, but the device does not support that
operation.  The hwmon documentation states that RW values may be RO, but
pwmconfig is out there and in use.  So far I simply return 0 to attempts
at those writes, silently ignoring them; functional, but certainly a
hack.

Second, if PWM control is disabled for a channel, but its pwm* and
pwm*_mode attributes are still visible, what should we return when
user-space attempts to write to them?  The practical answer may simply
be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
(of handling these cases with pwm*_enable instead of is_visible()) is
not doomed.

A final minor problem is that channels detected as unconnected run at
40% PWM, but the documentation for pwm*_enable == 0 is a bit too
specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
mine).

Do you have any suggestions and/or recommendations?

If it helps, a pre-RFC (but functional and mostly clean) version of the
driver can be found at:

https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c

Thanks,
Jonas

[1] According to Documentation/hwmon/sysfs-interface.rst.
[2] The device also does not respond to HID Get_Report, so it is not
    trivial to check whether it really needs to be reinitialized, since
    the only symptom of that being necessary is the absence of the
    asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

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.

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?
> 
> The other starts with PWM control being disabled for channels that the
> device identifies as unconnected.  This is not in itself a problem, but
> the initialization routine (where the detection happens) is
> asynchronous, takes somewhere around 5 seconds, and we do not have any
> way of directly querying its result.  We only know the control mode of
> each channel (be it DC, PWM or disabled) from the regular status
> reports.
> 
> These limitations make it complicated to simply use is_visible() to hide
> pwm attributes of unconnected channels.  We would need to register with
> the hwmon subsystem only after getting enough post-initialization status
> reports for all channels, and this would essentially mean to sleep for
> 6+ seconds.  We would also need to unregister and re-register when going
> through a suspend-reset_resume cycle, because the device may have its
> state wiped, requiring reinitialization.[2]
> 
> A different approach to handle this, which I have preferred _so far,_ is
> to use pwm*_enable = 0 to report the unconnected channels to user-space,
> while keeping the other pwm attributes visible.  But this comes with
> other problems.
> 
> First, lm-sensors' pwmconfig expects to be able to write to a
> pwm*_enable attribute if it exists, but the device does not support that
> operation.  The hwmon documentation states that RW values may be RO, but
> pwmconfig is out there and in use.  So far I simply return 0 to attempts
> at those writes, silently ignoring them; functional, but certainly a
> hack.
> 
> Second, if PWM control is disabled for a channel, but its pwm* and
> pwm*_mode attributes are still visible, what should we return when
> user-space attempts to write to them?  The practical answer may simply
> be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> (of handling these cases with pwm*_enable instead of is_visible()) is
> not doomed.
> 
> A final minor problem is that channels detected as unconnected run at
> 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> mine).
> 
> Do you have any suggestions and/or recommendations?
> 
> If it helps, a pre-RFC (but functional and mostly clean) version of the
> driver can be found at:
> 
> https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> 
> Thanks,
> Jonas
> 
> [1] According to Documentation/hwmon/sysfs-interface.rst.
> [2] The device also does not respond to HID Get_Report, so it is not
>     trivial to check whether it really needs to be reinitialized, since
>     the only symptom of that being necessary is the absence of the
>     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Guenter Roeck]

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.

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

> > 
> > The other starts with PWM control being disabled for channels that the
> > device identifies as unconnected.  This is not in itself a problem, but
> > the initialization routine (where the detection happens) is
> > asynchronous, takes somewhere around 5 seconds, and we do not have any
> > way of directly querying its result.  We only know the control mode of
> > each channel (be it DC, PWM or disabled) from the regular status
> > reports.

Again, I think the best solution is to return -ENODATA until the value
is known.

> > 
> > These limitations make it complicated to simply use is_visible() to hide
> > pwm attributes of unconnected channels.  We would need to register with
> > the hwmon subsystem only after getting enough post-initialization status
> > reports for all channels, and this would essentially mean to sleep for
> > 6+ seconds.  We would also need to unregister and re-register when going
> > through a suspend-reset_resume cycle, because the device may have its
> > state wiped, requiring reinitialization.[2]
> > 
I think the above should resolve that.

> > A different approach to handle this, which I have preferred _so far,_ is
> > to use pwm*_enable = 0 to report the unconnected channels to user-space,
> > while keeping the other pwm attributes visible.  But this comes with
> > other problems.
> > 
> > First, lm-sensors' pwmconfig expects to be able to write to a
> > pwm*_enable attribute if it exists, but the device does not support that
> > operation.  The hwmon documentation states that RW values may be RO, but
> > pwmconfig is out there and in use.  So far I simply return 0 to attempts
> > at those writes, silently ignoring them; functional, but certainly a
> > hack.

It is a bad idea to return 0 if the value is not accepted. You could check
if the written value matches the current value and return 0 if it does,
and an error such as -EOPNOTSUPP or -EINVAL otherwise.

> > 
> > Second, if PWM control is disabled for a channel, but its pwm* and
> > pwm*_mode attributes are still visible, what should we return when
> > user-space attempts to write to them?  The practical answer may simply
> > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> > (of handling these cases with pwm*_enable instead of is_visible()) is
> > not doomed.
> > 
Mode isn't really writeable either, isn't it ? If so, use the same trick as
with the _enable attribute.

The same is effectively true for the pwm value itself: Since both _enable
and _mode are effectively read-only, you can accept a write only if
fan control is enabled, and return an error if it isn't.

> > A final minor problem is that channels detected as unconnected run at
> > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> > mine).

Just document the difference. Reality doesn't always match our expectations.

Thanks,
Guenter

> > 
> > Do you have any suggestions and/or recommendations?
> > 
> > If it helps, a pre-RFC (but functional and mostly clean) version of the
> > driver can be found at:
> > 
> > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> > 
> > Thanks,
> > Jonas
> > 
> > [1] According to Documentation/hwmon/sysfs-interface.rst.
> > [2] The device also does not respond to HID Get_Report, so it is not
> >     trivial to check whether it really needs to be reinitialized, since
> >     the only symptom of that being necessary is the absence of the
> >     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

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:

# pwmconfig
[...]
Found the following PWM controls:
cat: hwmon0/pwm1: No data available
   hwmon0/pwm1           current value: 
cat: hwmon0/pwm1: No data available
/bin/pwmconfig: line 201: [: : integer expression expected
cat: hwmon0/pwm1: No data available
hwmon0/pwm1 stuck to 
Manual control mode not supported, skipping hwmon0/pwm1.
[...]

# fancontrol
[...]
Enabling PWM on fans...
cat: hwmon0/pwm1: No data available
Starting automatic fan control...
/bin/fancontrol: line 551: read: read error: 0: No data available
Error reading PWM value from /sys/class/hwmon/hwmon0/pwm1
Aborting, restoring fans...
cat: hwmon0/pwm1: No data available
/bin/fancontrol: line 458: [: : integer expression expected
hwmon0/pwm1_enable stuck to 1
Verify fans have returned to full speed

> 
> > > 
> > > The other starts with PWM control being disabled for channels that the
> > > device identifies as unconnected.  This is not in itself a problem, but
> > > the initialization routine (where the detection happens) is
> > > asynchronous, takes somewhere around 5 seconds, and we do not have any
> > > way of directly querying its result.  We only know the control mode of
> > > each channel (be it DC, PWM or disabled) from the regular status
> > > reports.
> 
> Again, I think the best solution is to return -ENODATA until the value
> is known.

Ok.

> 
> > > 
> > > These limitations make it complicated to simply use is_visible() to hide
> > > pwm attributes of unconnected channels.  We would need to register with
> > > the hwmon subsystem only after getting enough post-initialization status
> > > reports for all channels, and this would essentially mean to sleep for
> > > 6+ seconds.  We would also need to unregister and re-register when going
> > > through a suspend-reset_resume cycle, because the device may have its
> > > state wiped, requiring reinitialization.[2]
> > > 
> I think the above should resolve that.

Yes, as well as your suggestion bellow.

> 
> > > A different approach to handle this, which I have preferred _so far,_ is
> > > to use pwm*_enable = 0 to report the unconnected channels to user-space,
> > > while keeping the other pwm attributes visible.  But this comes with
> > > other problems.
> > > 
> > > First, lm-sensors' pwmconfig expects to be able to write to a
> > > pwm*_enable attribute if it exists, but the device does not support that
> > > operation.  The hwmon documentation states that RW values may be RO, but
> > > pwmconfig is out there and in use.  So far I simply return 0 to attempts
> > > at those writes, silently ignoring them; functional, but certainly a
> > > hack.
> 
> It is a bad idea to return 0 if the value is not accepted. You could check
> if the written value matches the current value and return 0 if it does,
> and an error such as -EOPNOTSUPP or -EINVAL otherwise.

It worked really well, thanks!

> 
> > > 
> > > Second, if PWM control is disabled for a channel, but its pwm* and
> > > pwm*_mode attributes are still visible, what should we return when
> > > user-space attempts to write to them?  The practical answer may simply
> > > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> > > (of handling these cases with pwm*_enable instead of is_visible()) is
> > > not doomed.
> > > 
> Mode isn't really writeable either, isn't it ? If so, use the same trick as
> with the _enable attribute.

You're right, but setting its visibility to 0444 didn't cause issues for
pwmconfig or fancontrol, so I don't think the trick is necessary here.

> 
> The same is effectively true for the pwm value itself: Since both _enable
> and _mode are effectively read-only, you can accept a write only if
> fan control is enabled, and return an error if it isn't.

Ok.

> 
> > > A final minor problem is that channels detected as unconnected run at
> > > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> > > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> > > mine).
> 
> Just document the difference. Reality doesn't always match our expectations.

Ok.

Thanks,
Jonas

> 
> Thanks,
> Guenter
> 
> > > 
> > > Do you have any suggestions and/or recommendations?
> > > 
> > > If it helps, a pre-RFC (but functional and mostly clean) version of the
> > > driver can be found at:
> > > 
> > > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> > > 
> > > Thanks,
> > > Jonas
> > > 
> > > [1] According to Documentation/hwmon/sysfs-interface.rst.
> > > [2] The device also does not respond to HID Get_Report, so it is not
> > >     trivial to check whether it really needs to be reinitialized, since
> > >     the only symptom of that being necessary is the absence of the
> > >     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

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.

s/appropriate/appreciate/

> 
> 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:
> 
> # pwmconfig
> [...]
> Found the following PWM controls:
> cat: hwmon0/pwm1: No data available
>    hwmon0/pwm1           current value: 
> cat: hwmon0/pwm1: No data available
> /bin/pwmconfig: line 201: [: : integer expression expected
> cat: hwmon0/pwm1: No data available
> hwmon0/pwm1 stuck to 
> Manual control mode not supported, skipping hwmon0/pwm1.
> [...]
> 
> # fancontrol
> [...]
> Enabling PWM on fans...
> cat: hwmon0/pwm1: No data available
> Starting automatic fan control...
> /bin/fancontrol: line 551: read: read error: 0: No data available
> Error reading PWM value from /sys/class/hwmon/hwmon0/pwm1
> Aborting, restoring fans...
> cat: hwmon0/pwm1: No data available
> /bin/fancontrol: line 458: [: : integer expression expected
> hwmon0/pwm1_enable stuck to 1
> Verify fans have returned to full speed
> 
> > 
> > > > 
> > > > The other starts with PWM control being disabled for channels that the
> > > > device identifies as unconnected.  This is not in itself a problem, but
> > > > the initialization routine (where the detection happens) is
> > > > asynchronous, takes somewhere around 5 seconds, and we do not have any
> > > > way of directly querying its result.  We only know the control mode of
> > > > each channel (be it DC, PWM or disabled) from the regular status
> > > > reports.
> > 
> > Again, I think the best solution is to return -ENODATA until the value
> > is known.
> 
> Ok.
> 
> > 
> > > > 
> > > > These limitations make it complicated to simply use is_visible() to hide
> > > > pwm attributes of unconnected channels.  We would need to register with
> > > > the hwmon subsystem only after getting enough post-initialization status
> > > > reports for all channels, and this would essentially mean to sleep for
> > > > 6+ seconds.  We would also need to unregister and re-register when going
> > > > through a suspend-reset_resume cycle, because the device may have its
> > > > state wiped, requiring reinitialization.[2]
> > > > 
> > I think the above should resolve that.
> 
> Yes, as well as your suggestion bellow.
> 
> > 
> > > > A different approach to handle this, which I have preferred _so far,_ is
> > > > to use pwm*_enable = 0 to report the unconnected channels to user-space,
> > > > while keeping the other pwm attributes visible.  But this comes with
> > > > other problems.
> > > > 
> > > > First, lm-sensors' pwmconfig expects to be able to write to a
> > > > pwm*_enable attribute if it exists, but the device does not support that
> > > > operation.  The hwmon documentation states that RW values may be RO, but
> > > > pwmconfig is out there and in use.  So far I simply return 0 to attempts
> > > > at those writes, silently ignoring them; functional, but certainly a
> > > > hack.
> > 
> > It is a bad idea to return 0 if the value is not accepted. You could check
> > if the written value matches the current value and return 0 if it does,
> > and an error such as -EOPNOTSUPP or -EINVAL otherwise.
> 
> It worked really well, thanks!
> 
> > 
> > > > 
> > > > Second, if PWM control is disabled for a channel, but its pwm* and
> > > > pwm*_mode attributes are still visible, what should we return when
> > > > user-space attempts to write to them?  The practical answer may simply
> > > > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> > > > (of handling these cases with pwm*_enable instead of is_visible()) is
> > > > not doomed.
> > > > 
> > Mode isn't really writeable either, isn't it ? If so, use the same trick as
> > with the _enable attribute.
> 
> You're right, but setting its visibility to 0444 didn't cause issues for
> pwmconfig or fancontrol, so I don't think the trick is necessary here.
> 
> > 
> > The same is effectively true for the pwm value itself: Since both _enable
> > and _mode are effectively read-only, you can accept a write only if
> > fan control is enabled, and return an error if it isn't.
> 
> Ok.
> 
> > 
> > > > A final minor problem is that channels detected as unconnected run at
> > > > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> > > > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> > > > mine).
> > 
> > Just document the difference. Reality doesn't always match our expectations.
> 
> Ok.
> 
> Thanks,
> Jonas
> 
> > 
> > Thanks,
> > Guenter
> > 
> > > > 
> > > > Do you have any suggestions and/or recommendations?
> > > > 
> > > > If it helps, a pre-RFC (but functional and mostly clean) version of the
> > > > driver can be found at:
> > > > 
> > > > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> > > > 
> > > > Thanks,
> > > > Jonas
> > > > 
> > > > [1] According to Documentation/hwmon/sysfs-interface.rst.
> > > > [2] The device also does not respond to HID Get_Report, so it is not
> > > >     trivial to check whether it really needs to be reinitialized, since
> > > >     the only symptom of that being necessary is the absence of the
> > > >     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Guenter Roeck]

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.

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.

Guenter

> # pwmconfig
> [...]
> Found the following PWM controls:
> cat: hwmon0/pwm1: No data available
>    hwmon0/pwm1           current value: 
> cat: hwmon0/pwm1: No data available
> /bin/pwmconfig: line 201: [: : integer expression expected
> cat: hwmon0/pwm1: No data available
> hwmon0/pwm1 stuck to 
> Manual control mode not supported, skipping hwmon0/pwm1.
> [...]
> 
> # fancontrol
> [...]
> Enabling PWM on fans...
> cat: hwmon0/pwm1: No data available
> Starting automatic fan control...
> /bin/fancontrol: line 551: read: read error: 0: No data available
> Error reading PWM value from /sys/class/hwmon/hwmon0/pwm1
> Aborting, restoring fans...
> cat: hwmon0/pwm1: No data available
> /bin/fancontrol: line 458: [: : integer expression expected
> hwmon0/pwm1_enable stuck to 1
> Verify fans have returned to full speed
> 
> > 
> > > > 
> > > > The other starts with PWM control being disabled for channels that the
> > > > device identifies as unconnected.  This is not in itself a problem, but
> > > > the initialization routine (where the detection happens) is
> > > > asynchronous, takes somewhere around 5 seconds, and we do not have any
> > > > way of directly querying its result.  We only know the control mode of
> > > > each channel (be it DC, PWM or disabled) from the regular status
> > > > reports.
> > 
> > Again, I think the best solution is to return -ENODATA until the value
> > is known.
> 
> Ok.
> 
> > 
> > > > 
> > > > These limitations make it complicated to simply use is_visible() to hide
> > > > pwm attributes of unconnected channels.  We would need to register with
> > > > the hwmon subsystem only after getting enough post-initialization status
> > > > reports for all channels, and this would essentially mean to sleep for
> > > > 6+ seconds.  We would also need to unregister and re-register when going
> > > > through a suspend-reset_resume cycle, because the device may have its
> > > > state wiped, requiring reinitialization.[2]
> > > > 
> > I think the above should resolve that.
> 
> Yes, as well as your suggestion bellow.
> 
> > 
> > > > A different approach to handle this, which I have preferred _so far,_ is
> > > > to use pwm*_enable = 0 to report the unconnected channels to user-space,
> > > > while keeping the other pwm attributes visible.  But this comes with
> > > > other problems.
> > > > 
> > > > First, lm-sensors' pwmconfig expects to be able to write to a
> > > > pwm*_enable attribute if it exists, but the device does not support that
> > > > operation.  The hwmon documentation states that RW values may be RO, but
> > > > pwmconfig is out there and in use.  So far I simply return 0 to attempts
> > > > at those writes, silently ignoring them; functional, but certainly a
> > > > hack.
> > 
> > It is a bad idea to return 0 if the value is not accepted. You could check
> > if the written value matches the current value and return 0 if it does,
> > and an error such as -EOPNOTSUPP or -EINVAL otherwise.
> 
> It worked really well, thanks!
> 
> > 
> > > > 
> > > > Second, if PWM control is disabled for a channel, but its pwm* and
> > > > pwm*_mode attributes are still visible, what should we return when
> > > > user-space attempts to write to them?  The practical answer may simply
> > > > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> > > > (of handling these cases with pwm*_enable instead of is_visible()) is
> > > > not doomed.
> > > > 
> > Mode isn't really writeable either, isn't it ? If so, use the same trick as
> > with the _enable attribute.
> 
> You're right, but setting its visibility to 0444 didn't cause issues for
> pwmconfig or fancontrol, so I don't think the trick is necessary here.
> 
> > 
> > The same is effectively true for the pwm value itself: Since both _enable
> > and _mode are effectively read-only, you can accept a write only if
> > fan control is enabled, and return an error if it isn't.
> 
> Ok.
> 
> > 
> > > > A final minor problem is that channels detected as unconnected run at
> > > > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> > > > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> > > > mine).
> > 
> > Just document the difference. Reality doesn't always match our expectations.
> 
> Ok.
> 
> Thanks,
> Jonas
> 
> > 
> > Thanks,
> > Guenter
> > 
> > > > 
> > > > Do you have any suggestions and/or recommendations?
> > > > 
> > > > If it helps, a pre-RFC (but functional and mostly clean) version of the
> > > > driver can be found at:
> > > > 
> > > > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> > > > 
> > > > Thanks,
> > > > Jonas
> > > > 
> > > > [1] According to Documentation/hwmon/sysfs-interface.rst.
> > > > [2] The device also does not respond to HID Get_Report, so it is not
> > > >     trivial to check whether it really needs to be reinitialized, since
> > > >     the only symptom of that being necessary is the absence of the
> > > >     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

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?

Thanks,
Jonas

> 
> Guenter
> 
> > # pwmconfig
> > [...]
> > Found the following PWM controls:
> > cat: hwmon0/pwm1: No data available
> >    hwmon0/pwm1           current value: 
> > cat: hwmon0/pwm1: No data available
> > /bin/pwmconfig: line 201: [: : integer expression expected
> > cat: hwmon0/pwm1: No data available
> > hwmon0/pwm1 stuck to 
> > Manual control mode not supported, skipping hwmon0/pwm1.
> > [...]
> > 
> > # fancontrol
> > [...]
> > Enabling PWM on fans...
> > cat: hwmon0/pwm1: No data available
> > Starting automatic fan control...
> > /bin/fancontrol: line 551: read: read error: 0: No data available
> > Error reading PWM value from /sys/class/hwmon/hwmon0/pwm1
> > Aborting, restoring fans...
> > cat: hwmon0/pwm1: No data available
> > /bin/fancontrol: line 458: [: : integer expression expected
> > hwmon0/pwm1_enable stuck to 1
> > Verify fans have returned to full speed
> > 
> > > 
> > > > > 
> > > > > The other starts with PWM control being disabled for channels that the
> > > > > device identifies as unconnected.  This is not in itself a problem, but
> > > > > the initialization routine (where the detection happens) is
> > > > > asynchronous, takes somewhere around 5 seconds, and we do not have any
> > > > > way of directly querying its result.  We only know the control mode of
> > > > > each channel (be it DC, PWM or disabled) from the regular status
> > > > > reports.
> > > 
> > > Again, I think the best solution is to return -ENODATA until the value
> > > is known.
> > 
> > Ok.
> > 
> > > 
> > > > > 
> > > > > These limitations make it complicated to simply use is_visible() to hide
> > > > > pwm attributes of unconnected channels.  We would need to register with
> > > > > the hwmon subsystem only after getting enough post-initialization status
> > > > > reports for all channels, and this would essentially mean to sleep for
> > > > > 6+ seconds.  We would also need to unregister and re-register when going
> > > > > through a suspend-reset_resume cycle, because the device may have its
> > > > > state wiped, requiring reinitialization.[2]
> > > > > 
> > > I think the above should resolve that.
> > 
> > Yes, as well as your suggestion bellow.
> > 
> > > 
> > > > > A different approach to handle this, which I have preferred _so far,_ is
> > > > > to use pwm*_enable = 0 to report the unconnected channels to user-space,
> > > > > while keeping the other pwm attributes visible.  But this comes with
> > > > > other problems.
> > > > > 
> > > > > First, lm-sensors' pwmconfig expects to be able to write to a
> > > > > pwm*_enable attribute if it exists, but the device does not support that
> > > > > operation.  The hwmon documentation states that RW values may be RO, but
> > > > > pwmconfig is out there and in use.  So far I simply return 0 to attempts
> > > > > at those writes, silently ignoring them; functional, but certainly a
> > > > > hack.
> > > 
> > > It is a bad idea to return 0 if the value is not accepted. You could check
> > > if the written value matches the current value and return 0 if it does,
> > > and an error such as -EOPNOTSUPP or -EINVAL otherwise.
> > 
> > It worked really well, thanks!
> > 
> > > 
> > > > > 
> > > > > Second, if PWM control is disabled for a channel, but its pwm* and
> > > > > pwm*_mode attributes are still visible, what should we return when
> > > > > user-space attempts to write to them?  The practical answer may simply
> > > > > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach
> > > > > (of handling these cases with pwm*_enable instead of is_visible()) is
> > > > > not doomed.
> > > > > 
> > > Mode isn't really writeable either, isn't it ? If so, use the same trick as
> > > with the _enable attribute.
> > 
> > You're right, but setting its visibility to 0444 didn't cause issues for
> > pwmconfig or fancontrol, so I don't think the trick is necessary here.
> > 
> > > 
> > > The same is effectively true for the pwm value itself: Since both _enable
> > > and _mode are effectively read-only, you can accept a write only if
> > > fan control is enabled, and return an error if it isn't.
> > 
> > Ok.
> > 
> > > 
> > > > > A final minor problem is that channels detected as unconnected run at
> > > > > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too
> > > > > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis
> > > > > mine).
> > > 
> > > Just document the difference. Reality doesn't always match our expectations.
> > 
> > Ok.
> > 
> > Thanks,
> > Jonas
> > 
> > > 
> > > Thanks,
> > > Guenter
> > > 
> > > > > 
> > > > > Do you have any suggestions and/or recommendations?
> > > > > 
> > > > > If it helps, a pre-RFC (but functional and mostly clean) version of the
> > > > > driver can be found at:
> > > > > 
> > > > > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c
> > > > > 
> > > > > Thanks,
> > > > > Jonas
> > > > > 
> > > > > [1] According to Documentation/hwmon/sysfs-interface.rst.
> > > > > [2] The device also does not respond to HID Get_Report, so it is not
> > > > >     trivial to check whether it really needs to be reinitialized, since
> > > > >     the only symptom of that being necessary is the absence of the
> > > > >     asynchronous status reports.

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Guenter Roeck]

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

Re: PWM control in NZXT Grid+ V3 and Smart Device

[Jonas Malaco]

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