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

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

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

On 01/13/2015 02:18 PM, Stephane Grosjean wrote:
> Hi Marc,
> 
> Le 07/01/2015 18:03, Marc Kleine-Budde a écrit :
>> On 01/06/2015 11:00 AM, Stephane Grosjean wrote:
>>   @@ -750,9 +795,10 @@ static int peak_usb_create_dev(struct
>> peak_usb_adapter *peak_usb_adapter,
>>       dev->can.clock = peak_usb_adapter->clock;
>>       dev->can.bittiming_const = &peak_usb_adapter->bittiming_const;
>>       dev->can.do_set_bittiming = peak_usb_set_bittiming;
>> +    dev->can.data_bittiming_const =
>> &peak_usb_adapter->data_bittiming_const;
>> +    dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
>>       dev->can.do_set_mode = peak_usb_set_mode;
>> -    dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>> -                      CAN_CTRLMODE_LISTENONLY;
>> +    dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
>> All ctrlmode_supported can go into a seperate patch.
> 
> I'll try but can you please explain why?

Because it's a separate feature. Making the review easier.

[...]

>> What about the following hunks (but not the .ctrlmode_supported) ? They
>> can go into a seperate patch which comes first?
> 
> So. You want me to do a serie of patches, right?
> - a first patch that would change the existing files
> - another one that would add the ctrl_mode_supported chanegs
> - another one that would add the new files

It's not about existing and new files. A patch should handle a single
topic and be easy to review. If something can be separated it should.

For example, adding the ctrl_mode_supported for the existing adapters
would be such a feature. Making the functions in pcan_usb_pro.c non
static, moving the #defines and adding the function declarations to
pcan_usb_pro.h is another preparation patch, so that the final patch,
that adds the new adapter is smaller and thus easier to review.

>>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>>> b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>>> index 4cfa3b8..a764045 100644
>>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>>> @@ -27,14 +27,6 @@
>>>     MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
>>>   -/* PCAN-USB Pro Endpoints */
>>> -#define PCAN_USBPRO_EP_CMDOUT        1
>>> -#define PCAN_USBPRO_EP_CMDIN        (PCAN_USBPRO_EP_CMDOUT |
>>> USB_DIR_IN)
>>> -#define PCAN_USBPRO_EP_MSGOUT_0        2
>>> -#define PCAN_USBPRO_EP_MSGIN        (PCAN_USBPRO_EP_MSGOUT_0 |
>>> USB_DIR_IN)
>>> -#define PCAN_USBPRO_EP_MSGOUT_1        3
>>> -#define PCAN_USBPRO_EP_UNUSED        (PCAN_USBPRO_EP_MSGOUT_1 |
>>> USB_DIR_IN)
>>> -
>>>   #define PCAN_USBPRO_CHANNEL_COUNT    2
>>>     /* PCAN-USB Pro adapter internal clock (MHz) */
>>> @@ -322,8 +314,8 @@ static int pcan_usb_pro_wait_rsp(struct
>>> peak_usb_device *dev,
>>>       return (i >= PCAN_USBPRO_RSP_SUBMIT_MAX) ? -ERANGE : err;
>>>   }
> 
>>>   +extern int pcan_usb_pro_probe(struct usb_interface *intf);
>>> +extern int pcan_usb_pro_send_req(struct peak_usb_device *dev, int
>>> req_id,
>>> +                                         int req_value, void *req_addr,
>>> +                         int req_size);
>>> +extern void pcan_usb_pro_restart_complete(struct urb *urb);
>> IIRC we're killing the "extern" in function definitions in the kernel.
> 
> Ok. So I remove all "extern" keywords even from existing files, right?

Up to you, this probably would be another patch. However, please don't
add new "extern"s :)

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 --]