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

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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