| Message ID | 1457122813-12791-3-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote: > The fimc_md_parse_port_node() function return 0 if an endpoint node is > not found but according to Documentation/devicetree/bindings/graph.txt, > a port must always have at least one enpoint. > > So return an -EINVAL errno code to the caller instead, so it knows that > the port node parse failed due an invalid Device Tree description. I don't think it is forbidden to have a port node in device tree containing no endpoint nodes. Empty port node means only that, for example, a subsystem has a port/bus for connecting external devices but nothing is actually connected to it. In case of Exynos CSIS it might not be so useful to have an empty port node specified in some top level *.dtsi file and only the endpoints specified in a board specific dts file. Nevertheless, I wouldn't be saying in general a port node must always have some endpoint node defined. I could apply this patch as it doesn't do any harm considering existing dts files in the kernel tree (arch/arm/boot/dts/ exynos4412-trats2.dts), but the commit description would need to be changed.
Hello Sylwester, On Fri, Mar 11, 2016 at 10:03 AM, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote: >> The fimc_md_parse_port_node() function return 0 if an endpoint node is >> not found but according to Documentation/devicetree/bindings/graph.txt, >> a port must always have at least one enpoint. >> >> So return an -EINVAL errno code to the caller instead, so it knows that >> the port node parse failed due an invalid Device Tree description. > > I don't think it is forbidden to have a port node in device tree > containing no endpoint nodes. Empty port node means only that, > for example, a subsystem has a port/bus for connecting external > devices but nothing is actually connected to it. > That's not what I understood by reading both Documentation/devicetree/bindings/media/video-interfaces.txt and Documentation/devicetree/bindings/graph.txt but maybe these are not that clear about it or I just failed to parse the english. > In case of Exynos CSIS it might not be so useful to have an empty > port node specified in some top level *.dtsi file and only > the endpoints specified in a board specific dts file. Nevertheless, > I wouldn't be saying in general a port node must always have some > endpoint node defined. > Ok, but if that is valid then I believe that at the very least Documentation/devicetree/bindings/media/samsung-fimc.txt should explicitly mention which (sub)nodes are optional and which are required so the DT parsing logic could follow what's documented there. > I could apply this patch as it doesn't do any harm considering > existing dts files in the kernel tree (arch/arm/boot/dts/ > exynos4412-trats2.dts), but the commit description would need to > be changed. > I don't mind if you want to change the commit message but if those nodes are really optional then a follow-up should be to update the DT binding docs to make that clear IMHO. > -- > Thanks, > Sylwester > -- Best regards, Javier
Hello Sylwester, On 03/11/2016 10:03 AM, Sylwester Nawrocki wrote: > On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote: >> The fimc_md_parse_port_node() function return 0 if an endpoint node is >> not found but according to Documentation/devicetree/bindings/graph.txt, >> a port must always have at least one enpoint. >> >> So return an -EINVAL errno code to the caller instead, so it knows that >> the port node parse failed due an invalid Device Tree description. > > I don't think it is forbidden to have a port node in device tree > containing no endpoint nodes. Empty port node means only that, > for example, a subsystem has a port/bus for connecting external > devices but nothing is actually connected to it. > > In case of Exynos CSIS it might not be so useful to have an empty > port node specified in some top level *.dtsi file and only > the endpoints specified in a board specific dts file. Nevertheless, > I wouldn't be saying in general a port node must always have some > endpoint node defined. > You are right, I asked Laurent and he confirms what you said that it's possible to have ports with no endpoints. I still think the DT binding docs could be more clear but that's a separate issue. > I could apply this patch as it doesn't do any harm considering > existing dts files in the kernel tree (arch/arm/boot/dts/ > exynos4412-trats2.dts), but the commit description would need to > be changed. > No worries, the current code is correct if endpoints are optional and this patch is wrong so it should not be applied. Best regards,
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index feb521f28e14..06f3d75c9a0e 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -394,7 +394,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd, /* Assume here a port node can have only one endpoint node. */ ep = of_get_next_child(port, NULL); if (!ep) - return 0; + return -EINVAL; ret = v4l2_of_parse_endpoint(ep, &endpoint); if (ret) {
The fimc_md_parse_port_node() function return 0 if an endpoint node is not found but according to Documentation/devicetree/bindings/graph.txt, a port must always have at least one enpoint. So return an -EINVAL errno code to the caller instead, so it knows that the port node parse failed due an invalid Device Tree description. Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- drivers/media/platform/exynos4-is/media-dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)