Re: [PATCH 7/7 v8] can/peak_usb: add support for PEAK new CANFD USB adapters

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 22.01.2015 17:13, Marc Kleine-Budde wrote:
> On 01/22/2015 04:59 PM, Stephane Grosjean wrote:
>>
>> Le 22/01/2015 16:41, Marc Kleine-Budde a écrit :
>>> On 01/22/2015 04:24 PM, Stephane Grosjean wrote:
>>>> Add support for the following new PEAK-System technik CANFD USB adapters:
>>>>
>>>> PCAN-USB FD             single CANFD channel USB adapter
>>>> PCAN-USB Pro FD         dual CANFD channels USB adapter
>>>>
>>>> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>> [...]
>>>
>>>> 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..2129549
>>>> --- /dev/null
>>>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
>>> [...]
>>>
>>>> +/* handle device specific info used by the netdevices */
>>>> +struct pcan_usb_fd_if {
>>>> +	struct peak_usb_device *dev[PCAN_USB_MAX_CHANNEL];
>>>> +	struct pcan_ufd_fw_info fw_info;
>>>> +	struct peak_time_ref    time_ref;
>>>> +	int                     cm_ignore_count;
>>>> +	int                     dev_opened_count;
>>> Please use tab for indention, not space.
>>>
>>>> +};
>>>> +
>>>> +/* device information */
>>>> +struct pcan_usb_fd_device {
>>>> +	struct peak_usb_device  dev;
>>>> +	struct can_berr_counter bec;
>>>> +	struct pcan_usb_fd_if  *usb_if;
>>>> +	u8                     *cmd_buffer_addr;
>>> dito
>>>
>>> Marc
>>>
>> Well, I'm afraid I have misunderstood one of your previous reMarks:
>>
>>> +/* device information */
>>> +struct pcan_usb_fd_device {
>>> +	struct peak_usb_device	dev;
>>> +	struct pcan_usb_fd_if *	usb_if;
>>> 	struct pcan_usb_fd_if *usb_if
>>>> +
>>>> +	u8 *	cmd_buffer_addr;
>>> 	u8 *cmd_buffer_addr
>>>> +
>>>> +	uint	tx_error_counter;
>>>> +	uint	rx_error_counter;
>>> 	unsigned int
>>>
>>> Please don't use a tab after the struct foo, u8, unsinged int, etc...,
>>> as it doesn't align.
>>>
>>
>> can u explain a bit more, please ? FYI, checkpatch.pl doesn't see any
>> warning nor fatal in these indentations...
>
> You have decided for indention in structs in your files, if you do so,
> please use TABs consistently everywhere. (Or don't do indention at all.)
>

I thought between the data type and the variable name is just sonly one single 
space.

So

+struct __packed pucan_timing_slow {
+	__le16	opcode_channel;
+
+	u8	ewl;		/* Error Warning limit */
+	u8	sjw_t;		/* Sync Jump Width + Triple sampling */
+	u8	tseg2;		/* Timing SEGment 2 */
+	u8	tseg1;		/* Timing SEGment 1 */
+
+	__le16	brp;		/* BaudRate Prescaler */
+};

becomes

+struct __packed pucan_timing_slow {
+	__le16 opcode_channel;
+
+	u8 ewl;		/* Error Warning limit */
+	u8 sjw_t;	/* Sync Jump Width + Triple sampling */
+	u8 tseg2;	/* Timing SEGment 2 */
+	u8 tseg1;	/* Timing SEGment 1 */
+
+	__le16 brp;	/* BaudRate Prescaler */
+};

With tabs indenting u8 from the start of the line and tabs before the comment.

Regards,
Oliver