Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:30 EET Niklas Söderlund wrote:
> The parsing and registering CSI-2 subdevices with the v4l2 async
> framework is a collaborative effort shared between the VIN instances
> which are part of the group. When the last VIN in the group is probed it
> asks all other VINs to parse its share of OF and record the async
> subdevices it finds in the notifier belonging to the last probed VIN.
>
> Once all CSI-2 subdevices in this notifier are bound proceed to register
> all VIN video devices of the group and crate media device links between
> all CSI-2 and VIN entities according to the SoC specific routing
> configuration.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 250 ++++++++++++++++++++++++-
> drivers/media/platform/rcar-vin/rcar-vin.h | 12 +-
> 2 files changed, 258 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 4a64df5019ce45f7..f08277a0dc11f477 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -27,6 +27,23 @@
>
> #include "rcar-vin.h"
>
> +/*
> + * The companion CSI-2 receiver driver (rcar-csi2) is known
> + * and we know it have one source pad (pad 0) and four sink
s/have/has/
> + * pads (pad 1-4). So to translate a pad on the remote
> + * CSI-2 receiver to/from the VIN internal channel number simply
> + * subtract/add or one from the pad/chan number.
s/or one/one/
and maybe s/chan/channel/ ?
> + */
> +#define rvin_group_csi_pad_to_chan(pad) ((pad) - 1)
> +#define rvin_group_csi_chan_to_pad(chan) ((chan) + 1)
> +
> +/*
> + * Not all VINs are created equal, master VINs control the
> + * routing for other VIN's. We can figure out which VIN is
> + * master by looking at a VINs id
> + */
> +#define rvin_group_id_to_master(vin) ((vin) < 4 ? 0 : 4)
> +
> /* ------------------------------------------------------------------------
> * Gen3 CSI2 Group Allocator
> */
> @@ -77,6 +94,8 @@ static int rvin_group_init(struct rvin_group *group,
> struct rvin_dev *vin) snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> "platform:%s",
> dev_name(mdev->dev));
>
> + group->notifier = NULL;
> +
The group has been allocated with kzalloc() so this isn't necessary.
> media_device_init(mdev);
>
> ret = media_device_register(&group->mdev);
> @@ -406,6 +425,218 @@ static int rvin_digital_graph_init(struct rvin_dev
> *vin) return 0;
> }
>
> +/* ------------------------------------------------------------------------
> + * Group async notifier
> + */
> +
> +static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct rvin_dev *vin = notifier_to_vin(notifier);
> + const struct rvin_group_route *route;
> + unsigned int i;
> + int ret;
> +
> + ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> + if (ret) {
> + vin_err(vin, "Failed to register subdev nodes\n");
> + return ret;
> + }
> +
> + /* Register all video nodes for the group */
> + for (i = 0; i < RCAR_VIN_NUM; i++) {
> + if (vin->group->vin[i]) {
> + ret = rvin_v4l2_register(vin->group->vin[i]);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + /* Create all media device links between VINs and CSI-2's */
> + mutex_lock(&vin->group->lock);
> + for (route = vin->info->routes; route->mask; route++) {
> + struct media_pad *source_pad, *sink_pad;
> + struct media_entity *source, *sink;
> + unsigned int source_idx;
> +
> + /* Check that VIN is part of the group */
> + if (!vin->group->vin[route->vin])
> + continue;
> +
> + /* Check that VIN' master is part of the group */
> + if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> + continue;
> +
> + /* Check that CSI-2 is part of the group */
> + if (!vin->group->csi[route->csi].subdev)
> + continue;
> +
> + source = &vin->group->csi[route->csi].subdev->entity;
> + source_idx = rvin_group_csi_chan_to_pad(route->chan);
> + source_pad = &source->pads[source_idx];
> +
> + sink = &vin->group->vin[route->vin]->vdev.entity;
> + sink_pad = &sink->pads[0];
> +
> + /* Skip if link already exists */
> + if (media_entity_find_link(source_pad, sink_pad))
> + continue;
> +
> + ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> + if (ret) {
> + vin_err(vin, "Error adding link from %s to %s\n",
> + source->name, sink->name);
> + break;
> + }
> + }
> + mutex_unlock(&vin->group->lock);
> +
> + return ret;
> +}
> +
> +static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct rvin_dev *vin = notifier_to_vin(notifier);
> + unsigned int i;
> +
> + for (i = 0; i < RCAR_VIN_NUM; i++)
> + if (vin->group->vin[i])
> + rvin_v4l2_unregister(vin->group->vin[i]);
> +
> + mutex_lock(&vin->group->lock);
> +
> + for (i = 0; i < RVIN_CSI_MAX; i++) {
> + if (vin->group->csi[i].fwnode != asd->match.fwnode)
> + continue;
> + vin->group->csi[i].subdev = NULL;
> + vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
> + break;
> + }
> +
> + mutex_unlock(&vin->group->lock);
> +}
> +
> +static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct rvin_dev *vin = notifier_to_vin(notifier);
> + unsigned int i;
> +
> + mutex_lock(&vin->group->lock);
> +
> + for (i = 0; i < RVIN_CSI_MAX; i++) {
> + if (vin->group->csi[i].fwnode != asd->match.fwnode)
> + continue;
> + vin->group->csi[i].subdev = subdev;
> + vin_dbg(vin, "Bound CSI-2 %s to slot %u\n", subdev->name, i);
> + break;
> + }
> +
> + mutex_unlock(&vin->group->lock);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations rvin_group_notify_ops =
> {
> + .bound = rvin_group_notify_bound,
> + .unbind = rvin_group_notify_unbind,
> + .complete = rvin_group_notify_complete,
> +};
> +
> +static int rvin_mc_parse_v4l2(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
As this parses one endpoint, maybe rvin_mc_parse_of_endpoint() ?
> +{
> + struct rvin_dev *vin = dev_get_drvdata(dev);
> + struct v4l2_async_notifier *notifier = vin->group->notifier;
> + unsigned int i;
> +
> + if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> + return -EINVAL;
> +
> + if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> + vin_dbg(vin, "Subdevice %pOF disabled, ignoring\n",
> + to_of_node(asd->match.fwnode));
It's not the subdevice that is disabled, it's the OF node, so I'd write this
"OF device %pOF disabled, ignoring".
> + return -ENOTCONN;
> +
> + }
> +
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + if (notifier->subdevs[i]->match.fwnode == asd->match.fwnode) {
> + vin_dbg(vin, "Subdevice %pOF already handled\n",
> + to_of_node(asd->match.fwnode));
Ditto.
> + return -ENOTCONN;
> + }
> + }
Do you need a loop, or could you just test if vin->group->csi[vep-
>base.id].fwnode is non-NULL ?
> + vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
> +
> + vin_dbg(vin, "Add group subdevice %pOF to slot %u\n",
> + to_of_node(asd->match.fwnode), vep->base.id);
And here too.
> + return 0;
> +}
> +
> +static int rvin_mc_try_parse(struct rvin_dev *vin)
"try" parse ? Do you really expect this to be so difficult that we'll likely
fail ? :-) How about rvin_mc_parse_of_graph() ?
> +{
> + unsigned int i, count = 0;
I'd split that on two lines.
> + int ret;
> +
> + mutex_lock(&vin->group->lock);
> +
> + /* If there already is a notifier something have gone wrong, bail */
s/have/has/
and I assume you mean "bail out", not "bail".
> + if (WARN_ON(vin->group->notifier)) {
> + mutex_unlock(&vin->group->lock);
> + return -EINVAL;
> + }
> +
> + /* If not all VIN's are registered don't register the notifier */
> + for (i = 0; i < RCAR_VIN_NUM; i++)
> + if (vin->group->vin[i])
> + count++;
> +
> + if (vin->group->count != count) {
> + mutex_unlock(&vin->group->lock);
> + return 0;
> + }
> +
> + /*
> + * Have all VIN's look for subdevices. Some subdevices will overlap
> + * but the parser function can handle it, so each subdevice will
> + * only be registered once with the notifier
Do you have anything particular against periods at end of sentences (and this
also applies to single-sentence comments) ? If not, this comment applies to
the whole series.
> + */
> +
> + vin->group->notifier = &vin->notifier;
> +
> + for (i = 0; i < RCAR_VIN_NUM; i++) {
> + if (!vin->group->vin[i])
> + continue;
> +
> + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> + vin->group->vin[i]->dev, vin->group->notifier,
> + sizeof(struct v4l2_async_subdev), 1,
> + rvin_mc_parse_v4l2);
> + if (ret) {
> + mutex_unlock(&vin->group->lock);
> + return ret;
> + }
> + }
I'll refrain from telling how much this makes me want to cry. I know we have
no other choice for now, but still :(
> + mutex_unlock(&vin->group->lock);
> +
> + vin->group->notifier->ops = &rvin_group_notify_ops;
> +
> + ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> + if (ret < 0) {
> + vin_err(vin, "Notifier registration failed\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int rvin_mc_init(struct rvin_dev *vin)
> {
> int ret;
> @@ -419,7 +650,15 @@ static int rvin_mc_init(struct rvin_dev *vin)
> if (ret)
> return ret;
>
> - return rvin_group_get(vin);
> + ret = rvin_group_get(vin);
> + if (ret)
> + return ret;
> +
> + ret = rvin_mc_try_parse(vin);
> + if (ret)
> + rvin_group_put(vin);
> +
> + return ret;
> }
>
> /* ------------------------------------------------------------------------
> @@ -539,10 +778,15 @@ static int rcar_vin_remove(struct platform_device
> *pdev) v4l2_async_notifier_unregister(&vin->notifier);
> v4l2_async_notifier_cleanup(&vin->notifier);
>
> - if (vin->info->use_mc)
> + if (vin->info->use_mc) {
> + mutex_lock(&vin->group->lock);
> + if (vin->group->notifier == &vin->notifier)
> + vin->group->notifier = NULL;
> + mutex_unlock(&vin->group->lock);
> rvin_group_put(vin);
> - else
> + } else {
> v4l2_ctrl_handler_free(&vin->ctrl_handler);
> + }
>
> rvin_dma_unregister(vin);
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> ca2c2a23cef8506c..6cef78df42047c8c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -207,9 +207,13 @@ struct rvin_dev {
> *
> * @mdev: media device which represents the group
> *
> - * @lock: protects the count and vin members
> + * @lock: protects the count, notifier, vin and csi members
> * @count: number of enabled VIN instances found in DT
> + * @notifier: pointer to the notifier of a VIN which handles the
> + * groups async sub-devices.
> * @vin: VIN instances which are part of the group
> + * @csi: array of pairs of fwnode and subdev pointers
> + * to all CSI-2 subdevices.
> */
> struct rvin_group {
> struct kref refcount;
> @@ -218,7 +222,13 @@ struct rvin_group {
>
> struct mutex lock;
> unsigned int count;
> + struct v4l2_async_notifier *notifier;
> struct rvin_dev *vin[RCAR_VIN_NUM];
> +
> + struct {
> + struct fwnode_handle *fwnode;
> + struct v4l2_subdev *subdev;
> + } csi[RVIN_CSI_MAX];
> };
>
> int rvin_dma_register(struct rvin_dev *vin, int irq);
With these small issues fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
We clearly need a new review tag to tell "this is horrible but I agree to let
it in" :-/
--
Regards,
Laurent Pinchart
