Re: [PATCH v2] hwmon: add POWER-Z driver

[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