Samsung i2c subdev drivers that set sd->name

Samsung i2c subdev drivers that set sd->name

[Hans Verkuil]

Hi Sylwester,

It came to my attention that several i2c subdev drivers overwrite the sd->name
as set by v4l2_i2c_subdev_init with a custom name.

This is wrong if it is possible that there are multiple identical sensors in
the system. The sd->name must be unique since it is used to prefix kernel
messages etc, so you have to be able to tell the sensor devices apart.

It concerns the following Samsung-contributed drivers:

drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));

If there can be only one sensor (because it is integrated in the SoC),
then there is no problem with doing this. But it is not obvious to me
which of these drivers are for integrated systems, and which aren't.

I can make patches for those that need to be fixed if you can tell me
which drivers are affected.

Regards,

	Hans

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

[Sylwester Nawrocki]

Hi Hans,

Cc: Laurent and Sakari

On 06/24/2013 10:54 AM, Hans Verkuil wrote:
> Hi Sylwester,
> 
> It came to my attention that several i2c subdev drivers overwrite the sd->name
> as set by v4l2_i2c_subdev_init with a custom name.
> 
> This is wrong if it is possible that there are multiple identical sensors in
> the system. The sd->name must be unique since it is used to prefix kernel
> messages etc, so you have to be able to tell the sensor devices apart.

This has been discussed in the past, please see thread [1]. 

> It concerns the following Samsung-contributed drivers:
> 
> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));

It seems ov9650 is missing on this list,

$ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
drivers/media/i2c/ov9650.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name, code->pad, code->index);
drivers/media/i2c/smiapp/smiapp-core.c: strlcpy(subdev->name, sensor->minfo.name, sizeof(subdev->name));
drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));

> If there can be only one sensor (because it is integrated in the SoC),
> then there is no problem with doing this. But it is not obvious to me
> which of these drivers are for integrated systems, and which aren't.

Those sensors are standalone devices, I'm not aware of any of them to be 
integrated with an Application Processor SoC. I've never seen something 
like that. However some of those devices are hybrid modules with a raw 
image sensor and an ISP SoC.
So in theory there could be multiple such devices in a single system, 
although personally I've never seen something like that.

> I can make patches for those that need to be fixed if you can tell me
> which drivers are affected.

You may want to have a look at the commits listed below, and the comments 
I received to that [2] patch series...

commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
[media] noon010pc30: Make subdev name independent of the I2C slave address

commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
[media] s5k6aa: Make subdev name independent of the I2C slave address

commit c5024a70bb60b678f08586ed786340ec162d250f
[media] m5mols: Make subdev name independent of the I2C slave address

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
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.

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 ? 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.

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. 

Regards,
Sylwester

[1] http://www.spinics.net/lists/linux-media/msg58437.html
[2] http://www.spinics.net/lists/linux-media/msg44445.html

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

[Sakari Ailus]

Hi Sylwester and Hans,

On Tue, Jun 25, 2013 at 06:55:49PM +0200, Sylwester Nawrocki wrote:
> Hi Hans,
> 
> Cc: Laurent and Sakari
> 
> On 06/24/2013 10:54 AM, Hans Verkuil wrote:
> > Hi Sylwester,
> > 
> > It came to my attention that several i2c subdev drivers overwrite the sd->name
> > as set by v4l2_i2c_subdev_init with a custom name.
> > 
> > This is wrong if it is possible that there are multiple identical sensors in
> > the system. The sd->name must be unique since it is used to prefix kernel
> > messages etc, so you have to be able to tell the sensor devices apart.
> 
> This has been discussed in the past, please see thread [1]. 
> 
> > It concerns the following Samsung-contributed drivers:
> > 
> > drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
> > drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> > drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> 
> It seems ov9650 is missing on this list,
> 
> $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> drivers/media/i2c/ov9650.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name, code->pad, code->index);
> drivers/media/i2c/smiapp/smiapp-core.c: strlcpy(subdev->name, sensor->minfo.name, sizeof(subdev->name));

For smiapp the issue is that smiapp is the name of the driver; there's no
sensor which would be called "smiapp" but a large number of different
devices that implement the SMIA or SMIA++ standard. The driver can recognise
some of them and call them according to their real name.

> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> 
> > If there can be only one sensor (because it is integrated in the SoC),
> > then there is no problem with doing this. But it is not obvious to me
> > which of these drivers are for integrated systems, and which aren't.
> 
> Those sensors are standalone devices, I'm not aware of any of them to be 
> integrated with an Application Processor SoC. I've never seen something 
> like that. However some of those devices are hybrid modules with a raw 
> image sensor and an ISP SoC.
> So in theory there could be multiple such devices in a single system, 
> although personally I've never seen something like that.
> 
> > I can make patches for those that need to be fixed if you can tell me
> > which drivers are affected.
> 
> You may want to have a look at the commits listed below, and the comments 
> I received to that [2] patch series...
> 
> commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
> [media] noon010pc30: Make subdev name independent of the I2C slave address
> 
> commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
> [media] s5k6aa: Make subdev name independent of the I2C slave address
> 
> commit c5024a70bb60b678f08586ed786340ec162d250f
> [media] m5mols: Make subdev name independent of the I2C slave address
> 
> 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?

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.

> 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.

> 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 ? 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.
> 
> 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. 

:-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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

[Hans Verkuil]

On Wed June 26 2013 11:00:51 Sakari Ailus wrote:
> Hi Sylwester and Hans,
> 
> On Tue, Jun 25, 2013 at 06:55:49PM +0200, Sylwester Nawrocki wrote:
> > Hi Hans,
> > 
> > Cc: Laurent and Sakari
> > 
> > On 06/24/2013 10:54 AM, Hans Verkuil wrote:
> > > Hi Sylwester,
> > > 
> > > It came to my attention that several i2c subdev drivers overwrite the sd->name
> > > as set by v4l2_i2c_subdev_init with a custom name.
> > > 
> > > This is wrong if it is possible that there are multiple identical sensors in
> > > the system. The sd->name must be unique since it is used to prefix kernel
> > > messages etc, so you have to be able to tell the sensor devices apart.
> > 
> > This has been discussed in the past, please see thread [1]. 
> > 
> > > It concerns the following Samsung-contributed drivers:
> > > 
> > > drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> > > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> > > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
> > > drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> > > drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > > drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > > drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > 
> > It seems ov9650 is missing on this list,
> > 
> > $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
> > drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > drivers/media/i2c/ov9650.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> > drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
> > drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> > drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name, code->pad, code->index);
> > drivers/media/i2c/smiapp/smiapp-core.c: strlcpy(subdev->name, sensor->minfo.name, sizeof(subdev->name));
> 
> For smiapp the issue is that smiapp is the name of the driver; there's no
> sensor which would be called "smiapp" but a large number of different
> devices that implement the SMIA or SMIA++ standard. The driver can recognise
> some of them and call them according to their real name.

But the smiapp driver can still prefix that real name with the i2c bus info,
right? Just as v4l2_i2c_subdev_init does.

> > drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> > drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> > 
> > > If there can be only one sensor (because it is integrated in the SoC),
> > > then there is no problem with doing this. But it is not obvious to me
> > > which of these drivers are for integrated systems, and which aren't.
> > 
> > Those sensors are standalone devices, I'm not aware of any of them to be 
> > integrated with an Application Processor SoC. I've never seen something 
> > like that. However some of those devices are hybrid modules with a raw 
> > image sensor and an ISP SoC.
> > So in theory there could be multiple such devices in a single system, 
> > although personally I've never seen something like that.
> > 
> > > I can make patches for those that need to be fixed if you can tell me
> > > which drivers are affected.
> > 
> > You may want to have a look at the commits listed below, and the comments 
> > I received to that [2] patch series...

What comments? I see no comments.

I would have Nacked those patches, but I probably never saw them since you posted
them during a period where I was mostly absent from the list.

> > 
> > commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
> > [media] noon010pc30: Make subdev name independent of the I2C slave address
> > 
> > commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
> > [media] s5k6aa: Make subdev name independent of the I2C slave address
> > 
> > commit c5024a70bb60b678f08586ed786340ec162d250f
> > [media] m5mols: Make subdev name independent of the I2C slave address
> > 
> > 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.

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).

> 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.

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.

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.

> > 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 :-)

> 
> > 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.

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 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).

> > 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.

> > 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.

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).

Regards,

	Hans

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

[Sylwester Nawrocki]

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:
>>>> Hi Sylwester,
>>>>
>>>> It came to my attention that several i2c subdev drivers overwrite the sd->name
>>>> as set by v4l2_i2c_subdev_init with a custom name.
>>>>
>>>> This is wrong if it is possible that there are multiple identical sensors in
>>>> the system. The sd->name must be unique since it is used to prefix kernel
>>>> messages etc, so you have to be able to tell the sensor devices apart.
>>>
>>> This has been discussed in the past, please see thread [1]. 
>>>
>>>> It concerns the following Samsung-contributed drivers:
>>>>
>>>> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
>>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
>>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
>>>> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
>>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>>>> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>>>> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>>>
>>> It seems ov9650 is missing on this list,
>>>
>>> $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
>>> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>>> drivers/media/i2c/ov9650.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name, "S5C73M3-OIF");
>>> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
>>> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>>> drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name, code->pad, code->index);
>>> drivers/media/i2c/smiapp/smiapp-core.c: strlcpy(subdev->name, sensor->minfo.name, sizeof(subdev->name));
>>
>> For smiapp the issue is that smiapp is the name of the driver; there's no
>> sensor which would be called "smiapp" but a large number of different
>> devices that implement the SMIA or SMIA++ standard. The driver can recognise
>> some of them and call them according to their real name.
> 
> But the smiapp driver can still prefix that real name with the i2c bus info,
> right? Just as v4l2_i2c_subdev_init does.
> 
>>> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
>>> drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>>>
>>>> If there can be only one sensor (because it is integrated in the SoC),
>>>> then there is no problem with doing this. But it is not obvious to me
>>>> which of these drivers are for integrated systems, and which aren't.
>>>
>>> Those sensors are standalone devices, I'm not aware of any of them to be 
>>> integrated with an Application Processor SoC. I've never seen something 
>>> like that. However some of those devices are hybrid modules with a raw 
>>> image sensor and an ISP SoC.
>>> So in theory there could be multiple such devices in a single system, 
>>> although personally I've never seen something like that.
>>>
>>>> I can make patches for those that need to be fixed if you can tell me
>>>> which drivers are affected.
>>>
>>> You may want to have a look at the commits listed below, and the comments 
>>> I received to that [2] patch series...
> 
> What comments? I see no comments.

Yes, that's the point :) IMHO it's a bit late for reverting that, and
breaking existing user space.

> I would have Nacked those patches, but I probably never saw them since you posted
> them during a period where I was mostly absent from the list.

Fair enough.

>>> commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
>>> [media] noon010pc30: Make subdev name independent of the I2C slave address
>>>
>>> commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
>>> [media] s5k6aa: Make subdev name independent of the I2C slave address
>>>
>>> commit c5024a70bb60b678f08586ed786340ec162d250f
>>> [media] m5mols: Make subdev name independent of the I2C slave address
>>>
>>> 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.
> 
> 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 ?

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.

>> 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.
> 
> 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 :-)

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.

> 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.

> 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.

>>> 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.

--
Regards,
Sylwester

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

[Laurent Pinchart]

Hello,

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:
> >>>> Hi Sylwester,
> >>>> 
> >>>> It came to my attention that several i2c subdev drivers overwrite the
> >>>> sd->name as set by v4l2_i2c_subdev_init with a custom name.
> >>>> 
> >>>> This is wrong if it is possible that there are multiple identical
> >>>> sensors in the system. The sd->name must be unique since it is used to
> >>>> prefix kernel messages etc, so you have to be able to tell the sensor
> >>>> devices apart.
> >>> 
> >>> This has been discussed in the past, please see thread [1].
> >>> 
> >>>> It concerns the following Samsung-contributed drivers:
> >>>> 
> >>>> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME,
> >>>> sizeof(sd->name)); drivers/media/i2c/s5c73m3/s5c73m3-core.c:      
> >>>> strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> >>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
> >>>> "S5C73M3-OIF"); drivers/media/i2c/sr030pc30.c:  strcpy(sd->name,
> >>>> MODULE_NAME);
> >>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
> >>>> sizeof(sd->name)); drivers/media/i2c/m5mols/m5mols_core.c:
> >>>> strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> >>>> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME,
> >>>> sizeof(sd->name));>>> 
> >>> It seems ov9650 is missing on this list,
> >>> 
> >>> $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
> >>> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME,
> >>> sizeof(sd->name));
> >>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
> >>> sizeof(sd->name));
> >>> drivers/media/i2c/ov9650.c:             strlcpy(sd->name, DRIVER_NAME,
> >>> sizeof(sd->name));
> >>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name,
> >>> "S5C73M3", sizeof(sd->name));
> >>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
> >>> "S5C73M3-OIF");
> >>> drivers/media/i2c/s5k4ecgx.c:                   strlcpy(sd->name,
> >>> S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> >>> drivers/media/i2c/s5k6aa.c:                     strlcpy(sd->name,
> >>> DRIVER_NAME, sizeof(sd->name));
> >>> drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name,
> >>> code->pad, code->index);
> >>> drivers/media/i2c/smiapp/smiapp-core.c:         strlcpy(subdev->name,
> >>> sensor->minfo.name, sizeof(subdev->name));
> >> 
> >> For smiapp the issue is that smiapp is the name of the driver; there's no
> >> sensor which would be called "smiapp" but a large number of different
> >> devices that implement the SMIA or SMIA++ standard. The driver can
> >> recognise some of them and call them according to their real name.
> > 
> > But the smiapp driver can still prefix that real name with the i2c bus
> > info, right? Just as v4l2_i2c_subdev_init does.

Do you mean postfix instead of prefix ?

> >>> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> >>> drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME,
> >>> sizeof(sd->name));
> >>> 
> >>>> If there can be only one sensor (because it is integrated in the SoC),
> >>>> then there is no problem with doing this. But it is not obvious to me
> >>>> which of these drivers are for integrated systems, and which aren't.
> >>> 
> >>> Those sensors are standalone devices, I'm not aware of any of them to be
> >>> integrated with an Application Processor SoC. I've never seen something
> >>> like that. However some of those devices are hybrid modules with a raw
> >>> image sensor and an ISP SoC.
> >>> So in theory there could be multiple such devices in a single system,
> >>> although personally I've never seen something like that.
> >>> 
> >>>> I can make patches for those that need to be fixed if you can tell me
> >>>> which drivers are affected.
> >>> 
> >>> You may want to have a look at the commits listed below, and the
> >>> comments I received to that [2] patch series...
> > 
> > What comments? I see no comments.
> 
> Yes, that's the point :) IMHO it's a bit late for reverting that, and
> breaking existing user space.
> 
> > I would have Nacked those patches, but I probably never saw them since you
> > posted them during a period where I was mostly absent from the list.
> 
> Fair enough.
> 
> >>> commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
> >>> [media] noon010pc30: Make subdev name independent of the I2C slave
> >>> address
> >>> 
> >>> commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
> >>> [media] s5k6aa: Make subdev name independent of the I2C slave address
> >>> 
> >>> commit c5024a70bb60b678f08586ed786340ec162d250f
> >>> [media] m5mols: Make subdev name independent of the I2C slave address
> >>> 
> >>> 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.

> 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 ?

> 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).

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 
that will become too long for the media entity and subdev name fields.

> > 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.

-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

On Thu 4 July 2013 00:49:36 Laurent Pinchart wrote:
> Hello,
> 
> 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:
> > >>>> Hi Sylwester,
> > >>>> 
> > >>>> It came to my attention that several i2c subdev drivers overwrite the
> > >>>> sd->name as set by v4l2_i2c_subdev_init with a custom name.
> > >>>> 
> > >>>> This is wrong if it is possible that there are multiple identical
> > >>>> sensors in the system. The sd->name must be unique since it is used to
> > >>>> prefix kernel messages etc, so you have to be able to tell the sensor
> > >>>> devices apart.
> > >>> 
> > >>> This has been discussed in the past, please see thread [1].
> > >>> 
> > >>>> It concerns the following Samsung-contributed drivers:
> > >>>> 
> > >>>> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME,
> > >>>> sizeof(sd->name)); drivers/media/i2c/s5c73m3/s5c73m3-core.c:      
> > >>>> strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> > >>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
> > >>>> "S5C73M3-OIF"); drivers/media/i2c/sr030pc30.c:  strcpy(sd->name,
> > >>>> MODULE_NAME);
> > >>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
> > >>>> sizeof(sd->name)); drivers/media/i2c/m5mols/m5mols_core.c:
> > >>>> strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
> > >>>> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME,
> > >>>> sizeof(sd->name));>>> 
> > >>> It seems ov9650 is missing on this list,
> > >>> 
> > >>> $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
> > >>> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME,
> > >>> sizeof(sd->name));
> > >>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
> > >>> sizeof(sd->name));
> > >>> drivers/media/i2c/ov9650.c:             strlcpy(sd->name, DRIVER_NAME,
> > >>> sizeof(sd->name));
> > >>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name,
> > >>> "S5C73M3", sizeof(sd->name));
> > >>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
> > >>> "S5C73M3-OIF");
> > >>> drivers/media/i2c/s5k4ecgx.c:                   strlcpy(sd->name,
> > >>> S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> > >>> drivers/media/i2c/s5k6aa.c:                     strlcpy(sd->name,
> > >>> DRIVER_NAME, sizeof(sd->name));
> > >>> drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name,
> > >>> code->pad, code->index);
> > >>> drivers/media/i2c/smiapp/smiapp-core.c:         strlcpy(subdev->name,
> > >>> sensor->minfo.name, sizeof(subdev->name));
> > >> 
> > >> For smiapp the issue is that smiapp is the name of the driver; there's no
> > >> sensor which would be called "smiapp" but a large number of different
> > >> devices that implement the SMIA or SMIA++ standard. The driver can
> > >> recognise some of them and call them according to their real name.
> > > 
> > > But the smiapp driver can still prefix that real name with the i2c bus
> > > info, right? Just as v4l2_i2c_subdev_init does.
> 
> Do you mean postfix instead of prefix ?

Yes, sorry.

> > >>> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
> > >>> drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME,
> > >>> sizeof(sd->name));
> > >>> 
> > >>>> If there can be only one sensor (because it is integrated in the SoC),
> > >>>> then there is no problem with doing this. But it is not obvious to me
> > >>>> which of these drivers are for integrated systems, and which aren't.
> > >>> 
> > >>> Those sensors are standalone devices, I'm not aware of any of them to be
> > >>> integrated with an Application Processor SoC. I've never seen something
> > >>> like that. However some of those devices are hybrid modules with a raw
> > >>> image sensor and an ISP SoC.
> > >>> So in theory there could be multiple such devices in a single system,
> > >>> although personally I've never seen something like that.
> > >>> 
> > >>>> I can make patches for those that need to be fixed if you can tell me
> > >>>> which drivers are affected.
> > >>> 
> > >>> You may want to have a look at the commits listed below, and the
> > >>> comments I received to that [2] patch series...
> > > 
> > > What comments? I see no comments.
> > 
> > Yes, that's the point :) IMHO it's a bit late for reverting that, and
> > breaking existing user space.
> > 
> > > I would have Nacked those patches, but I probably never saw them since you
> > > posted them during a period where I was mostly absent from the list.
> > 
> > Fair enough.
> > 
> > >>> commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
> > >>> [media] noon010pc30: Make subdev name independent of the I2C slave
> > >>> address
> > >>> 
> > >>> commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
> > >>> [media] s5k6aa: Make subdev name independent of the I2C slave address
> > >>> 
> > >>> commit c5024a70bb60b678f08586ed786340ec162d250f
> > >>> [media] m5mols: Make subdev name independent of the I2C slave address
> > >>> 
> > >>> 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.

Frankly, I don't think either should contain a space :-) Today nobody is
using spaces anywhere to the best of my knowledge.

> > 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.

> 
> > 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,
then it becomes dynamic (but still unique, and still it specifies exactly where
the device can be found in the hardware topology).

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 
> that will become too long for the media entity and subdev name fields.
> 
> > > 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.
> 

Regards,

	Hans

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

[Sylwester Nawrocki]

On 07/04/2013 01:13 PM, Hans Verkuil wrote:
> On Thu 4 July 2013 00:49:36 Laurent Pinchart wrote:
>> Hello,
>>
>> 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:
>>>>>>> Hi Sylwester,
>>>>>>>
>>>>>>> It came to my attention that several i2c subdev drivers overwrite the
>>>>>>> sd->name as set by v4l2_i2c_subdev_init with a custom name.
>>>>>>>
>>>>>>> This is wrong if it is possible that there are multiple identical
>>>>>>> sensors in the system. The sd->name must be unique since it is used to
>>>>>>> prefix kernel messages etc, so you have to be able to tell the sensor
>>>>>>> devices apart.
>>>>>>
>>>>>> This has been discussed in the past, please see thread [1].
>>>>>>
>>>>>>> It concerns the following Samsung-contributed drivers:
>>>>>>>
>>>>>>> drivers/media/i2c/s5k4ecgx.c:   strlcpy(sd->name, S5K4ECGX_DRIVER_NAME,
>>>>>>> sizeof(sd->name)); drivers/media/i2c/s5c73m3/s5c73m3-core.c:
>>>>>>> strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
>>>>>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
>>>>>>> "S5C73M3-OIF"); drivers/media/i2c/sr030pc30.c:  strcpy(sd->name,
>>>>>>> MODULE_NAME);
>>>>>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
>>>>>>> sizeof(sd->name)); drivers/media/i2c/m5mols/m5mols_core.c:
>>>>>>> strlcpy(sd->name, MODULE_NAME, sizeof(sd->name));
>>>>>>> drivers/media/i2c/s5k6aa.c:     strlcpy(sd->name, DRIVER_NAME,
>>>>>>> sizeof(sd->name));>>>
>>>>>> It seems ov9650 is missing on this list,
>>>>>>
>>>>>> $ git grep ".*cpy.*(.*sd\|subdev.*name" -- drivers/media/i2c
>>>>>> drivers/media/i2c/m5mols/m5mols_core.c: strlcpy(sd->name, MODULE_NAME,
>>>>>> sizeof(sd->name));
>>>>>> drivers/media/i2c/noon010pc30.c:        strlcpy(sd->name, MODULE_NAME,
>>>>>> sizeof(sd->name));
>>>>>> drivers/media/i2c/ov9650.c:             strlcpy(sd->name, DRIVER_NAME,
>>>>>> sizeof(sd->name));
>>>>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strlcpy(sd->name,
>>>>>> "S5C73M3", sizeof(sd->name));
>>>>>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:       strcpy(oif_sd->name,
>>>>>> "S5C73M3-OIF");
>>>>>> drivers/media/i2c/s5k4ecgx.c:                   strlcpy(sd->name,
>>>>>> S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
>>>>>> drivers/media/i2c/s5k6aa.c:                     strlcpy(sd->name,
>>>>>> DRIVER_NAME, sizeof(sd->name));
>>>>>> drivers/media/i2c/smiapp/smiapp-core.c:         subdev->name,
>>>>>> code->pad, code->index);
>>>>>> drivers/media/i2c/smiapp/smiapp-core.c:         strlcpy(subdev->name,
>>>>>> sensor->minfo.name, sizeof(subdev->name));
>>>>>
>>>>> For smiapp the issue is that smiapp is the name of the driver; there's no
>>>>> sensor which would be called "smiapp" but a large number of different
>>>>> devices that implement the SMIA or SMIA++ standard. The driver can
>>>>> recognise some of them and call them according to their real name.
>>>>
>>>> But the smiapp driver can still prefix that real name with the i2c bus
>>>> info, right? Just as v4l2_i2c_subdev_init does.
>>
>> Do you mean postfix instead of prefix ?
>
> Yes, sorry.
>
>>>>>> drivers/media/i2c/sr030pc30.c:  strcpy(sd->name, MODULE_NAME);
>>>>>> drivers/media/i2c/tvp514x.c:    strlcpy(sd->name, TVP514X_MODULE_NAME,
>>>>>> sizeof(sd->name));
>>>>>>
>>>>>>> If there can be only one sensor (because it is integrated in the SoC),
>>>>>>> then there is no problem with doing this. But it is not obvious to me
>>>>>>> which of these drivers are for integrated systems, and which aren't.
>>>>>>
>>>>>> Those sensors are standalone devices, I'm not aware of any of them to be
>>>>>> integrated with an Application Processor SoC. I've never seen something
>>>>>> like that. However some of those devices are hybrid modules with a raw
>>>>>> image sensor and an ISP SoC.
>>>>>> So in theory there could be multiple such devices in a single system,
>>>>>> although personally I've never seen something like that.
>>>>>>
>>>>>>> I can make patches for those that need to be fixed if you can tell me
>>>>>>> which drivers are affected.
>>>>>>
>>>>>> You may want to have a look at the commits listed below, and the
>>>>>> comments I received to that [2] patch series...
>>>>
>>>> What comments? I see no comments.
>>>
>>> Yes, that's the point :) IMHO it's a bit late for reverting that, and
>>> breaking existing user space.
>>>
>>>> I would have Nacked those patches, but I probably never saw them since you
>>>> posted them during a period where I was mostly absent from the list.
>>>
>>> Fair enough.
>>>
>>>>>> commit 2138d73b69be1cfa4982c9949f2445ec77ea9edc
>>>>>> [media] noon010pc30: Make subdev name independent of the I2C slave
>>>>>> address
>>>>>>
>>>>>> commit 14b702dd71d38b6d0662251b3f8cd60da98602ce
>>>>>> [media] s5k6aa: Make subdev name independent of the I2C slave address
>>>>>>
>>>>>> commit c5024a70bb60b678f08586ed786340ec162d250f
>>>>>> [media] m5mols: Make subdev name independent of the I2C slave address
>>>>>>
>>>>>> 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.

>>> 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.

>>> 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.

> 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=ee5c27440cc24d62ec463cce4c000bb32c5692c7

Regards,
Sylwester

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

[Laurent Pinchart]

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

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

[Sylwester Nawrocki]

Hi Laurent,

On 07/05/2013 01:30 PM, Laurent Pinchart wrote:
> 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.

Yes, I guess dash or underscore could be well used instead of spaces.
But my feeling is that 32 characters might be often not enough to hold
longer names and bus info. Also it would be good to denote what sort of
bus we refer to, i2c, spi, usb, platform, etc. I doesn't look like we
can always fit that information in 32 characters.

[...]
>>>> 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.

Indeed, it's a pretty serious limitation IMHO.

>>>>> 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 :-)

Ok, I just wanted to point out some alternatives. ;-)

> 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.

Yes, that sounds much more interesting than using just subdev name to sqeeze
all the information in. Why we don't have such an ioctl yet anyway ? Were
there some arguments against it, or its been just a low priority issue ?

Regards,
Sylwester

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

[Sakari Ailus]

Hi Sylwester and Laurent,

Sylwester Nawrocki wrote:
> Hi Laurent,
...
>> 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.
>
> Yes, that sounds much more interesting than using just subdev name to
> sqeeze
> all the information in. Why we don't have such an ioctl yet anyway ? Were
> there some arguments against it, or its been just a low priority issue ?

I think it's just been left unaddressed until now since there have been 
even more important things to work on. :-) I'm all for that, btw.; 
associating bus information to the media device instead of entities was 
always a little odd (feel free to blame me, too...).

Perhaps we could steal some bytes from the union in struct 
media_entity_desc? :-)

-- 
Cheers,

Sakari Ailus
sakari.ailus@iki.fi

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

[Laurent Pinchart]

Hi Sylwester,

On Saturday 06 July 2013 22:50:32 Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 07/05/2013 01:30 PM, Laurent Pinchart wrote:
> > 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.
>
> Yes, I guess dash or underscore could be well used instead of spaces.
> But my feeling is that 32 characters might be often not enough to hold
> longer names and bus info. Also it would be good to denote what sort of
> bus we refer to, i2c, spi, usb, platform, etc. I doesn't look like we
> can always fit that information in 32 characters.
> 
> [...]
> 
> >>>> 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.
> 
> Indeed, it's a pretty serious limitation IMHO.
> 
> >>>>> 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 :-)
> 
> Ok, I just wanted to point out some alternatives. ;-)
> 
> > 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.
> 
> Yes, that sounds much more interesting than using just subdev name to sqeeze
> all the information in. Why we don't have such an ioctl yet anyway ? Were
> there some arguments against it, or its been just a low priority issue ?

It has just been a low-priority issue.

All we need here is a way for userspace to pass a buffer pointer to the kernel 
that will be filled with typed key-value pairs. I'm not sure whether a 
hierarchical structured format such as ASN.1 would be useful or if a flat 
hierarchy would be enough.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

On Thursday 11 July 2013 01:19:35 Sakari Ailus wrote:
> Hi Sylwester and Laurent,
> 
> Sylwester Nawrocki wrote:
> > Hi Laurent,
> 
> ...
> 
> >> 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.
> > 
> > Yes, that sounds much more interesting than using just subdev name to
> > sqeeze all the information in. Why we don't have such an ioctl yet anyway
> > ? Were there some arguments against it, or its been just a low priority
> > issue ?
> 
> I think it's just been left unaddressed until now since there have been
> even more important things to work on. :-) I'm all for that, btw.;
> associating bus information to the media device instead of entities was
> always a little odd (feel free to blame me, too...).
> 
> Perhaps we could steal some bytes from the union in struct
> media_entity_desc? :-)

I've thought about that as well, but we will eventually need to pass more 
entity information to userspace, so a new ioctl would in my opinion be better, 
given the potentially large size of the bus information string.

-- 
Regards,

Laurent Pinchart