Re: [PATCH 0/4] hwmon: Add WITRN USB tester driver
From: Rong Zhang
Date: Fri Mar 27 2026 - 13:29:47 EST
Hi Guenter,
Thanks for your detailed elaboration. It's very helpful.
On Fri, 2026-03-27 at 08:42 -0700, Guenter Roeck wrote:
> 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).
>
Understood. I will squash the two patches into a single patch, as it
will only provide hwmon ABI.
> Reading ROM/RAM addresses,
> as mentioned below, would be outside that scope.
Thanks for clarification. I am not going to support this, as dancing
with undocumented ROM/RAM access is too dangerous.
>
> The entire powerz driver is 269 lines of code. Your driver has well above
> 1,000 LOC.
>
The witrn driver itself is less than 700 LOC. It's longer because of
more channels and the need to call floating-point conversion and
arithmetic methods.
Other LOC mostly locates at the floating-point conversions and
arithmetic module. Since I will turn it into a generic lib, it won't be
a part of the witrn driver when I resubmit it. Thanks for the
suggestion.
> Your code has separate source files plus an include file.
The include file is for the floating-point conversion and arithmetic
module.
> 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 should have explained the structure of the series more detailedly.
Sorry for causing the misunderstanding.
>
> 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.
Thanks for clarification. I think the feature has nothing to do with
UCSI because it's a passive dumb raw packet sniffer.
>
> > 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.
Sorry for causing the misunderstanding again.
>
> > 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.
Agreed. I am not going to support it in any other subsystem either.
Users should use hidraw and parse everything themselves if they need it.
Thanks,
Rong
>
> 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
> > > >