Hi,
On Thu 19 Apr 2018 at 12:22, Dan Carpenter wrote:
On Thu, Apr 19, 2018 at 11:17:59AM +0100, Rui Miguel Silva wrote:
+static int imx7_csi_link_setup(struct media_entity *entity,
+                              const struct media_pad *local,
+ const struct media_pad *remote, u32 flags)
+{
+ struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+       struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+       struct v4l2_subdev *remote_sd;
+       int ret = 0;
+
+ dev_dbg(csi->dev, "link setup %s -> %s\n", remote->entity->name,
+               local->entity->name);
+
+       mutex_lock(&csi->lock);
+
+       if (local->flags & MEDIA_PAD_FL_SINK) {
+ if (!is_media_entity_v4l2_subdev(remote->entity)) {
+                       ret = -EINVAL;
+                       goto unlock;
+               }
+
+ remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+
+               if (flags & MEDIA_LNK_FL_ENABLED) {
+                       if (csi->src_sd) {
+                               ret = -EBUSY;
+                               goto unlock;
+                       }
+                       csi->src_sd = remote_sd;
+               } else {
+                       csi->src_sd = NULL;
+               }
+
+               goto init;
+       }
+
+       /* source pad */
+       if (flags & MEDIA_LNK_FL_ENABLED) {
+               if (csi->sink) {
+                       ret = -EBUSY;
+                       goto unlock;
+               }
+               csi->sink = remote->entity;
+       } else {
+               v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
+               v4l2_ctrl_handler_init(&csi->ctrl_hdlr, 0);
+               csi->sink = NULL;
+       }
+
+init:
+       if (csi->sink || csi->src_sd)
+               imx7_csi_init(csi);
+       else
+               imx7_csi_deinit(csi);
+
+unlock:
+       mutex_unlock(&csi->lock);
+
+       return 0;

This should be "return ret;" because the failure paths go through here
as well.

Agree.


+}
+
+static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
+                                     struct media_link *link,
+ struct v4l2_subdev_format *source_fmt, + struct v4l2_subdev_format *sink_fmt)
+{
+       struct imx7_csi *csi = v4l2_get_subdevdata(sd);
+       struct v4l2_fwnode_endpoint upstream_ep;
+       int ret;
+
+ ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
+       if (ret)
+               return ret;
+
+ ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
+       if (ret) {
+ v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
+               return ret;
+       }
+
+       mutex_lock(&csi->lock);
+
+       csi->upstream_ep = upstream_ep;
+       csi->is_csi2 = (upstream_ep.bus_type == V4L2_MBUS_CSI2);
+
+       mutex_unlock(&csi->lock);
+
+       return ret;

return 0;

ack.


+}
+

[ snip ]

+
+static int imx7_csi_remove(struct platform_device *pdev)
+{
+       return 0;
+}

There is no need for this empty (struct platform_driver)->remove()
function.  See platform_drv_remove() for how it's called.

right.


This looks nice, though.

Thanks,
---
Cheers,
        Rui

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to