lkml.org 
[lkml]   [2022]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with port management
    Hi Sakari,

    On Fri 04 Mar 22, 00:43, Sakari Ailus wrote:
    > Hi Paul,
    >
    > On Wed, Mar 02, 2022 at 03:59:50PM +0100, Paul Kocialkowski wrote:
    > > > > +static int
    > > > > +sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
    > > > > + struct v4l2_subdev *remote_subdev,
    > > > > + struct v4l2_async_subdev *async_subdev)
    > > > > +{
    > > > > + struct sun6i_csi_device *csi_dev =
    > > > > + container_of(notifier, struct sun6i_csi_device,
    > > > > + bridge.notifier);
    > > > > + struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
    > > > > + struct sun6i_csi_bridge_source *source = NULL;
    > > > > + struct fwnode_handle *fwnode = dev_fwnode(csi_dev->dev);
    > > > > + struct fwnode_handle *handle = NULL;
    > > > > + bool enabled;
    > > > > + int ret;
    > > > > +
    > > > > + while ((handle = fwnode_graph_get_next_endpoint(fwnode, handle))) {
    > > >
    > > > I'd instead store the information you need here in struct sun6i_csi_bridge.
    > > > You could remove the loop here.
    > >
    > > Is there a different method for matching a remote subdev to a local port?
    > > The rationale here is that I need the handle for fwnode_graph_parse_endpoint
    > > but cannot get that handle from the remote subdev's fwnode pointer directly.
    >
    > You generally shouldn't try to match fwnodes here as the V4L2 async
    > framework has already done that job. This information can be found behind
    > the async_subdev pointer.
    >
    > See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2-main.c for an example.

    Thanks for the feedback, I'll look into that.

    > >
    > > > > + struct fwnode_endpoint endpoint = { 0 };
    > > > > + struct fwnode_handle *remote_fwnode;
    > > > > +
    > > > > + remote_fwnode = fwnode_graph_get_remote_port_parent(handle);
    > > > > + if (!remote_fwnode)
    > > > > + continue;
    > > > > +
    > > > > + if (remote_fwnode != remote_subdev->fwnode)
    > > > > + goto next;
    > > > > +
    > > > > + ret = fwnode_graph_parse_endpoint(handle, &endpoint);
    > > > > + if (ret < 0)
    > > > > + goto next;
    > > > > +
    > > > > + switch (endpoint.port) {
    > > > > + case SUN6I_CSI_PORT_PARALLEL:
    > > > > + source = &bridge->source_parallel;
    > > > > + enabled = true;
    > > > > + break;
    > > > > + default:
    > > > > + break;
    > > > > + }
    > > > > +
    > > > > +next:
    > > > > + fwnode_handle_put(remote_fwnode);
    > > > > + }
    > > > > +
    > > > > + if (!source)
    > > > > + return -EINVAL;
    > > > > +
    > > > > + source->subdev = remote_subdev;
    > > > > +
    > > > > + return sun6i_csi_bridge_link(csi_dev, SUN6I_CSI_BRIDGE_PAD_SINK,
    > > > > + remote_subdev, enabled);
    > > > > +}
    > > > > +
    > > > > +static int
    > > > > +sun6i_csi_bridge_notifier_complete(struct v4l2_async_notifier *notifier)
    > > > > +{
    > > > > + struct sun6i_csi_device *csi_dev =
    > > > > + container_of(notifier, struct sun6i_csi_device,
    > > > > + bridge.notifier);
    > > > > +
    > > > > + return sun6i_csi_v4l2_complete(csi_dev);
    > > >
    > > > You could call v4l2_device_register_subdev_nodes() here.
    > >
    > > That's definitely what sun6i_csi_v4l2_complete does (the diff is probably not
    > > very clear). Note that the wrapper is extended later on to register the capture
    > > video device for the no-isp path.
    >
    > I could be missing something... Do you need to call
    > sun6i_csi_v4l2_complete() in multiple places or not? If not, then I think
    > it'd be probably better to just move the code here.

    No this is only called here so I guess we can avoid it entirely.

    Thanks,

    Paul

    > >
    > > Maybe the capture registration could be kept in sun6i_csi_probe for the non-isp
    > > path and then the wrapper wouldn't be needed. I don't mind either way.
    >
    > --
    > Kind regards,
    >
    > Sakari Ailus

    --
    Paul Kocialkowski, Bootlin
    Embedded Linux and kernel engineering
    https://bootlin.com
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2022-03-04 10:00    [W:3.342 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site