[2/2] media: v4l2-common: simplify v4l2_i2c_subdev_init name generation
diff mbox series

Message ID 1533158457-15831-2-git-send-email-phdm@macqel.be
State New
Headers show
Series
  • [1/2] media: v4l2-common: v4l2_spi_subdev_init : generate unique name
Related show

Commit Message

Philippe De Muyter Aug. 1, 2018, 9:20 p.m. UTC
When v4l2_i2c_subdev_init is called, dev_name(&client->dev) has already
been set.  Use it to generate subdev's name instead of recreating it
with "%d-%04x".  This improves the similarity in subdev's name creation
between v4l2_i2c_subdev_init and v4l2_spi_subdev_init.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>
---
 drivers/media/v4l2-core/v4l2-common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Sakari Ailus Aug. 3, 2018, 12:43 p.m. UTC | #1
Hi Philippe,

On Wed, Aug 01, 2018 at 11:20:57PM +0200, Philippe De Muyter wrote:
> When v4l2_i2c_subdev_init is called, dev_name(&client->dev) has already
> been set.  Use it to generate subdev's name instead of recreating it
> with "%d-%04x".  This improves the similarity in subdev's name creation
> between v4l2_i2c_subdev_init and v4l2_spi_subdev_init.
> 
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 5471c6d..b062111 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -121,9 +121,8 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
>  	v4l2_set_subdevdata(sd, client);
>  	i2c_set_clientdata(client, sd);
>  	/* initialize name */
> -	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
> -		client->dev.driver->name, i2c_adapter_id(client->adapter),
> -		client->addr);
> +	snprintf(sd->name, sizeof(sd->name), "%s %s",
> +		client->dev.driver->name, dev_name(&client->dev));
>  }
>  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>  

I like the patch in principle. But what's the effect of this on the actual
sub-device (and entity) names? Looking at i2c_dev_set_name(), this will be
different. We can't change the existing entity naming in drivers, this will
break applications that expect them to be named in a certain way.
Philippe De Muyter Aug. 3, 2018, 1:46 p.m. UTC | #2
Hi Sakari,

On Fri, Aug 03, 2018 at 03:43:15PM +0300, Sakari Ailus wrote:
> Hi Philippe,
> 
> On Wed, Aug 01, 2018 at 11:20:57PM +0200, Philippe De Muyter wrote:
> > When v4l2_i2c_subdev_init is called, dev_name(&client->dev) has already
> > been set.  Use it to generate subdev's name instead of recreating it
> > with "%d-%04x".  This improves the similarity in subdev's name creation
> > between v4l2_i2c_subdev_init and v4l2_spi_subdev_init.
> > 
> > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 5471c6d..b062111 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -121,9 +121,8 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> >  	v4l2_set_subdevdata(sd, client);
> >  	i2c_set_clientdata(client, sd);
> >  	/* initialize name */
> > -	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
> > -		client->dev.driver->name, i2c_adapter_id(client->adapter),
> > -		client->addr);
> > +	snprintf(sd->name, sizeof(sd->name), "%s %s",
> > +		client->dev.driver->name, dev_name(&client->dev));
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
> >  
> 
> I like the patch in principle. But what's the effect of this on the actual
> sub-device (and entity) names? Looking at i2c_dev_set_name(), this will be
> different. We can't change the existing entity naming in drivers, this will
> break applications that expect them to be named in a certain way.

Yeah.  I am 10 years too late.

Maybe adding for a long transition period a kernel message giving the new
name and the old one if they are different ?

Thank you

Philippe
Sakari Ailus Aug. 3, 2018, 2:11 p.m. UTC | #3
On Fri, Aug 03, 2018 at 03:46:32PM +0200, Philippe De Muyter wrote:
> Hi Sakari,
> 
> On Fri, Aug 03, 2018 at 03:43:15PM +0300, Sakari Ailus wrote:
> > Hi Philippe,
> > 
> > On Wed, Aug 01, 2018 at 11:20:57PM +0200, Philippe De Muyter wrote:
> > > When v4l2_i2c_subdev_init is called, dev_name(&client->dev) has already
> > > been set.  Use it to generate subdev's name instead of recreating it
> > > with "%d-%04x".  This improves the similarity in subdev's name creation
> > > between v4l2_i2c_subdev_init and v4l2_spi_subdev_init.
> > > 
> > > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-common.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > index 5471c6d..b062111 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -121,9 +121,8 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> > >  	v4l2_set_subdevdata(sd, client);
> > >  	i2c_set_clientdata(client, sd);
> > >  	/* initialize name */
> > > -	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
> > > -		client->dev.driver->name, i2c_adapter_id(client->adapter),
> > > -		client->addr);
> > > +	snprintf(sd->name, sizeof(sd->name), "%s %s",
> > > +		client->dev.driver->name, dev_name(&client->dev));
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
> > >  
> > 
> > I like the patch in principle. But what's the effect of this on the actual
> > sub-device (and entity) names? Looking at i2c_dev_set_name(), this will be
> > different. We can't change the existing entity naming in drivers, this will
> > break applications that expect them to be named in a certain way.
> 
> Yeah.  I am 10 years too late.
> 
> Maybe adding for a long transition period a kernel message giving the new
> name and the old one if they are different ?

I think this information would be usable for the property API if we ever get
around defining it precisely and implementing it. That's long term stuff in
any case.

<URL:https://spinics.net/lists/linux-media/msg90160.html>
Philippe De Muyter Aug. 8, 2018, 8:43 a.m. UTC | #4
On Fri, Aug 03, 2018 at 03:43:15PM +0300, Sakari Ailus wrote:
> Hi Philippe,
> 
> On Wed, Aug 01, 2018 at 11:20:57PM +0200, Philippe De Muyter wrote:
> > When v4l2_i2c_subdev_init is called, dev_name(&client->dev) has already
> > been set.  Use it to generate subdev's name instead of recreating it
> > with "%d-%04x".  This improves the similarity in subdev's name creation
> > between v4l2_i2c_subdev_init and v4l2_spi_subdev_init.
> > 
> > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 5471c6d..b062111 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -121,9 +121,8 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> >  	v4l2_set_subdevdata(sd, client);
> >  	i2c_set_clientdata(client, sd);
> >  	/* initialize name */
> > -	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
> > -		client->dev.driver->name, i2c_adapter_id(client->adapter),
> > -		client->addr);
> > +	snprintf(sd->name, sizeof(sd->name), "%s %s",
> > +		client->dev.driver->name, dev_name(&client->dev));
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
> >  
> 
> I like the patch in principle. But what's the effect of this on the actual
> sub-device (and entity) names? Looking at i2c_dev_set_name(), this will be
> different. We can't change the existing entity naming in drivers, this will
> break applications that expect them to be named in a certain way.

Does your comment also prevent patch 1/2
"media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
to be accepted ?

Philippe

Patch
diff mbox series

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 5471c6d..b062111 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -121,9 +121,8 @@  void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
 	v4l2_set_subdevdata(sd, client);
 	i2c_set_clientdata(client, sd);
 	/* initialize name */
-	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
-		client->dev.driver->name, i2c_adapter_id(client->adapter),
-		client->addr);
+	snprintf(sd->name, sizeof(sd->name), "%s %s",
+		client->dev.driver->name, dev_name(&client->dev));
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);