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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 26.01.2015 16:51, Marc Kleine-Budde wrote:
> On 01/26/2015 04:42 PM, Stephane Grosjean wrote:
>> Le 26/01/2015 16:14, Marc Kleine-Budde a écrit :
>>> On 01/26/2015 04:11 PM, Stephane Grosjean wrote:
>>>> Hi Mark,
>>>>
>>>> Well, the ISO/non-ISO feature is not available for all USB devices and
>>>> is currently always under development.
>>>> So, we think that it's a bit early to add it in the peak_usb driver.
>>> Which mode do the controller support then?
>>>
>>> Marc
>>>
>> Current versions of the fw run in non-ISO mode.
>
> So we have to set .ctrlmode = CAN_CTRLMODE_FD_NON_ISO, analogue to
>
>      6cfda7fbebe8 can: m_can: tag current CAN FD controllers as non-ISO

Ack.

>
> e.g.:
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index b2f2694d7d5b..d00bc094b91d 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -799,6 +799,7 @@ static int peak_usb_create_dev(struct peak_usb_adapter *peak_usb_adapter,
>          dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
>          dev->can.do_set_mode = peak_usb_set_mode;
>          dev->can.do_get_berr_counter = peak_usb_adapter->do_get_berr_counter;
> +       dev->can.ctrlmode = peak_usb_adapter->ctrlmode;
>          dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
>
>          netdev->netdev_ops = &peak_usb_netdev_ops;
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.h b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> index 7c213b26f3fb..a8a71ebaa0b4 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.h
> @@ -46,6 +46,7 @@ struct peak_usb_device;
>   struct peak_usb_adapter {
>          char *name;
>          u32 device_id;
> +       u32 ctrlmode;
>          u32 ctrlmode_supported;
>          struct can_clock clock;
>          const struct can_bittiming_const bittiming_const;
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index 8aad8f79fc1b..73c4be840dc7 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -960,6 +960,7 @@ struct peak_usb_adapter pcan_usb_fd = {
>          .name = "PCAN-USB FD",
>          .device_id = PCAN_USBFD_PRODUCT_ID,
>          .ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
> +       .ctrlmode = CAN_CTRLMODE_FD_NON_ISO,
>          .ctrlmode_supported = CAN_CTRLMODE_FD |
>                          CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>          .clock = {
> @@ -1028,6 +1029,7 @@ struct peak_usb_adapter pcan_usb_pro_fd = {
>          .name = "PCAN-USB Pro FD",
>          .device_id = PCAN_USBPROFD_PRODUCT_ID,
>          .ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
> +       .ctrlmode = CAN_CTRLMODE_FD_NON_ISO,
>          .ctrlmode_supported = CAN_CTRLMODE_FD |
>                          CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
>          .clock = {
>
> Correct, Oliver?

No.

I assume it is better to set the ctrlmode directly in the struct can_priv 
element in  pcan_usb_fd_init() after retrieving the firmware version.

IMO it doesn't make sense to transport this info through the mechanic above 
which mostly initialized quasi constant settings for the adapter.


Btw.

Should

+/* describes the PCAN-USB Pro FD adapter */
+struct peak_usb_adapter pcan_usb_pro_fd = {

be

+/* describes the PCAN-USB Pro FD adapter */
+const struct peak_usb_adapter pcan_usb_pro_fd = {

???

Cheers from Stockholm,
Oliver