Messages in this thread | | | Date | Thu, 31 Aug 2023 13:02:32 -0700 | Subject | Re: [PATCH v2] hwmon: add POWER-Z driver | From | Guenter Roeck <> |
| |
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
| |