Re: [PATCH 2/3] can/peak_usb: CAN-FD: add new adapters specific files

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 8436 bytes --]

On 12/02/2014 04:08 PM, Stephane Grosjean wrote:
>>>   drivers/net/can/usb/peak_usb/pcan_ucan.h   | 208 +++++++
>>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 955
>>> +++++++++++++++++++++++++++++
>>>   drivers/net/can/usb/peak_usb/pcan_usb_fd.h | 108 ++++
>> Please don't create a header file, if it's only included once. Please
>> merge into the correct .c file.
> 
> Ok. So I merge both header files into the .c file. There will be only a
> single (and big) pcan_usb_fd.c file.

Yes, we only want code in header files that's used in more than one .c file.

>> Please have a look the _all_ multi-byte values and check for endianess.
>>
>> Hint, you might want to fix the existing driver first:
>>
>> pcan_usb_core.c:863:60: warning: restricted __le16 degrades to integer
>>
>> pcan_usb.c:322:34: warning: cast to restricted __le32
>>
>> pcan_usb.c:357:20: warning: cast to restricted __le16
>>
>> pcan_usb.c:382:28: warning: cast to restricted __le16
>>
>> pcan_usb.c:625:30: warning: cast to restricted __le32
>>
>> pcan_usb.c:635:30: warning: cast to restricted __le16
>>
>> pcan_usb_pro.c:158:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:158:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:158:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:168:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:168:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:168:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:176:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:176:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:176:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:182:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:182:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:182:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:184:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:184:28:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:184:28:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:190:28: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:190:28:    expected unsigned short [unsigned] [short]
>> [usertype] <noident>
>> pcan_usb_pro.c:190:28:    got restricted __le16 [usertype] <noident>
>>
>> pcan_usb_pro.c:203:32: warning: incorrect type in assignment (different
>> base types)
>> pcan_usb_pro.c:203:32:    expected unsigned int [unsigned] [usertype]
>> <noident>
>> pcan_usb_pro.c:203:32:    got restricted __le32 [usertype] <noident>
>>
>> pcan_usb_pro.c:286:27: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:458:30: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:550:29: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:561:47: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:575:32: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:605:35: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:606:35: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:678:47: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:696:37: warning: cast to restricted __le32
>>
>> pcan_usb_pro.c:720:19: warning: cast to restricted __le16
>>
>>
>> Compile your kernel with "make C=1 CF=-D__CHECK_ENDIAN__", you neet to
>> have the tool "sparse" install. On debian/ubuntu/... it's: "sudo
>> aptitude install sparse"
> 
> Ok, I'll do it too. And I suppose I'll push these changes in a separate
> patch too...

Yes, as this is a fix this should be done before doing any improvements.

> 
>>>   3 files changed, 1271 insertions(+)
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_ucan.h
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>>   create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_fd.h
>>>
>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_ucan.h
>>> b/drivers/net/can/usb/peak_usb/pcan_ucan.h
>>> new file mode 100644
>>> index 0000000..2223c05
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_ucan.h
[...]

>>> +#define PUCAN_CMD_OPCODE_CHANNEL(c, o)    cpu_to_le16(((c) << 12) |
>>> ((o) & 0x3ff))
>> I'm not sure, but I think I don't like hiding of the cpu_to_le16.
>> Can you make this a static inline function?
> 
> Ok. So I suppose I have to do the same (inline function making) for all
> the other occurrences of "cpu_to_le16()"  in this file, right?

Yes, the function would have a return value of __le16 then.

[...]

>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>> b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>> new file mode 100644
>>> index 0000000..3d392a8
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c

[...]

>>> +/* send PCAN-USB Pro FD commands synchronously */
>>> +static int pcan_usb_fd_send_cmd(struct peak_usb_device *dev, void
>>> *cmd_tail)
>>> +{
>>> +    void *cmd_head = pcan_usb_fd_cmd_buffer(dev);
>>> +    int actual_length;
>>> +    int err, cmd_len;
>> cmd_len is a long (or ptrdiff_t), as it's the diff of two pointers.
> 
> Ok. So it will be a "ptrdiff_t" .
> 
>>
>>> +    u8 *packet_ptr;
>>> +    int p, n = 1, packet_len;
>>> +
>>> +    /* usb device unregistered? */
>>> +    if (!(dev->state & PCAN_USB_STATE_CONNECTED))
>>> +        return 0;
>>> +
>>> +    /*
>>> +     * if a packet is not filled completely by commands, the command
>>> list
>>> +     * is terminated with an "end of collection" record.
>>> +     */
>>> +    cmd_len = cmd_tail - cmd_head;
>>> +    if (cmd_len < PCAN_UFD_CMD_BUFFER_SIZE) {
>> What happens if cmd_len turns out to be 511?
> 
> Well, it can't: "commands" are 64-bits records. The "end of record"
> "command" must be added when the list of commands doesn't fill the
> entire buffer content.
> Maybe it would be better to write:
> 
>    if (cmdlen <= (PCAN_UFD_CMD_BUFFER_SIZE - sizeof(u64)) {
> 
> 
> instead. What's your opinion about that?

Yes, better.

> 
>>
>>> +        memset(cmd_tail, 0xff, sizeof(u64));
>>> +        cmd_len += sizeof(u64);
>>> +    }
>>> +
>>> +    packet_ptr = cmd_head;
>>> +
>>> +    /* firmware is not able to re-assemble 512 bytes buffer in
>>> full-speed */
>>> +    if ((dev->udev->speed != USB_SPEED_HIGH) &&
>>> +        (cmd_len > PCAN_UFD_LOSPD_PKT_SIZE)) {
>>> +        packet_len = PCAN_UFD_LOSPD_PKT_SIZE;
>>> +        n += cmd_len / packet_len;
>>> +    } else {
>>> +        packet_len = cmd_len;
>>> +    }
>>> +
>>> +    for (p = 1; p <= n; p++) {
>> why not the usual, start at 0? (p = 0, p < n, p++), or even "i" :)
> 
> Well ok no problemo...
> 
>>
>>> +        err = usb_bulk_msg(dev->udev,
>>> +                   usb_sndbulkpipe(dev->udev,
>>> +                           PCAN_USBPRO_EP_CMDOUT),
>>> +                   packet_ptr, packet_len,
>>> +                   &actual_length, PCAN_UFD_COMMAND_TIMEOUT);
>> Do you have to take care about actual_length? If not, you can pass NULL
>> here AFAICS.
> 
> Ok, you're right. This parameter can be NULL. Will be changed.
> 
>>
>>> +        if (err) {
>>> +            netdev_err(dev->netdev,
>>> +                   "sending command failure: %d\n", err);
>>> +            break;
>>> +        }
>>> +
>>> +        packet_ptr += packet_len;
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +static int pcan_usb_fd_set_bus(struct peak_usb_device *dev, u8 onoff)
>>                                                                 bool on
> 
> Humm... ok, but be advised that this change will have some side-effects
> on the other existing files (pcan_usb_core.h, pcan_usb.c and
> pcan_usb_pro.c should be modified accordingly).

Okay, I haven't checked this. Keep it an u8 then.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]