| Message ID | 1389019922-31992-3-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 06, 2014 at 03:52:01PM +0100, Philipp Zabel wrote: > @@ -438,24 +453,21 @@ int imx_drm_encoder_parse_of(struct drm_device *drm, > struct drm_encoder *encoder, struct device_node *np) > { > struct imx_drm_device *imxdrm = drm->dev_private; > + struct device_node *ep, *last_ep = NULL; > uint32_t crtc_mask = 0; > int i, ret = 0; > > for (i = 0; !ret; i++) { > - struct of_phandle_args args; > uint32_t mask; > - int id; > > - ret = of_parse_phandle_with_args(np, "crtcs", "#crtc-cells", i, > - &args); > - if (ret == -ENOENT) > + ep = v4l2_of_get_next_endpoint(np, last_ep); > + if (last_ep) > + of_node_put(last_ep); > + if (!ep) > break; > - if (ret < 0) > - return ret; > > - id = args.args_count > 0 ? args.args[0] : 0; > - mask = imx_drm_find_crtc_mask(imxdrm, args.np, id); > - of_node_put(args.np); > + /* CSI */ > + mask = imx_drm_find_crtc_mask(imxdrm, ep); > > /* > * If we failed to find the CRTC(s) which this encoder is > @@ -463,12 +475,20 @@ int imx_drm_encoder_parse_of(struct drm_device *drm, > * not been registered yet. Defer probing, and hope that > * the required CRTC is added later. > */ > - if (mask == 0) > + if (mask == 0) { > + of_node_put(ep); > return -EPROBE_DEFER; > + } > > crtc_mask |= mask; > + last_ep = ep; > } > > + if (ep) > + of_node_put(ep); 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 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; } 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.
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
> 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 loop reduces down to this without comments: > > for (i = 0; !ret; i++) { ^^^^ Philipp, "ret" isn't set anymore in the new loop. > 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; > } regards, dan carpenter
Hi Dan, Am Dienstag, den 11.02.2014, 11:38 +0300 schrieb Dan Carpenter: > > 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 loop reduces down to this without comments: > > > > for (i = 0; !ret; i++) { > ^^^^ > Philipp, "ret" isn't set anymore in the new loop. thanks, I'll drop ret in the next version. regards Philipp
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index 2490dc3..2c20434 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -23,6 +23,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_fb_cma_helper.h> +#include <media/v4l2-of.h> #include "imx-drm.h" @@ -420,9 +421,23 @@ EXPORT_SYMBOL_GPL(imx_drm_remove_crtc); * or removed once the DRM device has been fully initialised. */ static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm, - void *cookie, int id) + struct device_node *endpoint) { + struct device_node *remote_port; + void *cookie; unsigned i; + int id = 0; + + remote_port = v4l2_of_get_remote_port(endpoint); + if (remote_port) + of_property_read_u32(remote_port, "reg", &id); + else + return 0; + cookie = remote_port->parent; + of_node_put(remote_port); + + /* IPU specific: CSI0/1 at 0/1, DI0/1 at 2/3 */ + id -= 2; for (i = 0; i < MAX_CRTC; i++) { struct imx_drm_crtc *imx_drm_crtc = imxdrm->crtc[i]; @@ -438,24 +453,21 @@ int imx_drm_encoder_parse_of(struct drm_device *drm, struct drm_encoder *encoder, struct device_node *np) { struct imx_drm_device *imxdrm = drm->dev_private; + struct device_node *ep, *last_ep = NULL; uint32_t crtc_mask = 0; int i, ret = 0; for (i = 0; !ret; i++) { - struct of_phandle_args args; uint32_t mask; - int id; - ret = of_parse_phandle_with_args(np, "crtcs", "#crtc-cells", i, - &args); - if (ret == -ENOENT) + ep = v4l2_of_get_next_endpoint(np, last_ep); + if (last_ep) + of_node_put(last_ep); + if (!ep) break; - if (ret < 0) - return ret; - id = args.args_count > 0 ? args.args[0] : 0; - mask = imx_drm_find_crtc_mask(imxdrm, args.np, id); - of_node_put(args.np); + /* CSI */ + mask = imx_drm_find_crtc_mask(imxdrm, ep); /* * If we failed to find the CRTC(s) which this encoder is @@ -463,12 +475,20 @@ int imx_drm_encoder_parse_of(struct drm_device *drm, * not been registered yet. Defer probing, and hope that * the required CRTC is added later. */ - if (mask == 0) + if (mask == 0) { + of_node_put(ep); return -EPROBE_DEFER; + } crtc_mask |= mask; + last_ep = ep; } + if (ep) + of_node_put(ep); + if (i == 0) + return -ENOENT; + encoder->possible_crtcs = crtc_mask; /* FIXME: this is the mask of outputs which can clone this output. */
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/staging/imx-drm/imx-drm-core.c | 44 ++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 12 deletions(-)