Re: [PATCH v2] hwmon: add POWER-Z driver
From: Guenter Roeck
Date: Thu Aug 31 2023 - 13:41:29 EST
On 8/31/23 02:58, Thomas Weißschuh wrote:
POWER-Z is a series of devices to monitor power characteristics of USB-C
connections and display those on a on-device display.
Some of the devices, notably KM002C and KM003C, contain an additional
port which exposes the measurements via USB.
This is a driver for this monitor port.
It was developed and tested with the KM003C.
Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
---
Changes in v2:
- fix syntax error in Kconfig
- avoid double free of urb in case of failure
- Link to v1: https://lore.kernel.org/r/20230831-powerz-v1-1-03979e519f52@xxxxxxxxxxxxxx
---
MAINTAINERS | 6 ++
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/powerz.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++
Documentation missing.
4 files changed, 274 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 4e3419c04f27..12bcf14597c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4795,6 +4795,12 @@ X: drivers/char/ipmi/
X: drivers/char/random.c
X: drivers/char/tpm/
+CHARGERLAB POWER-Z HARDWARE MONITOR DRIVER
+M: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
+L: linux-hwmon@xxxxxxxxxxxxxxx
+S: Maintained
+F: drivers/hwmon/powerz.c
+
CHECKPATCH
M: Andy Whitcroft <apw@xxxxxxxxxxxxx>
M: Joe Perches <joe@xxxxxxxxxxx>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ec38c8892158..12af9f9cfd9f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -839,6 +839,16 @@ config SENSORS_JC42
This driver can also be built as a module. If so, the module
will be called jc42.
+config SENSORS_POWERZ
+ tristate "ChargerLAB POWER-Z USB-C tester"
+ depends on USB
+ help
+ If you say yes here you get support for ChargerLAB POWER-Z series of
+ USB-C charging testers.
+
+ This driver can also be built as a module. If so, the module
+ will be called powerz.
+
config SENSORS_POWR1220
tristate "Lattice POWR1220 Power Monitoring"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4ac9452b5430..019189500e5d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -176,6 +176,7 @@ obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
+obj-$(CONFIG_SENSORS_POWERZ) += powerz.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
new file mode 100644
index 000000000000..cbdbc1ce0e81
--- /dev/null
+++ b/drivers/hwmon/powerz.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/completion.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/usb.h>
Various include files missing. I did not check all of them, but for example
umode_t and various other types are defined in include/linux/types.h,
and struct device is declared in include/linux/device.h, both of which
should therefore be included.
The idea is not to include as little as possible, but to include everything
that is referenced in the driver. There should be no ddependency on indirect
includes.
+
+#define DRIVER_NAME "powerz"
+#define POWERZ_EP_CMD_OUT 0x01
+#define POWERZ_EP_DATA_IN 0x81
+
+struct powerz_sensor_data {
+ u8 _unknown_1[8];
+ __le32 Vbus;
CHECK: Avoid CamelCase: <Vbus>
#160: FILE: drivers/hwmon/powerz.c:18:
+ __le32 Vbus;
Please run your patches through checkpatch --strict.
+ __le32 Ibus;
+ __le32 Vbus_avg;
+ __le32 Ibus_avg;
+ u8 _unknown_2[8];
+ u8 temp[2];
+ __le16 cc1;
+ __le16 cc2;
+ __le16 dp;
+ __le16 dm;
+ u8 _unknown_3[6];
+} __packed;
+
+struct powerz_priv {
+ struct device *hwmon_dev;
+ struct usb_interface *intf;
+};
+
+static const struct hwmon_channel_info * const powerz_info[] = {
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_AVERAGE,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL),
+ HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_AVERAGE),
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
+ NULL
+};
+
+static umode_t powerz_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return 0444;
+}
+
+static int powerz_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ if (type == hwmon_curr && attr == hwmon_curr_label)
+ *str = "bus";
+ else if (type == hwmon_in && attr == hwmon_in_label && channel == 0)
+ *str = "bus";
+ else if (type == hwmon_in && attr == hwmon_in_label && channel == 1)
+ *str = "cc1";
+ else if (type == hwmon_in && attr == hwmon_in_label && channel == 2)
+ *str = "cc2";
+ else if (type == hwmon_in && attr == hwmon_in_label && channel == 3)
+ *str = "dp";
+ else if (type == hwmon_in && attr == hwmon_in_label && channel == 4)
+ *str = "dm";
+ else if (type == hwmon_temp && attr == hwmon_temp_label)
+ *str = "temp";
+ else
+ return -EINVAL;
+
-EOPNOTSUPP, and please use nested if statement to avoid checking the same
value several times.
+ return 0;
+}
+
+struct powerz_usb_ctx {
+ char transfer_buffer[64];
+ struct completion completion;
+ int status;
+};
+
+static void powerz_usb_data_complete(struct urb *urb)
+{
+ struct powerz_usb_ctx *ctx = urb->context;
+
+ ctx->status = 0;
+ complete(&ctx->completion);
+}
+
+static void powerz_usb_cmd_complete(struct urb *urb)
+{
+ struct powerz_usb_ctx *ctx = urb->context;
+ int ret;
+
+ usb_fill_bulk_urb(urb, urb->dev, usb_rcvbulkpipe(urb->dev, POWERZ_EP_DATA_IN),
+ ctx->transfer_buffer, sizeof(ctx->transfer_buffer),
+ powerz_usb_data_complete, ctx);
+
+ ret = usb_submit_urb(urb, GFP_ATOMIC);
+ if (ret) {
+ ctx->status = ret;
+ complete(&ctx->completion);
+ }
+}
+
+static struct powerz_sensor_data *powerz_read_data(struct usb_device *udev,
+ struct powerz_usb_ctx *ctx)
+{
+ struct urb *urb;
+ int r;
+
+ ctx->status = -ETIMEDOUT;
+ init_completion(&ctx->completion);
+
+ urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!urb)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->transfer_buffer[0] = 0x0c;
+ ctx->transfer_buffer[1] = 0x00;
+ ctx->transfer_buffer[2] = 0x02;
+ ctx->transfer_buffer[3] = 0x00;
+
+ usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, POWERZ_EP_CMD_OUT),
+ ctx->transfer_buffer, 4, powerz_usb_cmd_complete, ctx);
+ r = usb_submit_urb(urb, GFP_KERNEL);
+ if (r) {
+ usb_free_urb(urb);
+ return ERR_PTR(r);
+ }
+
+ if (!wait_for_completion_interruptible_timeout(&ctx->completion, msecs_to_jiffies(5))) {
+ usb_kill_urb(urb);
+ usb_free_urb(urb);
+ return ERR_PTR(-EIO);
+ }
+
+ if (ctx->status) {
+ usb_free_urb(urb);
+ return ERR_PTR(ctx->status);
+ }
+
+ if (urb->actual_length < (sizeof(struct powerz_sensor_data))) {
+ usb_free_urb(urb);
+ return ERR_PTR(-EIO);
+ }
+
+ usb_free_urb(urb);
+ return (struct powerz_sensor_data *)(ctx->transfer_buffer);
The return pointer is always either an error or ctx->transfer_buffer,
which is known by the caller. It would be much easier to just return
an integer error code and let the caller access ctx->transfer_buffer.
With that, the code can be simplified to have something like
if (error condition) {
ret = error contition;
goto error;
...
error:
usb_free_urb(urb);
return ret;
which more follows kernel coding style and avoids the repeated
call to usb_free_urb().
+}
+
+static int powerz_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct usb_interface *intf = to_usb_interface(dev->parent);
+ struct usb_device *udev = interface_to_usbdev(intf);
+ struct powerz_sensor_data *data;
+ struct powerz_usb_ctx *ctx;
+
+ ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
I think it would be much better to allocate ctx once as part of
struct powerz_priv and keep it around. Sure, that means that this
function requires a lock, but I don't see that as problem (and who
knows how the device reacts to multiple pending usb transactions).
You'd need to allocate transfer_buffer separately because it needs to be
dma aligned, but that should not be a problem either.
+ data = powerz_read_data(udev, ctx);
+ if (IS_ERR(data)) {
+ kfree(ctx);
+ return PTR_ERR(data);
+ }
With the above, this could be
ret = powerz_read_data(udev, ctx);
if (ret)
return ret;
data = ctx->transfer_buffer;
(with ctx replaced to use struct powerz_priv).
+
+ if (type == hwmon_curr && attr == hwmon_curr_input)
+ *val = ((s32)le32_to_cpu(data->Ibus)) / 1000;
+ else if (type == hwmon_curr && attr == hwmon_curr_average)
+ *val = ((s32)le32_to_cpu(data->Ibus_avg)) / 1000;
+ else if (type == hwmon_in && attr == hwmon_in_input && channel == 0)
+ *val = le32_to_cpu(data->Vbus) / 1000;
+ else if (type == hwmon_in && attr == hwmon_in_average && channel == 0)
+ *val = le32_to_cpu(data->Vbus_avg) / 1000;
+ else if (type == hwmon_in && attr == hwmon_in_input && channel == 1)
+ *val = le16_to_cpu(data->cc1) / 10;
+ else if (type == hwmon_in && attr == hwmon_in_input && channel == 2)
+ *val = le16_to_cpu(data->cc2) / 10;
+ else if (type == hwmon_in && attr == hwmon_in_input && channel == 3)
+ *val = le16_to_cpu(data->dp) / 10;
+ else if (type == hwmon_in && attr == hwmon_in_input && channel == 4)
+ *val = le16_to_cpu(data->dm) / 10;
+ else if (type == hwmon_temp && attr == hwmon_temp_input)
+ *val = ((long)data->temp[1]) * 2000 + ((long)data->temp[0]) * 1000 / 128;
+ else
+ return -EINVAL;
-EOPNOTSUPP.
Also, please use nested if statements to avoid checking the same
value several times.
+
+ kfree(ctx);
+
+ return 0;
+}
+
+static const struct hwmon_ops powerz_hwmon_ops = {
+ .is_visible = powerz_is_visible,
+ .read = powerz_read,
+ .read_string = powerz_read_string,
+};
+
+static const struct hwmon_chip_info powerz_chip_info = {
+ .ops = &powerz_hwmon_ops,
+ .info = powerz_info,
+};
+
+static int powerz_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+ struct usb_device *udev = interface_to_usbdev(intf);
+ struct powerz_priv *priv;
+ struct device *parent;
+ const char *name;
+ int ret;
+
+ parent = &intf->dev;
+
+ priv = devm_kzalloc(parent, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ name = devm_hwmon_sanitize_name(parent, udev->product ?: DRIVER_NAME);
Why not just use DRIVER_NAME ? This would be much more consistent.
+ priv->hwmon_dev = hwmon_device_register_with_info(parent, name,
+ priv,
+ &powerz_chip_info,
+ NULL);
+ ret = PTR_ERR_OR_ZERO(priv->hwmon_dev);
+ priv->intf = intf;
Maybe I am missing something, but I don't see this used anywhere.
+ usb_set_intfdata(intf, priv);
+
+ return ret;
+}
+
+static void powerz_disconnect(struct usb_interface *intf)
+{
+ struct powerz_priv *priv = usb_get_intfdata(intf);
+
+ hwmon_device_unregister(priv->hwmon_dev);
Please use devm_hwmon_device_register_with_info() to register the device.
+}
+
+static const struct usb_device_id powerz_id_table[] = {
+ { USB_DEVICE_INTERFACE_NUMBER(0x5FC9, 0x0063, 0x00) }, /* ChargerLAB POWER-Z KM003C */
+ { }
+};
+MODULE_DEVICE_TABLE(usb, powerz_id_table);
+
+static struct usb_driver powerz_driver = {
+ .name = DRIVER_NAME,
+ .id_table = powerz_id_table,
+ .probe = powerz_probe,
+ .disconnect = powerz_disconnect,
+};
+module_usb_driver(powerz_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Thomas Weißschuh <linux@xxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("ChargerLAB POWER-Z USB-C tester");
---
base-commit: b97d64c722598ffed42ece814a2cb791336c6679
change-id: 20230831-powerz-2ccb978a8e57
Best regards,