Re: [PATCH 05/17] can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable bulk_rd_buf_size, because this will have different values for CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function pointer ems_usb_write_mode. In device probe function added a switch statement to select between CPC-USB/ARM7 and CPC-USB/FD and rearranged initialization sequence accordingly.

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

[-- Attachment #1.1: Type: text/plain, Size: 2901 bytes --]

On 11/10/20 11:31 AM, Gerhard Uttenthaler wrote:
> Am 06.11.20 um 18:15 schrieb Marc Kleine-Budde:
>> Please keep the patch subject within reasonable length.
>> It sould describe what you have done. The patch description should describe the why.
> OK
> 
>>
>> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>>> ---
>>>  drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
>>>  1 file changed, 47 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>>> index 4ed0d681a68c..a3943042b8c8 100644
>>> --- a/drivers/net/can/usb/ems_usb.c
>>> +++ b/drivers/net/can/usb/ems_usb.c
>>> @@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
>>>  
>>>  MODULE_DEVICE_TABLE(usb, ems_usb_table);
>>>  
>>> -#define RX_BUFFER_SIZE      64
>>
>> Can you keep this define instead of using a "64" below? Give it a proper
>> prefix/postfix if needed.
> OK
> 
>>
>>>  #define CPC_HEADER_SIZE     4
>>>  #define INTR_IN_BUFFER_SIZE 4
>>>  
>>> @@ -290,6 +289,8 @@ struct ems_usb {
>>>  	struct usb_device *udev;
>>>  	struct net_device *netdev;
>>>  
>>> +	u32 bulk_rd_buf_size;
>>> +
>>>  	atomic_t active_tx_urbs;
>>>  	struct usb_anchor tx_submitted;
>>>  	struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
>>> @@ -301,7 +302,9 @@ struct ems_usb {
>>>  	u8 *tx_msg_buffer;
>>>  
>>>  	u8 *intr_in_buffer;
>>> -	unsigned int free_slots; /* remember number of available slots */
>>> +	u32 free_slots; /* remember number of available slots */
>>
>> nitpick
>> Why this change?
> It was the last "unsigned int". They all are u32 now.

seems unrelated to the rest of the changes?

[...]

>>> -static const struct can_bittiming_const ems_usb_bittiming_const = {
>>> -	.name = "ems_usb",
>>> +static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
>>> +	.name = "ems_usb_arm7",
>>
>> You are changing the user space visible name of the CAN device. Is this needed?
> The driver will be able to handle both devices CPC-USB/ARM7 and
> CPC-USB/FD. If a user calls:
> 
> ip -details -statistics link show can0
> 
> then I noticed that "ems_usb" is inside the output. This seemed
> ambiguous for me as it could be a CPC-USB/ARM7 or a CPC-USB/FD. Sure
> this will need all patches applied to become visible.

Consider there is a software that relies on the "ems_usb" string. A kernel
update should not change this if not needed. The new "FD" adapter can and should
have a different name there.

Marc

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


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