drivers: can: use flags fields for can_frame and can_filter structs

The can_frame and can_filter structs support a number of different flags
(standard/extended CAN ID type, Remote Transmission Request, CAN-FD format,
Bit Rate Switch, ...). Each of these flags is represented as a discrete bit
in the given structure.

This design pattern requires every user of these structs to initialize all
of these flags to either 0 or 1, which does not scale well for future flag
additions.

Some of these flags have associated enumerations to be used for assignment,
some do not. CAN drivers and protocols tend to rely on the logical value of
the flag instead of using the enumeration, leading to a very fragile
API. The enumerations are used inconsistently between the can_frame and
can_filter structures, which further complicates the API.

Instead, convert these flags to bitfields with separate flag definitions
for the can_frame and can_filter structures. This API allows for future
extensions without having to revisit existing users of the two
structures. Furthermore, this allows driver to easily check for unsupported
flags in the respective API calls.

As this change leads to the "id_mask" field of the can_filter to be the
only mask present in that structure, rename it to "mask" for simplicity.

Fixes: zephyrproject-rtos#50776

Signed-off-by: Henrik Brix Andersen <[email protected]>

Author: henrikbrixandersen

doc/hardware/peripherals/canbus/controller.rst

@@ -152,8 +152,7 @@ a mailbox. When a transmitting mailbox is assigned, sending cannot be canceled.
 .. code-block:: C
 
   struct can_frame frame = {
-          .id_type = CAN_STANDARD_IDENTIFIER,
-          .rtr = CAN_DATAFRAME,
+          .flags = 0,
           .id = 0x123,
           .dlc = 8,
           .data = {1,2,3,4,5,6,7,8}
@@ -187,8 +186,7 @@ occurred. It does not block until the message is sent like the example above.
   int send_function(const struct device *can_dev)
   {
           struct can_frame frame = {
-                  .id_type = CAN_EXTENDED_IDENTIFIER,
-                  .rtr = CAN_DATAFRAME,
+                  .flags = CAN_FRAME_IDE,
                   .id = 0x1234567,
                   .dlc = 2
           };
@@ -227,10 +225,8 @@ The filter for this example is configured to match the identifier 0x123 exactly.
 .. code-block:: C
 
   const struct can_filter my_filter = {
-          .id_type = CAN_STANDARD_IDENTIFIER,
-          .rtr = CAN_DATAFRAME,
+          .flags = CAN_FILTER_DATA,
           .id = 0x123,
-          .rtr_mask = 1,
           .id_mask = CAN_STD_ID_MASK
   };
   int filter_id;
@@ -252,10 +248,8 @@ The filter for this example is configured to match the extended identifier
 .. code-block:: C
 
   const struct can_filter my_filter = {
-          .id_type = CAN_EXTENDED_IDENTIFIER,
-          .rtr = CAN_DATAFRAME,
+          .flags = CAN_FILTER_DATA | CAN_FILTER_IDE,
           .id = 0x1234567,
-          .rtr_mask = 1,
           .id_mask = CAN_EXT_ID_MASK
   };
   CAN_MSGQ_DEFINE(my_can_msgq, 2);

drivers/can/can_handlers.c

@@ -129,12 +129,12 @@ static inline int z_vrfy_can_set_bitrate_data(const struct device *dev,
 
 #endif /* CONFIG_CAN_FD_MODE */
 
-static inline int z_vrfy_can_get_max_filters(const struct device *dev, enum can_ide id_type)
+static inline int z_vrfy_can_get_max_filters(const struct device *dev, bool ide)
 {
 	/* Optional API function */
 	Z_OOPS(Z_SYSCALL_OBJ(dev, K_OBJ_DRIVER_CAN));
 
-	return z_impl_can_get_max_filters(dev, id_type);
+	return z_impl_can_get_max_filters(dev, ide);
 }
 #include <syscalls/can_get_max_filters_mrsh.c>
 

drivers/can/can_loopback.c

@@ -33,11 +33,14 @@ struct can_loopback_filter {
 struct can_loopback_data {
 	struct can_loopback_filter filters[CONFIG_CAN_MAX_FILTER];
 	struct k_mutex mtx;
-	bool loopback;
 	struct k_msgq tx_msgq;
 	char msgq_buffer[CONFIG_CAN_LOOPBACK_TX_MSGQ_SIZE * sizeof(struct can_loopback_frame)];
 	struct k_thread tx_thread_data;
 	bool started;
+	bool loopback;
+#ifdef CONFIG_CAN_FD_MODE
+	bool fd;
+#endif /* CONFIG_CAN_FD_MODE */
 
 	K_KERNEL_STACK_MEMBER(tx_thread_stack,
 		      CONFIG_CAN_LOOPBACK_TX_THREAD_STACK_SIZE);
@@ -51,9 +54,8 @@ static void dispatch_frame(const struct device *dev,
 
 	LOG_DBG("Receiving %d bytes. Id: 0x%x, ID type: %s %s",
 		frame->dlc, frame->id,
-		frame->id_type == CAN_STANDARD_IDENTIFIER ?
-				  "standard" : "extended",
-		frame->rtr == CAN_DATAFRAME ? "" : ", RTR frame");
+		(frame->flags & CAN_FRAME_IDE) != 0 ? "extended" : "standard",
+		(frame->flags & CAN_FRAME_RTR) != 0 ? ", RTR frame" : "");
 
 	filter->rx_cb(dev, &frame_tmp, filter->cb_arg);
 }
@@ -99,15 +101,29 @@ static int can_loopback_send(const struct device *dev,
 
 	LOG_DBG("Sending %d bytes on %s. Id: 0x%x, ID type: %s %s",
 		frame->dlc, dev->name, frame->id,
-		frame->id_type == CAN_STANDARD_IDENTIFIER ?
-				  "standard" : "extended",
-		frame->rtr == CAN_DATAFRAME ? "" : ", RTR frame");
+		(frame->flags & CAN_FRAME_IDE) != 0 ? "extended" : "standard",
+		(frame->flags & CAN_FRAME_RTR) != 0 ? ", RTR frame" : "");
 
 #ifdef CONFIG_CAN_FD_MODE
-	if (frame->fd != 0) {
+	if ((frame->flags & ~(CAN_FRAME_IDE | CAN_FRAME_RTR |
+		CAN_FRAME_FDF | CAN_FRAME_BRS)) != 0) {
+		LOG_ERR("unsupported CAN frame flags 0x%02x", frame->flags);
+		return -ENOTSUP;
+	}
+
+	if ((frame->flags & CAN_FRAME_FDF) != 0) {
+		if (!data->fd) {
+			return -ENOTSUP;
+		}
+
 		max_dlc = CANFD_MAX_DLC;
 	}
-#endif /* CONFIG_CAN_FD_MODE */
+#else /* CONFIG_CAN_FD_MODE */
+	if ((frame->flags & ~(CAN_FRAME_IDE | CAN_FRAME_RTR)) != 0) {
+		LOG_ERR("unsupported CAN frame flags 0x%02x", frame->flags);
+		return -ENOTSUP;
+	}
+#endif /* !CONFIG_CAN_FD_MODE */
 
 	if (frame->dlc > max_dlc) {
 		LOG_ERR("DLC of %d exceeds maximum (%d)", frame->dlc, max_dlc);
@@ -150,14 +166,12 @@ static int can_loopback_add_rx_filter(const struct device *dev, can_rx_callback_
 	struct can_loopback_filter *loopback_filter;
 	int filter_id;
 
-	LOG_DBG("Setting filter ID: 0x%x, mask: 0x%x", filter->id,
-		    filter->id_mask);
-	LOG_DBG("Filter type: %s ID %s mask",
-		filter->id_type == CAN_STANDARD_IDENTIFIER ?
-				   "standard" : "extended",
-		((filter->id_type && (filter->id_mask == CAN_STD_ID_MASK)) ||
-		(!filter->id_type && (filter->id_mask == CAN_EXT_ID_MASK))) ?
-		"with" : "without");
+	LOG_DBG("Setting filter ID: 0x%x, mask: 0x%x", filter->id, filter->mask);
+
+	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
+		return -ENOTSUP;
+	}
 
 	k_mutex_lock(&data->mtx, K_FOREVER);
 	filter_id = get_free_filter(data->filters);
@@ -242,14 +256,17 @@ static int can_loopback_set_mode(const struct device *dev, can_mode_t mode)
 		LOG_ERR("unsupported mode: 0x%08x", mode);
 		return -ENOTSUP;
 	}
+
+	data->fd = (mode & CAN_MODE_FD) != 0;
 #else
 	if ((mode & ~(CAN_MODE_LOOPBACK)) != 0) {
 		LOG_ERR("unsupported mode: 0x%08x", mode);
 		return -ENOTSUP;
 	}
 #endif /* CONFIG_CAN_FD_MODE */
 
-	data->loopback = (mode & CAN_MODE_LOOPBACK) != 0 ? 1 : 0;
+	data->loopback = (mode & CAN_MODE_LOOPBACK) != 0;
+
 	return 0;
 }
 
@@ -335,9 +352,9 @@ static int can_loopback_get_core_clock(const struct device *dev, uint32_t *rate)
 	return 0;
 }
 
-static int can_loopback_get_max_filters(const struct device *dev, enum can_ide id_type)
+static int can_loopback_get_max_filters(const struct device *dev, bool ide)
 {
-	ARG_UNUSED(id_type);
+	ARG_UNUSED(ide);
 
 	return CONFIG_CAN_MAX_FILTER;
 }