Re: [PATCH 0/4] hwmon: Add WITRN USB tester driver

[Guenter Roeck]

On 3/27/26 05:01, Rong Zhang wrote:
> Hi Guenter,
>
> Thanks a lot for your review and applying patch 1 :-)
>
> On Thu, 2026-03-26 at 17:05 -0700, Guenter Roeck wrote:
> > On 3/26/26 12:19, Rong Zhang wrote:
> > > WITRN produces a series of devices to monitor power characteristics of
> > > USB connections and display those on a on-device display. Most of them
> > > contain an additional port which exposes the measurements via USB HID.
> > >
> > > These devices report sensor values in IEEE-754 float (binary32) format.
> > > The driver must perform floating-point number to integer conversions to
> > > provide hwmon channels. Meanwhile, they also report accumulative float
> > > values, and simple division or multiplication turns them into useful
> > > hwmon channels.
> > >
> > > Patch 1 adds label support for 64-bit energy attributes, as the driver
> > > needs it.
> > >
> > > Patch 2 adds a helper module for floating-point to integer conversions,
> > > so that the conversion, multification and division methods can be used
> > > in this driver as well as other drivers (I am also working on another
> > > USB tester driver that needs it).
> > >
> > > Patch 3 adds a barebone HID driver for WITRN K2.
> > >
> > > Patch 4 adds hwmon channels and attributes to the driver.
> > >
> > > Signed-off-by: Rong Zhang <i@xxxxxxxx>
> > > ---
> > > Rong Zhang (4):
> > >         hwmon: Add label support for 64-bit energy attributes
> > >         hwmon: New helper module for floating-point to integer conversions
> >
> > Nack. This is not a hwmon problem and should reside in a driver or in lib/
> > (if it is needed by multiple drivers).
>
> Makes sense. I will try.
>
> > >         hwmon: Add barebone HID driver for WITRN
> >
> > Nack. This is the wrong place for such a driver. It should reside somewhere
> > in drivers/usb, or maybe in drivers/misc/.
>
> Hmm, I decided to place it here because:
>
> - It's not a hid_ll_driver but a dumb hid_driver, i.e., does no low
> level things but just receives hid event from the HID core. It doesn't
> even send any HID report to the device.
>
> - There has been numerous hid_driver in drivers/hwmon/.
>
> - There has been a similar USB tester driver in drivers/hwmon/, i.e.,
> powerz. That's the major reason of my decision.

powerz is a pure hwmon driver, nothing else. It does not claim to be a
"pure hid driver". If your driver _only_ provides a hwmon ABI, it would
be acceptable. But then this and the next patch should be one patch,
and it should only provide the hwmon ABI, nothing else (except maybe
debugfs entries or sysfs entries attached directly to the HID device
to display information such as serial number etc). Reading ROM/RAM addresses,
as mentioned below, would be outside that scope.

The entire powerz driver is 269 lines of code. Your driver has well above
1,000 LOC. Your code has separate source files plus an include file.
That suggests that it does more than just reporting hardware monitoring
attributes.

I have not looked further into the code itself. My response is based purely
on the subjects and code organization, which suggests that this is a HID
driver with attached hardware monitoring.

I am not sure I understand what all that would have to do with UCSI. UCSI
support is implemented in drivers/usb/typec/ucsi. Anything associated
with that protocol should be implemented there if it is part of the
protocol.

> Could you kindly explain what kinds of driver can be accepted into
> drivers/hwmon/?
>
> > >         hwmon: (witrn) Add monitoring support
> >
> > This should be implemented as auxiliary driver.
>
> Could you kindly elaborate? Did you mean that if the device supports
> multiple functionalities they should be implemented as multiple
> auxiliary drivers in different subsystems?

Correct. Your series suggests that this would be the case.

> FYI, the USB tester doesn't provide any other meaningful feature that
> fits other subsystems. The tester only provides two features through USB
> HID: power measurements (this series), and raw PD packets sniffing.

Again, support for raw PD packets sniffing would be outside the scope
of the hardware monitoring subsystem.

Thanks,
Guenter

> As for the latter, the USB tester can sniff raw PD packets between the
> source and sink if enabled in the device menu. It doesn't provide the
> parsed result for packets, and the PC cannot ask the tester to send PD
> packets. This doesn't fit UCSI at all, as a UCSI device operates at a
> higher level and must accept commands. AFAIK such a dumb sniffer won't
> fit any subsystem in the kernel. Hence, the only thing fits a subsystem
> is its power measurements.
>
> All measurements supported by the official utility for Windows can be
> found in `struct witrn_sensor'. Other than that, all extra features
> provided by the utility are implemented in software and I didn't see any
> extra USB packets other than querying the serial number [1] when I was
> randomly messing around with the utility [2].
>
> I separated patch 3 and 4 just for easier review. If you are not in
> favor of such a style, I will squash them.
>
> [1]: In fact, the utility directly asks the device to return the content
> on several specific ROM/RAM addresses, and then the utility calculates
> the serial number with an unknown algorithm. Reading a ROM/RAM address
> seems to be the only command that the device accepts from the USB host.
>
> [2]: Yeah, their utility does not support PD packet capturing or
> parsing. It seems that the manufacturer provides the PD sniffing feature
> as is and expects users to capture it via hidraw or libusb and parse it
> themselves.
>
> > Sashiko has a lot of feedback that you might want to address before
> > resubmitting.
> >
> > https://sashiko.dev/#/patchset/20260327-b4-hwmon-witrn-v1-0-8d2f1896c045%40rong.moe
>
> Sashiko's feedback helps a lot. Thanks.
>
> Thanks,
> Rong
>
> > Thanks,
> > Guenter
> >
> > >    Documentation/hwmon/index.rst |   1 +
> > >    Documentation/hwmon/witrn.rst |  53 ++++
> > >    MAINTAINERS                   |   7 +
> > >    drivers/hwmon/Kconfig         |  14 +
> > >    drivers/hwmon/Makefile        |   2 +
> > >    drivers/hwmon/hwmon-fp.c      | 262 ++++++++++++++++
> > >    drivers/hwmon/hwmon-fp.h      | 212 +++++++++++++
> > >    drivers/hwmon/hwmon.c         |   1 +
> > >    drivers/hwmon/witrn.c         | 691 ++++++++++++++++++++++++++++++++++++++++++
> > >    9 files changed, 1243 insertions(+)
> > > ---
> > > base-commit: 0138af2472dfdef0d56fc4697416eaa0ff2589bd
> > > change-id: 20260327-b4-hwmon-witrn-a629b9040250
> > >
> > > Thanks,
> > > Rong