Re: Samsung i2c subdev drivers that set sd->name

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Thursday 04 July 2013 22:19:20 Sylwester Nawrocki wrote:
> On 07/04/2013 01:13 PM, Hans Verkuil wrote:
> > On Thu 4 July 2013 00:49:36 Laurent Pinchart wrote:
> >> On Thursday 27 June 2013 11:53:15 Sylwester Nawrocki wrote:
> >>> On 06/27/2013 08:43 AM, Hans Verkuil wrote:
> >>>> On Wed June 26 2013 11:00:51 Sakari Ailus wrote:
> >>>>> On Tue, Jun 25, 2013 at 06:55:49PM +0200, Sylwester Nawrocki wrote:
> >>>>>> On 06/24/2013 10:54 AM, Hans Verkuil wrote:

[snip]

> >>>>>> Before we start messing with those drivers it would be nice to have
> >>>>>> defined rules of the media entity naming. I2C bus number and address
> >>>>>> is not something that's useful in the media entity name. And multiple
> >>>>> 
> >>>>> Isn't it?
> >>>> 
> >>>> Why not? As long as the format is strictly adhered to then I see no
> >>>> reason not to use it. Not only does it make the name unique, it also
> >>>> tells you where the device is in the hardware topology.
> >> 
> >> It's a shame that entities don't have a bus info field in addition to
> >> their name, but we have to live with that.
> >> 
> >> Userspace needs a way to distinguish between multiple identical subdevs.
> >> We can't rely on IDs only, as they're not guaranteed to be stable. We
> >> thus need to use names and possibly connection information.
> >> 
> >> Two identical sensors connected to separate receivers could be
> >> distinguished by checking which receiver they're connected to.
> >> Unfortunately this breaks when the two sensors are connected to the same
> >> receiver, in which case we can only rely on the name. Media entity names
> >> thus need to be unique when connection information can't help
> >> distinguishing otherwise identical subdevs, which implies that subdev
> >> names must be unique.
> >> 
> >>>> We could make the simple rule that the driver name is the first word of
> >>>> the name. So it would be easy to provide a function that matches just
> >>>> the first word and ignores the bus info (if there is any).
> >>> 
> >>> Yes, and that's basically all I needed before "fixing" those affected
> >>> drivers. No matter what exact rules, if there are any, user space could
> >>> handle various hardware configurations without issues.
> >>> 
> >>> Besides, the drivers would need to strip/replace with something else any
> >>> spaces when initializing subddev name, as that character would be used
> >>> as the bus info delimiter ?
> >> 
> >> Or we could decide that the bus info can't contain any space, in which
> >> case the last space would be the delimiter.
> 
> Sounds reasonable as well.
> 
> > Frankly, I don't think either should contain a space :-) Today nobody is
> > using spaces anywhere to the best of my knowledge.
> 
> OK, then there would be spaces neither in <name> nor in <bus-info>. From
> a quick grep I can't see any driver currently using spaces in its subdev
> name.

In case of multi-subdev sensors (when the sensor includes a scaler for 
instance) the subdev names will likely be made of the sensor name (or driver 
name) and a subdev description. Something like "xxxxx pixel array" and "xxxxxx 
scaler". We could use a dash or underscore to replace spaces though.

> >>> Then in media-ctl or any user space code the entity name could be
> >>> matched exactly, and if that fails a fuzzy matching could be done, with
> >>> the bus info discarded.
> >> 
> >> That's a good idea, it would help making media-ctl based scripts more
> >> portable.
> >> 
> >>>>> Well... there's currently no other way to figure out which I2C bus and
> >>>>> address the device has, to find the I2C device. It'd be very, very
> >>>>> good if entities had bus information which is currently is limited to
> >>>>> the media device itself.
> >>>>> 
> >>>>> But beyond that I see no use for it.
> >>>> 
> >>>> I don't really care all that much how the name is made unique, as long
> >>>> as it is. It's used in the kernel log as a prefix and it is used for
> >>>> async loading of drivers. Probably there are other uses as well.
> >> 
> >> I actually care, as we need to provide a meaningful way for userspace to
> >> relate those unique names to the hardware instance they correspond to.
> >> 
> >>>> The problem by taking a shortcut now is that *when* somebody uses two
> >>>> identical sensors he'll uncover a big mess that needs to be cleaned up.
> >>> 
> >>> True, I'm not against fixing it, I'd like to respect your name
> >>> uniqueness rule. :) But I'm against modifying drivers in a way that
> >>> doesn't give user space a chance to handle it correctly.
> >>> 
> >>>> As an aside: perhaps we should start making checklists for subdev
> >>>> drivers for developers. Ensuring that the subdev name is unique would
> >>>> be one of them.
> >>> 
> >>> Sounds good.
> >>> 
> >>>>>> sensors (smiapp, s5c73m3, upcoming s5k6bafx) have "logical" subdevs
> >>>>>> that are not initialized with the i2c specific v4l2 functions.
> >>>>>> 
> >>>>>> I guess there are other means to ensure the subdev's name is unique,
> >>>>>> rather than appending I2C bus info to it that changes from board to
> >>>>>> board and is totally irrelevant in user space.
> >>>>> 
> >>>>> There may be cases where the same board contains two sensors that are
> >>>>> exactly similar (think of stereo cameras!) but the user still must
> >>>>> know which one is which. I2C bus information might not be that bad way
> >>>>> to tell it.
> >>>>> 
> >>>>> But I don't think it necessarily should be part of the subdev's name.
> >>>> 
> >>>> If you mean that the i2c bus info doesn't have to be part of the
> >>>> subdev's name, then that's correct. But it does have to be unique. It's
> >>>> how it was designed. Since I designed it, I should know :-)
> >> 
> >> How should bus info be retrieved if it's not part of the media entity
> >> name ?
> > 
> > If that subdev name is also going to be used in the MC, then yes, it
> > should contain the i2c bus info. At the moment the v4l2 core makes no
> > assumptions on the subdev name, other than that it must be unique. which
> > is generally achieved by appending the i2c bus info. But some platform
> > subdevs (non-i2c) may not have any bus info since that doesn't apply in
> > all cases.
> > 
> > I would propose a guideline for the subdev naming like this:
> > 	<name>  <bus-info>
> > 
> > where <bus-info> is optional and neither string contains spaces.
> 
> Hmm, it might be inconvenient for platform subdevs. E.g. it could mean
> something like:
> 
> currently             | <name> <bus-info>
> ----------------------+------------------------------------------
> s5p-mipi-csis.0       | s5p-mipi-csis 11800000.csis
> s5p-mipi-csis.1       | s5p-mipi-csis 11810000.csis
> FIMC-LITE.0           | FIMC-LITE 12040000.fimc-lite
> FIMC-LITE.0           | FIMC-LITE 12050000.fimc-lite
> 
> 
> The register window addresses can vary across various SoCs and it doesn't
> sound very clever to expose that to user space, when a device is exactly
> same from the user point of view.
> 
> Presumably the ".<index>" part in the names in the above cases should be
> kept, and user space could just ignore bus-info, e.g.
> 
> s5p-mipi-csis.0       | s5p-mipi-csis.0 11800000.csis
> FIMC-LITE.0           | FIMC-LITE.0 12050000.fimc-lite
> 
> If the bus info is too long it would get truncated.

We're limited to 32 characters, which isn't much to store both the name and 
bus info.

> >>> While we are at it, how about v4l2_i2c_subdev_init() ? It initializes
> >>> sd->name with SPI driver name. It doesn't look like it could be unique
> >>> then ?
> >>> 
> >>>>>> Presumably we could have subdev name postfixed with I2C bus id/slave
> >>>>>> address as it is done currently and the media core would be using
> >>>>>> only a part of subdev's name up to ' ' character to initialize the
> >>>>>> entity name ?
> >>>> 
> >>>> Yes, that's an option. But I would like Laurent's opinion on this. The
> >>>> problem I see with that is that it would actually make it hard to map
> >>>> an entity name to a subdev since there is no bus_info information
> >>>> associated with the entity, just an ID.
> >>> 
> >>> Yes, without bus info in the entity structure this would likely not be a
> >>> good idea.
> >> 
> >> As explained above, userspace needs to know which entity corresponds to
> >> which piece of hardware, so non-unique (in the context of a media
> >> device, and when connection information doesn't provide the required
> >> information) entity names are a bad idea in the general case.
> >> 
> >>>> So if you have two identical subdevs, e.g. "saa7115 6-0021" and
> >>>> "saa7115 7-0021", and you name the corresponding entities "saa7115",
> >>>> but with different IDs, then how do you know which ID maps to which
> >>>> subdev? If you keep the i2c postfix, then that's unambiguous.
> >>> 
> >>> The I2C bus info in the subdev's name can be a completely random string.
> >>> Please note that I2C bus id can be assigned dynamically. So there is no
> >>> guarantee you get reproducible bus IDs assigned to each sensor in all
> >>> cases. That's said I2C bus info is not reliable means to identify
> >>> physical
> >>> device.
> >> 
> >> I'm afraid you're right :-) (I don't know whether I2C bus IDs will be
> >> assigned dynamically in practice on systems where the information is
> >> important though).
> > 
> > i2c devices on an embedded system (i.e. hooked up to the SoC i2c bus) will
> > always get the same bus number. Obviously, if the i2c device is on a
> > PCI(e) or USB board,
>
> That has not always been true, before patch [1] most drivers used to
> register I2C adapters with dynamically assigned IDs. Now there is a standard
> way to specify the adapter's id in DT.
> 
> > then it becomes dynamic (but still unique, and still it specifies exactly
> > where the device can be found in the hardware topology).
> 
> Presumably it allows to locate exactly a specific hardware device
> indirectly, by e.g. parsing some additional data from sysfs. But it is not
> very useful as an absolute identifier of a device.
> 
> Perhaps a sysfs link would have been a better way to expose the media
> entity's underlying device, its placement in the hardware topology, etc. But
> not all subdevs have struct device associated with them, not all have /dev
> entry. Perhaps the entities could be listed in sysfs under corresponding
> media device, with relevant bus information associated with them.

I'd rather not get started with the whole "media controller should have been 
implemented in sysfs" discussion again :-)

We need an ioctl to report additional information about media entities (it's 
been on my to-do list for wayyyyyyyyy too long). It could be used to report 
bus information as well.

> > In other words, the i2c bus info is by no means a random string.
> > 
> >> If we can't use the bus info then I see few options other than getting
> >> the name directly from platform data or DT. We could use the full device
> >> path, but
>
> AFAIK it is valid to put in DT information required for user visible labels
> identifying parts of hardware.
> 
> >> that will become too long for the media entity and subdev name fields.
> 
> Yes, that wouldn't work I'm afraid.
> 
> >>>> The problem is that the entity documentation gives no guidelines as to
> >>>> what can be expected of the entity name. In my opinion the entity name
> >>>> should be copied from the subdev name, thus making it unique (at least
> >>>> between subdevs). In addition, the first word of the name should be the
> >>>> driver name, the remainder is the identifier (usually the i2c bus).
> >>> 
> >>> Sounds reasonable. The specific use case this causes problems to us is
> >>> when there are multiple revisions of similar product, where same sensor
> >>> is on different I2C busses. Either physically or on a device tree based
> >>> system, where bus IDs can be assigned dynamically.
> >>> 
> >>> Then same sensor will have different media entity names, and without
> >>> some rules it quickly becomes impossible to specify pipeline
> >>> configuration in, e.g. text file. This makes the media controller
> >>> drivers even less portable.
> >>> 
> >>>>>> The media entities have unique ID, hence it would have probably been
> >>>>>> OK to have entities with same name, should it happen there are
> >>>>>> multiple identical devices in a single system.
> >>>> 
> >>>> Actually, from what I remember the name was just a way to make things
> >>>> more understandable for humans and the ID was meant to be used as the
> >>>> real identifier. I'm not 100% sure that that was the idea behind the
> >>>> original design, I would have to go back to my first RFCs to confirm
> >>>> that.
> >>>> 
> >>>> But since that time there has been a movement inside the kernel away
> >>>> from numerical IDs towards unique strings. So if I were to design it
> >>>> today I would definitely specify that the entity name must be unique,
> >>>> at least within the set of entities of the same type.
> >>> 
> >>> Not sure such uniqueness would be much useful as long as those names are
> >>> random.
> >> 
> >> Uniqueness will only be useful if we have a way to relate names to
> >> hardware device instances. If that relationship is provide through a
> >> different API then there's not much added value in having unique media
> >> entity names.
> >> 
> >>>>>> To summarize, I would prefer to avoid modifying those drivers in a
> >>>>>> backward incompatible way, for a sake of pure API correctness and
> >>>>>> due to vague reasons. There is currently no board in mainline for
> >>>>>> which the subdev names wouldn't have been unique. Usually there
> >>>>>> are different types of image sensors used for the front and the
> >>>>>> rear facing camera. But for stereoscopy there most likely would
> >>>>>> be two identical image sensors on a board.
> >>>> 
> >>>> This isn't about what it in the mainline. If you make a product that
> >>>> uses two identical sensor drivers then you will reuse the sensor driver
> >>>> code but you will not typically try to upstream your bridge driver
> >>>> since that's unique for your product and generally useless for anyone
> >>>> else.
> >>> 
> >>> Not sure if that's a "proper" philosophy, in general there is likely
> >>> plenty out of tree drivers. But if everyone thought like this we would
> >>> have very little drivers in mainline. And little chances to adapt the
> >>> core frameworks to the needs of those "unique" devices. Resulting in
> >>> various incompatible forks of the core frameworks.
> >>> 
> >>> However I see you point we shouldn't come up with a code that is known
> >>> to possibly cause problems.
> >>> 
> >>>> Clean subdev drivers using the API correctly *are* important to promote
> >>>> reuse. I would like to fix the non-Samsung, non-smiapp subdev drivers
> >>>> soon. With regards to the Samsung/smiapp drivers we need at the very
> >>>> least a comment in the driver mentioning that they behave in a non-
> >>>> standard way with possible complications if there are more than one of
> >>>> them in a system. (Frankly, that's a particular concern for the smiapp
> >>>> driver. I do think that it would be good if that one can be fixed
> >>>> soon).
> >>> 
> >>> I can prepare patches for all the affected Samsung device drivers,
> >>> reverting back the I2C bus info postfix. No need to add any ugly
> >>> comments to them :)
> >>> 
> >>> I not sure what exactly are the reasons smiapp chose not to postfix the
> >>> name with I2C bus info like v4l2_i2c_subdev_init() does. Presumably
> >>> this driver could be modified to do that, if there is chance to handle
> >>> it in standard way in user space.
> 
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e
> e5c27440cc24d62ec463cce4c000bb32c5692c7

-- 
Regards,

Laurent Pinchart