From: pza@pengutronix.de (Philipp Zabel)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC PATCH 2/3] staging: imx-drm-core: Use graph to find connection between crtc and encoder
Date: Mon, 10 Feb 2014 21:45:45 +0100 [thread overview]
Message-ID: <20140210204545.GA2793@pengutronix.de> (raw)
In-Reply-To: <20140210162631.GC26684@n2100.arm.linux.org.uk>
On Mon, Feb 10, 2014 at 04:26:31PM +0000, Russell King - ARM Linux wrote:
[...]
> Why is this loop soo complicated? Why do you need to mess around with
> this "last_ep" stuff - you don't actually end up using it.
The last_ep dance is necessary because v4l2_of_get_next_endpoint(node,prev)
does not decrement the reference count of its prev argument.
To make the loop as simple as you propose, I'd either have to change
the get_next_endpoint library function or wrap it to release the prev
node:
static struct device_node *imx_drm_of_get_next_endpoint(
struct device_node *node, struct device_node *prev)
{
node = v4l2_of_get_next_endpoint(node, prev);
of_node_put(prev);
return node;
}
> The loop reduces down to this without comments:
>
> for (i = 0; !ret; i++) {
> uint32_t mask;
>
> ep = v4l2_of_get_next_endpoint(np, last_ep);
> if (!ep)
> break;
>
> /* CSI */
> mask = imx_drm_find_crtc_mask(imxdrm, ep);
> of_node_put(ep);
>
> if (mask == 0)
> return -EPROBE_DEFER;
>
> crtc_mask |= mask;
> }
Yes, that is easier to read.
> Now, here's the big question: why do we want to use v4l2_* here? We
> may want to use this functionality, but if this functionality is going
> to be used outside of v4l2, it needs to become something generic, not
> v4l2 specific.
>
> Let's think about this for a moment... if we want to build imx-drm into
> the kernel, can we do it with modular videodev, or with videodev
> completely unconfigured. We may wish to do this because we have no
> videodev requirement on a platform. Should the build fail because the
> v4l2 function isn't there?
>
> So, before we can change this, I think we first need to get agreement
> from Mauro to move this function out of V4L2, so that it can be
> available independently of V4L2.
Either that, or we add a temporary copy of the v4l2_of_get_next_endpoint
and v4l2_of_get_remote_port functions while doing the same.
regards
Philipp
WARNING: multiple messages have this Message-ID (diff)
From: Philipp Zabel <pza@pengutronix.de>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: devel@driverdev.osuosl.org,
Philipp Zabel <p.zabel@pengutronix.de>,
David Airlie <airlied@linux.ie>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
dri-devel@lists.freedesktop.org, kernel@pengutronix.de,
Shawn Guo <shawn.guo@linaro.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 2/3] staging: imx-drm-core: Use graph to find connection between crtc and encoder
Date: Mon, 10 Feb 2014 21:45:45 +0100 [thread overview]
Message-ID: <20140210204545.GA2793@pengutronix.de> (raw)
In-Reply-To: <20140210162631.GC26684@n2100.arm.linux.org.uk>
On Mon, Feb 10, 2014 at 04:26:31PM +0000, Russell King - ARM Linux wrote:
[...]
> Why is this loop soo complicated? Why do you need to mess around with
> this "last_ep" stuff - you don't actually end up using it.
The last_ep dance is necessary because v4l2_of_get_next_endpoint(node,prev)
does not decrement the reference count of its prev argument.
To make the loop as simple as you propose, I'd either have to change
the get_next_endpoint library function or wrap it to release the prev
node:
static struct device_node *imx_drm_of_get_next_endpoint(
struct device_node *node, struct device_node *prev)
{
node = v4l2_of_get_next_endpoint(node, prev);
of_node_put(prev);
return node;
}
> The loop reduces down to this without comments:
>
> for (i = 0; !ret; i++) {
> uint32_t mask;
>
> ep = v4l2_of_get_next_endpoint(np, last_ep);
> if (!ep)
> break;
>
> /* CSI */
> mask = imx_drm_find_crtc_mask(imxdrm, ep);
> of_node_put(ep);
>
> if (mask == 0)
> return -EPROBE_DEFER;
>
> crtc_mask |= mask;
> }
Yes, that is easier to read.
> Now, here's the big question: why do we want to use v4l2_* here? We
> may want to use this functionality, but if this functionality is going
> to be used outside of v4l2, it needs to become something generic, not
> v4l2 specific.
>
> Let's think about this for a moment... if we want to build imx-drm into
> the kernel, can we do it with modular videodev, or with videodev
> completely unconfigured. We may wish to do this because we have no
> videodev requirement on a platform. Should the build fail because the
> v4l2 function isn't there?
>
> So, before we can change this, I think we first need to get agreement
> from Mauro to move this function out of V4L2, so that it can be
> available independently of V4L2.
Either that, or we add a temporary copy of the v4l2_of_get_next_endpoint
and v4l2_of_get_remote_port functions while doing the same.
regards
Philipp
next prev parent reply other threads:[~2014-02-10 20:45 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-06 14:51 [RFC PATCH 0/3] imx-drm dt bindings Philipp Zabel
2014-01-06 14:51 ` Philipp Zabel
2014-01-06 14:52 ` [RFC PATCH 1/3] ARM: dts: imx6qdl: Add ports and endpoints to IPU DIs Philipp Zabel
2014-01-06 14:52 ` Philipp Zabel
2014-01-06 14:52 ` [RFC PATCH 2/3] staging: imx-drm-core: Use graph to find connection between crtc and encoder Philipp Zabel
2014-01-06 14:52 ` Philipp Zabel
2014-02-10 16:26 ` Russell King - ARM Linux
2014-02-10 16:26 ` Russell King - ARM Linux
2014-02-10 20:45 ` Philipp Zabel [this message]
2014-02-10 20:45 ` Philipp Zabel
2014-02-11 8:38 ` Dan Carpenter
2014-02-11 8:38 ` Dan Carpenter
2014-02-11 9:00 ` Philipp Zabel
2014-02-11 9:00 ` Philipp Zabel
2014-01-06 14:52 ` [RFC PATCH 3/3] staging: imx-drm-core: associate crtc devices with di port nodes Philipp Zabel
2014-01-06 14:52 ` Philipp Zabel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140210204545.GA2793@pengutronix.de \
--to=pza@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.