Hi Sakari,
Thanks for your patch, I like how this turned out :-)
On 2017-08-29 14:03:13 +0300, Sakari Ailus wrote:
> This is the preferred way to parse the endpoints.
>
> Comment rcar-vin as an example.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 111 ++++++++--------------------
> drivers/media/platform/rcar-vin/rcar-dma.c | 10 +--
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 ++--
> drivers/media/platform/rcar-vin/rcar-vin.h | 4 +-
> drivers/media/v4l2-core/v4l2-fwnode.c | 47 ++++++++++++
> include/media/v4l2-fwnode.h | 39 ++++++++++
> 6 files changed, 132 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 142de447aaaa..4378806be1d4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
>
> +#include <media/v4l2-async.h>
> #include <media/v4l2-fwnode.h>
>
> #include "rcar-vin.h"
> @@ -77,14 +78,14 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> int ret;
>
> /* Verify subdevices mbus format */
> - if (!rvin_mbus_supported(&vin->digital)) {
> + if (!rvin_mbus_supported(vin->digital)) {
> vin_err(vin, "Unsupported media bus format for %s\n",
> - vin->digital.subdev->name);
> + vin->digital->subdev->name);
> return -EINVAL;
> }
>
> vin_dbg(vin, "Found media bus format for %s: %d\n",
> - vin->digital.subdev->name, vin->digital.code);
> + vin->digital->subdev->name, vin->digital->code);
>
> ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> if (ret < 0) {
> @@ -103,7 +104,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
>
> vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> rvin_v4l2_remove(vin);
> - vin->digital.subdev = NULL;
> + vin->digital->subdev = NULL;
> }
>
> static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -120,117 +121,67 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> if (ret < 0)
> return ret;
> - vin->digital.source_pad = ret;
> + vin->digital->source_pad = ret;
>
> ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> - vin->digital.sink_pad = ret < 0 ? 0 : ret;
> + vin->digital->sink_pad = ret < 0 ? 0 : ret;
>
> - vin->digital.subdev = subdev;
> + vin->digital->subdev = subdev;
>
> vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> - subdev->name, vin->digital.source_pad,
> - vin->digital.sink_pad);
> + subdev->name, vin->digital->source_pad,
> + vin->digital->sink_pad);
>
> return 0;
> }
>
> -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin,
> - struct device_node *ep,
> - struct v4l2_mbus_config *mbus_cfg)
> +static int rvin_digital_parse_v4l2(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> {
> - struct v4l2_fwnode_endpoint v4l2_ep;
> - int ret;
> -
> - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> - if (ret) {
> - vin_err(vin, "Could not parse v4l2 endpoint\n");
> - return -EINVAL;
> - }
> + struct rvin_dev *vin = dev_get_drvdata(dev);
> + struct rvin_graph_entity *rvge =
> + container_of(asd, struct rvin_graph_entity, asd);
>
> - mbus_cfg->type = v4l2_ep.bus_type;
> + rvge->mbus_cfg.type = vep->bus_type;
>
> - switch (mbus_cfg->type) {
> + switch (rvge->mbus_cfg.type) {
> case V4L2_MBUS_PARALLEL:
> vin_dbg(vin, "Found PARALLEL media bus\n");
> - mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
> + rvge->mbus_cfg.flags = vep->bus.parallel.flags;
> break;
> case V4L2_MBUS_BT656:
> vin_dbg(vin, "Found BT656 media bus\n");
> - mbus_cfg->flags = 0;
> + rvge->mbus_cfg.flags = 0;
> break;
> default:
> vin_err(vin, "Unknown media bus type\n");
> return -EINVAL;
> }
>
> - return 0;
> -}
> -
> -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> -{
> - struct device_node *ep, *np;
> - int ret;
> -
> - vin->digital.asd.match.fwnode.fwnode = NULL;
> - vin->digital.subdev = NULL;
> -
> - /*
> - * Port 0 id 0 is local digital input, try to get it.
> - * Not all instances can or will have this, that is OK
> - */
> - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> - if (!ep)
> - return 0;
> -
> - np = of_graph_get_remote_port_parent(ep);
> - if (!np) {
> - vin_err(vin, "No remote parent for digital input\n");
> - of_node_put(ep);
> - return -EINVAL;
> - }
> - of_node_put(np);
> -
> - ret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg);
> - of_node_put(ep);
> - if (ret)
> - return ret;
> -
> - vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np);
> - vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> + vin->digital = rvge;
>
> return 0;
> }
>
> static int rvin_digital_graph_init(struct rvin_dev *vin)
> {
> - struct v4l2_async_subdev **subdevs = NULL;
> int ret;
>
> - ret = rvin_digital_graph_parse(vin);
> + ret = v4l2_async_notifier_parse_fwnode_endpoint(
> + vin->dev, &vin->notifier, 0, 0,
> + sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> if (ret)
> return ret;
>
> - if (!vin->digital.asd.match.fwnode.fwnode) {
> - vin_dbg(vin, "No digital subdevice found\n");
> - return -ENODEV;
> - }
> -
> - /* Register the subdevices notifier. */
> - subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL);
> - if (subdevs == NULL)
> - return -ENOMEM;
> + if (vin->notifier.num_subdevs > 0)
> + vin_dbg(vin, "Found digital subdevice %pOF\n",
> + to_of_node(
> + vin->notifier.subdevs[0]->match.fwnode.fwnode));
>
> - subdevs[0] = &vin->digital.asd;
> -
> - vin_dbg(vin, "Found digital subdevice %pOF\n",
> - to_of_node(subdevs[0]->match.fwnode.fwnode));
> -
> - vin->notifier.num_subdevs = 1;
> - vin->notifier.subdevs = subdevs;
> vin->notifier.bound = rvin_digital_notify_bound;
> vin->notifier.unbind = rvin_digital_notify_unbind;
> vin->notifier.complete = rvin_digital_notify_complete;
> -
> ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> if (ret < 0) {
> vin_err(vin, "Notifier registration failed\n");
> @@ -290,6 +241,8 @@ static int rcar_vin_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + platform_set_drvdata(pdev, vin);
> +
> ret = rvin_digital_graph_init(vin);
> if (ret < 0)
> goto error;
> @@ -297,11 +250,10 @@ static int rcar_vin_probe(struct platform_device *pdev)
> pm_suspend_ignore_children(&pdev->dev, true);
> pm_runtime_enable(&pdev->dev);
>
> - platform_set_drvdata(pdev, vin);
> -
> return 0;
> error:
> rvin_dma_remove(vin);
> + v4l2_async_notifier_release(&vin->notifier);
>
> return ret;
> }
> @@ -313,6 +265,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
>
> v4l2_async_notifier_unregister(&vin->notifier);
> + v4l2_async_notifier_release(&vin->notifier);
>
> rvin_dma_remove(vin);
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b136844499f6..23fdff7a7370 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -183,7 +183,7 @@ static int rvin_setup(struct rvin_dev *vin)
> /*
> * Input interface
> */
> - switch (vin->digital.code) {
> + switch (vin->digital->code) {
> case MEDIA_BUS_FMT_YUYV8_1X16:
> /* BT.601/BT.1358 16bit YCbCr422 */
> vnmc |= VNMC_INF_YUV16;
> @@ -191,7 +191,7 @@ static int rvin_setup(struct rvin_dev *vin)
> break;
> case MEDIA_BUS_FMT_UYVY8_2X8:
> /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
> - vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
> + vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
> VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> input_is_yuv = true;
> break;
> @@ -200,7 +200,7 @@ static int rvin_setup(struct rvin_dev *vin)
> break;
> case MEDIA_BUS_FMT_UYVY10_2X10:
> /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
> - vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
> + vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ?
> VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
> input_is_yuv = true;
> break;
> @@ -212,11 +212,11 @@ static int rvin_setup(struct rvin_dev *vin)
> dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>
> /* Hsync Signal Polarity Select */
> - if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> + if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> dmr2 |= VNDMR2_HPS;
>
> /* Vsync Signal Polarity Select */
> - if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> + if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> dmr2 |= VNDMR2_VPS;
>
> /*
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index dd37ea811680..b479b882da12 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> struct v4l2_mbus_framefmt *mf = &fmt.format;
> int ret;
>
> - fmt.pad = vin->digital.source_pad;
> + fmt.pad = vin->digital->source_pad;
>
> ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
> if (ret)
> @@ -172,13 +172,13 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
>
> sd = vin_to_source(vin);
>
> - v4l2_fill_mbus_format(&format.format, pix, vin->digital.code);
> + v4l2_fill_mbus_format(&format.format, pix, vin->digital->code);
>
> pad_cfg = v4l2_subdev_alloc_pad_config(sd);
> if (pad_cfg == NULL)
> return -ENOMEM;
>
> - format.pad = vin->digital.source_pad;
> + format.pad = vin->digital->source_pad;
>
> field = pix->field;
>
> @@ -555,7 +555,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
> if (timings->pad)
> return -EINVAL;
>
> - timings->pad = vin->digital.sink_pad;
> + timings->pad = vin->digital->sink_pad;
>
> ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>
> @@ -607,7 +607,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
> if (cap->pad)
> return -EINVAL;
>
> - cap->pad = vin->digital.sink_pad;
> + cap->pad = vin->digital->sink_pad;
>
> ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
>
> @@ -625,7 +625,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> if (edid->pad)
> return -EINVAL;
>
> - edid->pad = vin->digital.sink_pad;
> + edid->pad = vin->digital->sink_pad;
>
> ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>
> @@ -643,7 +643,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> if (edid->pad)
> return -EINVAL;
>
> - edid->pad = vin->digital.sink_pad;
> + edid->pad = vin->digital->sink_pad;
>
> ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 9bfb5a7c4dc4..5382078143fb 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -126,7 +126,7 @@ struct rvin_dev {
> struct v4l2_device v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
> struct v4l2_async_notifier notifier;
> - struct rvin_graph_entity digital;
> + struct rvin_graph_entity *digital;
>
> struct mutex lock;
> struct vb2_queue queue;
> @@ -145,7 +145,7 @@ struct rvin_dev {
> struct v4l2_rect compose;
> };
>
> -#define vin_to_source(vin) vin->digital.subdev
> +#define vin_to_source(vin) ((vin)->digital->subdev)
>
> /* Debug */
> #define vin_dbg(d, fmt, arg...) dev_dbg(d->dev, fmt, ##arg)
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 39a587c6992a..e1a07916b9ca 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -445,6 +445,53 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> }
> EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>
> +int v4l2_async_notifier_parse_fwnode_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + unsigned int port_id, unsigned int endpoint_id, size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_handle *fwnode = NULL;
> + int ret;
> +
> + while ((fwnode = fwnode_graph_get_next_endpoint(
> + dev_fwnode(dev), fwnode))) {
> + struct fwnode_endpoint ep;
> +
> + ret = fwnode_graph_parse_endpoint(fwnode, &ep);
> + if (ret < 0)
> + continue;
> +
> + if (!fwnode_device_is_available(
> + fwnode_graph_get_port_parent(fwnode)))
> + continue;
> +
> + if (ep.port == port_id && ep.id == endpoint_id)
> + break;
> + }
> +
> + if (!fwnode)
> + return -ENOENT;
> +
> + ret = notifier_realloc(notifier, notifier->num_subdevs + 1);
> + if (ret)
> + goto out_err;
> +
> + ret = parse_endpoint(dev, notifier, fwnode, asd_struct_size,
> + parse_single);
> + if (ret)
> + goto out_err;
> +
> + return 0;
> +
> +out_err:
> + fwnode_handle_put(fwnode);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoint);
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 46521e8c8872..6bc7b02b2f46 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -240,4 +240,43 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd));
>
> +/**
> + * v4l2_async_notifier_parse_fwnode_endpoint - Set up async notifier for an
> + * fwnode based sub-device
> + * @dev: @struct device pointer
> + * @notifier: pointer to &struct v4l2_async_notifier
> + * @port_id: port number
> + * @endpoint_id: endpoint number
> + * @asd_struct_size: size of the driver's async sub-device struct, including
> + * sizeof(struct v4l2_async_subdev). The &struct
> + * v4l2_async_subdev shall be the first member of
> + * the driver's async sub-device struct, i.e. both
> + * begin at the same memory address.
> + * @parse_single: driver's callback function called on each V4L2 fwnode endpoint
> + *
> + * Find an endpoint node for the given port and endpoint IDs and allocate an
> + * async sub-device struct for it. Parse the V4L2 fwnode endpoint and call the
> + * provided callback function.
> + *
> + * The function may not be called on a registered notifier.
> + *
> + * Once the user has called this function, the resources released by it need to
> + * be released by callin v4l2_async_notifier_release after the notifier has been
> + * unregistered and the sub-devices are no longer in use.
> + *
> + * A driver supporting fwnode (currently Devicetree and ACPI) should call this
> + * function as part of its probe function before it registers the notifier.
> + *
> + * Return: %0 on success, including when no async sub-devices are found
> + * %-ENOMEM if memory allocation failed
> + * %-EINVAL if graph or endpoint parsing failed
> + * Other error codes as returned by @parse_single
> + */
> +int v4l2_async_notifier_parse_fwnode_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + unsigned int port_id, unsigned int endpoint_id, size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd));
> +
> #endif /* _V4L2_FWNODE_H */
> --
> 2.11.0
>
--
Regards,
Niklas Söderlund
