Hi Niklas, On Monday, 8 January 2018 18:42:05 EET Niklas Söderlund wrote: > On 2018-01-08 18:35:23 +0200, Laurent Pinchart wrote: > > On Wednesday, 20 December 2017 17:20:55 EET Niklas Söderlund wrote: > >> On 2017-12-14 17:50:24 +0200, Laurent Pinchart wrote: > >>> On Thursday, 14 December 2017 16:25:00 EET Sakari Ailus wrote: > >>>> On Fri, Dec 08, 2017 at 10:17:36AM +0200, Laurent Pinchart wrote: > >>>>> On Friday, 8 December 2017 03:08:21 EET Niklas Söderlund wrote: > >>>>>> The rcar-vin driver needs to be part of a media controller to > >>>>>> support Gen3. Give each VIN instance a unique name so it can be > >>>>>> referenced from userspace. > >>>>>> > >>>>>> Signed-off-by: Niklas Söderlund > >>>>>> <niklas.soderlund+renesas@xxxxxxxxxxxx> > >>>>>> Reviewed-by: Kieran Bingham > >>>>>> <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> > >>>>>> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >>>>>> --- > >>>>>> > >>>>>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 ++- > >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > >>>>>> 59ec6d3d119590aa..19de99133f048960 100644 > >>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >>>>>> @@ -876,7 +876,8 @@ int rvin_v4l2_register(struct rvin_dev *vin) > >>>>>> vdev->fops = &rvin_fops; > >>>>>> vdev->v4l2_dev = &vin->v4l2_dev; > >>>>>> vdev->queue = &vin->queue; > >>>>>> - strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name)); > >>>>>> + snprintf(vdev->name, sizeof(vdev->name), "%s %s", KBUILD_MODNAME, > >>>>>> + dev_name(vin->dev)); > >>>>> > >>>>> Do we need the module name here ? How about calling them "%s > >>>>> output", dev_name(vin->dev) to emphasize the fact that this is a video > >>>>> node and not a VIN subdev ? This is what the omap3isp and vsp1 drivers > >>>>> do. > >>>>> > >>>>> We're suffering a bit from the fact that V4L2 has never standardized > >>>>> a naming scheme for the devices. It wouldn't be fair to ask you to fix > >>>>> that as a prerequisite to get the VIN driver merged, but we clearly > >>>>> have to work on that at some point. > >>>> > >>>> Agreed, this needs to be stable and I think aligning to what omap3isp > >>>> or vsp1 do would be a good fix here. > >>> > >>> Even omap3isp and vsp1 are not fully aligned, so I think we need to > >>> design a naming policy and document it. > >> > >> I agree that align this is a good idea. And for this reason I chosen to > >> update this patch as such: > >> > >> "%s output", dev_name(vin->dev) > > > > Wouldn't it be easier for userspace to use > > > > "VIN%u output", index > > > > where index is the VIN index as specified in DT ? > > I would be OK whit this, but do this agree with the idea above that we > should try to align this name across drivers? Do you mean does this align with the naming policy that we haven't documented, or even designed, yet ? :-) If you want to start designing that policy, here are a few ideas (that may or may not reflect general consensus). First of all, a few general considerations about the entity name. Entity names shall be unique within the context of a media graph. There have been discussions in the past to move to a single media graph that would span the whole system but no concrete proposal has been submitted. Names thus don't have to be unique system-wide at the moment but this constraint should still be kept in mind. One option would be to simply prepend all entity names with the name of the media device (as defined today) if we later move to a single media graph. Another important point is that entity names should be consumable by both machines and humans (at least by developers). They should thus describe the entity to some extend. A case could be made to make them parsable by machines (for instance using the "input" or "output" suffix to infer both the nature of the entity and its direction) but I don't think that's a good idea. A more formal description of the entity through structured properties would be a better API in my opinion. It could be argued that an "input" or "output" suffix would duplicate information conveyed through those structured properties. While I can't deny that, the need to have unique names readable by developers is met by adding such suffixes. Another point to consider is the prefix. Using the device name (as in dev_name(dev)) is an option but makes reading the media graph by humans more difficult. Using a more ad-hoc naming scheme (that would then be driver- specific to some extend, as is "VIN%u", index for the VIN driver) would create names that are easier to read and interpret, and that would also vary less between SoCs of the same family. I believe there's a balance to consider between readability and avoidance of duplicated information. One could argue that using "VIN" in the name would duplicate information conveyed at the media device level: only the VIN has video nodes, so there's need to make that explicit. Similarly the "OMAP3 ISP" prefix in the OMAP3 ISP media graph could be argued to be redundant, and is to some extend. If I had to redesign the OMAP3 ISP entity naming scheme today I would consider dropping the "OMAP3 ISP" prefix. All of the ISP entities have a name of their own (CCP2, CSI2, CCDC, preview, resizer) that is unique already. The media device reports the device name through the info model field as "TI OMAP3 ISP" so I could argue that the prefix is not needed. However, the graph can also contain external entities (mostly camera sensors in this case). Keeping the "OMAP3 ISP" prefix for all ISP entities would make it clear what the entities belong to, and would also help avoiding duplicate names. In the R-Car case we face a similar situation, as the graph contains, in addition to external camera sensors and video decoders, CSI-2 receivers that are in the SoC but not part of the VIN as such. One possible way to solve this would be to report an entity hierarchy, or at least make it possible to group entities in named groups. The name of the group wouldn't need to be duplicated as a prefix in every entity. > >> I hope this is a step in the correct direction. If not please let me > >> know as soon as possible so I can minimize the trouble for the other > >> developers doing stuff on-top of this series and there test scripts :-) -- Regards, Laurent Pinchart
