On 8/31/23 11:03, Thomas Weißschuh wrote:
Hi Guenter,
thanks for your review!
Ack to most of your points.
[..]
+
+#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.
I did. Weird that it didn't show. I'll investigate.
(And fix it)
+ __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;
+
[..]
+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.
What is your opinion on making the transfer buffer the first member of
struct powerz_priv? It would simplify the code and still provide a
DMA-capable buffer.
Sure, works for me.
[..]
+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.
I liked the more detailed name better.
But if you prefer otherwise I'll simplify it.
I think it just confuses users because it isn't well defined.
Thanks,
Guenter
