On 28.5.2022 10.37, Jimmy Assarsson wrote:
> On 5/16/22 15:47, Anssi Hannula wrote:
>> The 0bfd:0124 Kvaser Mini PCI Express 2xHS (FW 4.18.778) seems to support
>> TX/RX error counters in exactly the same way (via unsolicited cmd 106 on
>> bus errors and via cmd 20 when queried with cmd 19) as 0bfd:0017 Kvaser
>> Memorator Professional HS/HS (FW 2.0.50), but only the latter has
>> KVASER_USB_HAS_TXRX_ERRORS set to enable do_get_berr_counter().
>>
>> Enable error counter retrieval for Kvaser Mini PCI Express 2xHS, too.
>>
>> Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
>> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
>>
>> ---
>>
>> I'm not really sure what KVASER_USB_HAS_TXRX_ERRORS means, exactly,
>> w.r.t. device behavior, though, i.e. how does a device without it behave.
> Devices without KVASER_USB_HAS_TXRX_ERRORS, firmware will always report
> zero for the Rx and Tx error counters.
>
> It's possible to query the device for specific capabilities.
> i.e. Kvaser Mini PCI Express 2xHS does also got support for silent mode.
> I want to replace this patch with the one below:
Sounds good!
A couple of minor style comments below. I didn't test the code yet.
> From fbf1c02e5f7860be9bdafd1c9b4f01c903dd9258 Mon Sep 17 00:00:00 2001
> From: Jimmy Assarsson <extja@xxxxxxxxxx>
> Date: Wed, 25 May 2022 20:21:19 +0200
> Subject: [PATCH 04/13] can: kvaser_usb: kvaser_usb_leaf: Get
> capabilities from
> device
>
> Use the CMD_GET_CAPABILITIES_REQ command to query the device for certain
> capabilities. We are only interested in LISTENONLY mode and wither the
> device reports CAN error counters.
>
> And remove hard coded capabilities for all Leaf devices.
>
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB
> devices")
> Reported-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
> Signed-off-by: Jimmy Assarsson <extja@xxxxxxxxxx>
> ---
> .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 61 ++------
> .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 146 +++++++++++++++++-
> 2 files changed, 162 insertions(+), 45 deletions(-)
[...]
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 09ade66256b2..aee2dae67459 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
[...]
> +static int kvaser_usb_leaf_get_single_capability(struct kvaser_usb *dev,
> + u16 cap_cmd_req, u16 *status)
> +{
> + struct kvaser_usb_dev_card_data *card_data = &dev->card_data;
> + struct kvaser_cmd *cmd;
> + u32 value = 0;
> + u32 mask = 0;
> + u16 cap_cmd_res;
> + int err;
> + int i;
> +
> + cmd = kcalloc(1, sizeof(struct kvaser_cmd), GFP_KERNEL);
kzalloc can be used here, and prefer sizeof(*ptr) to avoid the risk of
type mismatch:
cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
[...]
> +static int kvaser_usb_leaf_get_capabilities_leaf(struct kvaser_usb *dev)
> +{
> + int err;
> + u16 status;
> +
> + if (!(dev->card_data.capabilities & KVASER_USB_CAP_EXT_CAP)) {
> + dev_info(&dev->intf->dev,
> + "No extended capability support. Upgrade device firmware.\n");
> + return 0;
> + }
> +
> + err = kvaser_usb_leaf_get_single_capability
> + (dev,
> + KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE,
> + &status);
I believe kernel style is to keep the opening parenthesis on the same
line with the function name here.
> + if (err)
> + return err;
> + if (status)
> + dev_info(&dev->intf->dev,
> + "KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE failed %u\n",
> + status);
> +
> + err = kvaser_usb_leaf_get_single_capability
> + (dev,
> + KVASER_USB_LEAF_CAP_CMD_ERR_REPORT,
> + &status);
Ditto.
[...]
--
Anssi Hannula / Bitwise Oy
+358 503803997
