On 08/29/2018 12:58 PM, Sakari Ailus wrote: > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > entities) to what is fairly certainly known to be unique in the system, > even if there were more devices of the same kind. > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > the name to the name of the driver or the module while omitting the > device's I²C address and bus, leaving the devices with a static name and > effectively limiting the number of such devices in a media device to 1. > > Address this by using the name set by the V4L2 framework. > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> If Samsung agrees to fix this, then you can add my: Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> I think it depends a bit on whether these sensors are for in-house Samsung use only, or if anyone can buy them and use in products. In the latter case I am in favor of fixing this, but if it is in-house only, then I don't care that much. Although I would like to see a patch adding comments to these drivers that warn of this complication (to avoid people using code from these drivers as a template). Regards, Hans > --- > Hi, > > I'm a bit uncertain about this one. I discussed the matter with Hans and > his view was this is a bug (I don't disagree), but this bug affects uAPI. > Also these devices tend to be a few years old and might not see much use > in newer devices, so why bother? The naming convention musn't be copied to > newer drivers though. > > Any opinions? > > This patch depends on my non-RFC set I just posted, this patch in particular: > > <URL:https://patchwork.linuxtv.org/patch/51788/> > > drivers/media/i2c/m5mols/m5mols_core.c | 1 - > drivers/media/i2c/noon010pc30.c | 1 - > drivers/media/i2c/ov9650.c | 1 - > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 4 ++-- > drivers/media/i2c/s5k4ecgx.c | 1 - > drivers/media/i2c/s5k6aa.c | 1 - > 6 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c > index 12e79f9e32d5..320e79b63555 100644 > --- a/drivers/media/i2c/m5mols/m5mols_core.c > +++ b/drivers/media/i2c/m5mols/m5mols_core.c > @@ -987,7 +987,6 @@ static int m5mols_probe(struct i2c_client *client, > > sd = &info->sd; > v4l2_i2c_subdev_init(sd, client, &m5mols_ops); > - strlcpy(sd->name, MODULE_NAME, sizeof(sd->name)); > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > sd->internal_ops = &m5mols_subdev_internal_ops; > diff --git a/drivers/media/i2c/noon010pc30.c b/drivers/media/i2c/noon010pc30.c > index 88c498ad45df..0629bc138fbc 100644 > --- a/drivers/media/i2c/noon010pc30.c > +++ b/drivers/media/i2c/noon010pc30.c > @@ -720,7 +720,6 @@ static int noon010_probe(struct i2c_client *client, > mutex_init(&info->lock); > sd = &info->sd; > v4l2_i2c_subdev_init(sd, client, &noon010_ops); > - strlcpy(sd->name, MODULE_NAME, sizeof(sd->name)); > > sd->internal_ops = &noon010_subdev_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index 5bea31cd41aa..7e116eb415e5 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -1546,7 +1546,6 @@ static int ov965x_probe(struct i2c_client *client, > > sd = &ov965x->sd; > v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops); > - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > > sd->internal_ops = &ov965x_sd_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > index ce196b60f917..64212551524e 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > @@ -1683,7 +1683,7 @@ static int s5c73m3_probe(struct i2c_client *client, > v4l2_subdev_init(sd, &s5c73m3_subdev_ops); > sd->owner = client->dev.driver->owner; > v4l2_set_subdevdata(sd, state); > - strlcpy(sd->name, "S5C73M3", sizeof(sd->name)); > + v4l2_i2c_subdev_set_name(sd, client, NULL, NULL); > > sd->internal_ops = &s5c73m3_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > @@ -1698,7 +1698,7 @@ static int s5c73m3_probe(struct i2c_client *client, > return ret; > > v4l2_i2c_subdev_init(oif_sd, client, &oif_subdev_ops); > - strcpy(oif_sd->name, "S5C73M3-OIF"); > + v4l2_i2c_subdev_set_name(sd, client, NULL, "-OIF"); > > oif_sd->internal_ops = &oif_internal_ops; > oif_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c > index 6ebcf254989a..8aaf5ad26826 100644 > --- a/drivers/media/i2c/s5k4ecgx.c > +++ b/drivers/media/i2c/s5k4ecgx.c > @@ -954,7 +954,6 @@ static int s5k4ecgx_probe(struct i2c_client *client, > sd = &priv->sd; > /* Registering subdev */ > v4l2_i2c_subdev_init(sd, client, &s5k4ecgx_ops); > - strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name)); > > sd->internal_ops = &s5k4ecgx_subdev_internal_ops; > /* Support v4l2 sub-device user space API */ > diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c > index 13c10b5e2b45..9536316e2d80 100644 > --- a/drivers/media/i2c/s5k6aa.c > +++ b/drivers/media/i2c/s5k6aa.c > @@ -1576,7 +1576,6 @@ static int s5k6aa_probe(struct i2c_client *client, > > sd = &s5k6aa->sd; > v4l2_i2c_subdev_init(sd, client, &s5k6aa_subdev_ops); > - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > > sd->internal_ops = &s5k6aa_subdev_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >
