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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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:
signature.asc
Description: OpenPGP digital signature
