Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: video: Introduce macro to deal with endpoints #73023

Closed
wants to merge 1 commit into from
Closed

drivers: video: Introduce macro to deal with endpoints #73023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions dts/bindings/test/vnd,video.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Copyright (c) 2024 tinyVision.ai Inc.
# SPDX-License-Identifier: Apache-2.0

description: Test Video device

compatible: "vnd,video"

child-binding:
child-binding:
include: video-interfaces.yaml
64 changes: 64 additions & 0 deletions include/zephyr/devicetree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3787,6 +3787,70 @@
*/
#define DT_ON_BUS(node_id, bus) IS_ENABLED(DT_CAT3(node_id, _BUS_, bus))

/**
* @}
*/

/**
* @defgroup devicetree-generic-endpoint Endpoint helpers
* @ingroup devicetree
* @{
*/

/**
* @brief Get the remote endpoint node connected to a local endpoint.
*
* Devices can be interconnected through ports and endpoints.
*
* Example devicetree overlay:
*
* @code{.dts}
* &source {
* port {
* source_out: endpoint {
* remote-endpoint-label = "sink_in";
* source-property = <1>;
* };
* };
* };
*
* &sink {
* port {
* sink_in: endpoint {
* remote-endpoint-label = "source_out";
* sink-property = <2>;
* };
* };
* };
* @endcode
*
* @c DT_REMOTE_ENDPOINT(endpoint_node) permits to access the other endpoint
* connected to @c endpoint_node.
*
* Example usage: starting from the sink, access the source endpoint connected
* to the @c sink_in endpoint:
*
* @code{.c}
* DT_PROP(DT_REMOTE_ENDPOINT(DT_NODELABEL(sink_in)), source_property)
* @endcode
*
* Example usage, starting from the source, to access the sink endpoint
* connected to the @c source_out endpoint:
*
* @code{.c}
* DT_PROP(DT_REMOTE_ENDPOINT(DT_NODELABEL(source_out)), sink_property)
* @endcode
*
* @note Once circular phandle references are supported in Zephyr devicetree,
* @c remote-endpoint-label (string) may be changed into
* @c remote-endpoint (phandle).
*
* @param node The local endpoint node to use.
*
* @return The remote endpoint node connected to @p node.
*/
#define DT_REMOTE_ENDPOINT(node) DT_NODELABEL(DT_STRING_TOKEN(node, remote_endpoint_label))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given real macro usages in drivers as below:

Case 1:

#define DEVICE_DT_INST_GET_SOURCE_DEV(n)                                                           \
	DEVICE_DT_GET(DT_PARENT(DT_GPARENT(DT_NODELABEL(DT_STRING_TOKEN(                           \
		DT_CHILD(DT_INST_CHILD(n, port), endpoint), remote_endpoint_label)))))

the node parameter in this case will be DT_CHILD(DT_INST_CHILD(n, port), endpoint)

Case 2:

#define DEVICE_DT_INST_GET_SENSOR_DEV(n)                                                           \
	DEVICE_DT_GET(DT_GPARENT(DT_NODELABEL(                                                     \
		DT_STRING_TOKEN(DT_CHILD(DT_CHILD(DT_INST_CHILD(n, ports), port_1), endpoint),     \
				remote_endpoint_label))))

the node parameter will be DT_CHILD(DT_CHILD(DT_INST_CHILD(n, ports), port_1), endpoint)

There's still endpointcommon for all cases. Is it possible to take endpoint in the DT_REMOTE_ENDPOINT api ?

Also, to retrieve the peer device object, we always has at least DT_GPARENT, is it possible to also take this in the macro API ?

Copy link
Collaborator Author

@josuah josuah Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to reduce the overhead by putting endpoint in common to each, but then comes looping over every endpoint:

#define ENDPOINT(n) DT_PROP(DT_REMOTE_ENDPOINT(n), link_parameter),

int link_parameters[] = {
        DT_FOREACH_CHILD(DT_INST_CHILD(DT_DRV_INST(0), port), ENDPOINT)
};

So this means that it is also useful to expose an intermediate macro that can be used in both case, only wrapping the remote-endpoint-label -> nodelabel -> node conversion.

It will also be possible to have some macro using COND_CODE_1() to make the "ports" vs "port" abstraction.

#define DT_PORTS(n) \
COND_CODE_1(DT_HAS_PROP(n, ports), (DT_CHILD(n, ports)), (n))

#define DT_INST_PORTS(n) \
DT_PORTS(DT_DRV_INST(n))

/* Example */
DEVICE_DT_GET(DT_GPARENT(DT_REMOTE_ENDPOINT(
        DT_CHILD(DT_CHILD(DT_INST_PORTS(0), port_3, endpoint_2)))))

This will return the node that contains all the ports, for instance:

#define DT_PORTS_REMOTE_ENDPOINT(n, port, endpoint) \
DT_REMOTE_ENDPOINT(DT_CHILD(DT_CHILD(DT_PORTS(DT_DRV_INST(0)), port), endpoint))

#define DT_INST_PORTS_REMOTE_ENDPOINT(n, port, endpoint) \
DT_PORTS(DT_DRV_INST(n))

/* Example */
DEVICE_DT_GET(DT_GPARENT(DT_INST_PORTS_REMOTE_ENDPOINT(0, port_3, endpoint_1)))

Then, there is going down from the other device, and we might need the same check for "ports" present or not.

#define DT_ENDPOINT_DEVICE(n) \
COND_CODE_1(DT_PROP_HAS(DT_GPARENT(n), port), (DT_GPARENT(n)), (DT_PARENT(DT_GPARENT(n))))

/* Example */
DEVICE_DT_GET(DT_ENDPOINT_DEVICE(DT_INST_PORTS_REMOTE_ENDPOINT(0, port_3, endpoint_1)))

And then why not combine them two, as the most frequent use-case for simple drivers would be to pick the device connected through a particular endpoint:

#define DT_REMOTE_DEVICE(n, port, endpoint) \
DT_ENDPOINT_DEVICE(DT_INST_PORTS_REMOTE_ENDPOINT(0, port, endpoint))

#define DT_INST_REMOTE_DEVICE(n, port, endpoint) \
DT_REMOTE_DEVICE(DT_DRV_INST(n), port, endpoint)

/* Example */
DEVICE_DT_GET(DT_INST_REMOTE_DEVICE(0, port, endpoint))

Would it make sense to decompose the implementation this much?

The intermediate macros would need to be implemented anyway to avoid making them too large, so we might as well document and test each intermediate step.


/**
* @}
*/
Expand Down
20 changes: 20 additions & 0 deletions tests/lib/devicetree/api/app.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,26 @@
status = "okay";
};

test_video_source: video_source {
compatible = "vnd,video";

port {
test_video_source_out: endpoint {
remote-endpoint-label = "test_video_sink_in";
};
};
};

test_video_sink: video_sink {
compatible = "vnd,video";

port {
test_video_sink_in: endpoint {
remote-endpoint-label = "test_video_source_out";
};
};
};

test_pwm1: pwm@55551111 {
compatible = "vnd,pwm";
#pwm-cells = <3>;
Expand Down
16 changes: 16 additions & 0 deletions tests/lib/devicetree/api/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@
#define TEST_DMA_CTLR_1 DT_NODELABEL(test_dma1)
#define TEST_DMA_CTLR_2 DT_NODELABEL(test_dma2)

#define TEST_VIDEO_SOURCE_OUT DT_NODELABEL(test_video_source_out)
#define TEST_VIDEO_SINK_IN DT_NODELABEL(test_video_sink_in)

#define TEST_IO_CHANNEL_CTLR_1 DT_NODELABEL(test_adc_1)
#define TEST_IO_CHANNEL_CTLR_2 DT_NODELABEL(test_adc_2)

Expand Down Expand Up @@ -1278,6 +1281,19 @@
zassert_false(DT_INST_DMAS_HAS_IDX(0, 2), "");
}

#undef DT_DRV_COMPAT
#define DT_DRV_COMPAT vnd_video_device
ZTEST(devicetree_api, test_video)
{
/* DT_REMOTE_ENDPOINT(source) */
zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SOURCE_OUT),
TEST_VIDEO_SINK_IN), "");

/* DT_REMOTE_ENDPOINT(sink) */
zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SINK_IN),
TEST_VIDEO_SOURCE_OUT), "");
}

Check notice on line 1295 in tests/lib/devicetree/api/src/main.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

tests/lib/devicetree/api/src/main.c:1295 
-	zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SOURCE_OUT),
-				  TEST_VIDEO_SINK_IN), "");
+	zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SOURCE_OUT), TEST_VIDEO_SINK_IN),
+		     "");
 
 	/* DT_REMOTE_ENDPOINT(sink) */
-	zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SINK_IN),
-				  TEST_VIDEO_SOURCE_OUT), "");
+	zassert_true(DT_SAME_NODE(DT_REMOTE_ENDPOINT(TEST_VIDEO_SINK_IN), TEST_VIDEO_SOURCE_OUT),
+		     "");

#undef DT_DRV_COMPAT
#define DT_DRV_COMPAT vnd_phandle_holder
ZTEST(devicetree_api, test_pwms)
Expand Down
Loading