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

Rong Zhang
2026-03-27T12:07:04Z

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@rong.moe>
> > ---
> > 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.

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?

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.

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