Hi Sakari, On Saturday, 2 September 2017 01:57:48 EEST Sakari Ailus wrote: > On Fri, Sep 01, 2017 at 01:28:40PM +0200, Hans Verkuil wrote: > >> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > >> index d063ab4ff67b..dd13604178b4 100644 > >> --- a/include/media/v4l2-fwnode.h > >> +++ b/include/media/v4l2-fwnode.h > >> @@ -249,4 +249,53 @@ 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 > >> + * > >> + * Parse the fwnode endpoint of the @dev device corresponding to the > >> given port > >> + * and endpoint numbres and populate the async sub- devices array of > >> the > > > > numbers > > no space after sub- > > > > > + * notifier. The @parse_endpoint callback function is called for the > > > endpoint > > > > parse_single, but (as in the previous patch) I actually prefer > > parse_endpoint. > > > >> + * with the corresponding async sub-device pointer to let the caller > >> initialize > >> + * the driver-specific part of the async sub-device structure. > >> + * > >> + * The notifier memory shall be zeroed before this function is called > >> on the > >> + * notifier. > > > > Should it? Doesn't this add additional subdevs? > > > > I'm lost. What's the relationship between > > v4l2_async_notifier_parse_fwnode_endpoints and this function? When do you > > use which? When you should zero the notifier? > I thought there would be advantages in this approach as it lets you to > choose which endpoints specifically you want to parse. That said, the > expectation is that the device has no endpoints that aren't supported in > hardware either. > > Some drivers currently iterate over all the endpoints and then validate > them whereas others poke for some endpoints only (port 0, endpoint 0, for > the rcar-vin driver, for instance). In DT binding documentation the > endpoint numbers are currently not specified nor drivers have checked them. > Common sense tells to use zero if there's no reason to do otherwise, but > still this hasn't been documented nor validated in the past. So if we add > that now, there could be a chance of breaking something. > > Additionally, specifying the endpoints to parse explicitly has been seen > beneficial (or even necessary) in parsing endpoints for devices that have > both input and output interfaces. Perhaps Niklas can comment on that. > > What I though was to introduce a specific error code (EPERM, better > suggestions are taken) Maybe ENOTCONN ? > for the driver callback function (parse_endpoint) to silently skip endpoints > the driver doesn't like for reason or another. This lets drivers to use the > endpoint parser function added by the previous patch and still maintain the > old behaviour, i.e. ignore endpoints that aren't explicitly recognised by > the driver. > > I'll drop this patch from the next version. Parsing a specific endpoint of a specific port is probably indeed a bit too fine-grained, but I think there are use cases for parsing at the port level instead of parsing all ports. -- Regards, Laurent Pinchart
