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.
> [..]
> > +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.
