lkml.org 
[lkml]   [2023]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] hwmon: add POWER-Z driver
From
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

\
 
 \ /
  Last update: 2023-08-31 22:03    [W:0.066 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site