[RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Sakari Ailus]

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@linux.intel.com>
---
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;
-- 
2.11.0

Re: [RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Hans Verkuil]

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@linux.intel.com>

If Samsung agrees to fix this, then you can add my:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

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

Re: [RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Akinobu Mita]

2018年8月29日(水) 19:58 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> 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@linux.intel.com>
> ---
> 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?

The change for the ov9650 driver looks OK to me.

My media device setup script needs to be updated by this change, but
it is not a big deal.

Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com>

Re: [RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Sylwester Nawrocki]

Hi Sakari,

On Thu, 13 Sep 2018 at 11:21, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
[...]
> 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");

I would suggest to change the "OIF-" prefix to lower case, to avoid
something like
"s5c73m3-OIF". IIRC client->name is derived from DT compatible string, which is
in lower case.
Otherwise looks OK to me.

--
Thanks,
Sylwester