[PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Alexander Stein]

Documentation to v4l2_i2c_subdev_set_name says I²C device name if
devname parameter is NULL. But instead the I²C driver name is assigned.
Fix this by actually using the i2c_client->name for the subdev name.

Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C sub-device's name")
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
name is "ov9282" without this patch. With this patch applied it is
"ov9281". The i2c_client->name is "ov9281" in every case.

 drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-i2c.c b/drivers/media/v4l2-core/v4l2-i2c.c
index b4acca75644b..90c3c799317a 100644
--- a/drivers/media/v4l2-core/v4l2-i2c.c
+++ b/drivers/media/v4l2-core/v4l2-i2c.c
@@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd,
 			      const char *devname, const char *postfix)
 {
 	if (!devname)
-		devname = client->dev.driver->name;
+		devname = client->name;
 	if (!postfix)
 		postfix = "";
 
-- 
2.25.1

Re: [PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Sakari Ailus]

Hi Alexander,

Thanks for the patch.

On Mon, Jul 18, 2022 at 02:01:51PM +0200, Alexander Stein wrote:
> Documentation to v4l2_i2c_subdev_set_name says I²C device name if
> devname parameter is NULL. But instead the I²C driver name is assigned.
> Fix this by actually using the i2c_client->name for the subdev name.
> 
> Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C sub-device's name")
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
> name is "ov9282" without this patch. With this patch applied it is
> "ov9281". The i2c_client->name is "ov9281" in every case.
> 
>  drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-i2c.c b/drivers/media/v4l2-core/v4l2-i2c.c
> index b4acca75644b..90c3c799317a 100644
> --- a/drivers/media/v4l2-core/v4l2-i2c.c
> +++ b/drivers/media/v4l2-core/v4l2-i2c.c
> @@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd,
>  			      const char *devname, const char *postfix)
>  {
>  	if (!devname)
> -		devname = client->dev.driver->name;
> +		devname = client->name;

This would work OF somehow but not on ACPI. At the moment, at least, I think
this information needs to come from the driver itself.

>  	if (!postfix)
>  		postfix = "";
>  

-- 
Kind regards,

Sakari Ailus

Re: Re: [PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Alexander Stein]

Hi Sakari,

thanks for the feedback.

Am Montag, 18. Juli 2022, 20:39:27 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> Thanks for the patch.
> 
> On Mon, Jul 18, 2022 at 02:01:51PM +0200, Alexander Stein wrote:
> > Documentation to v4l2_i2c_subdev_set_name says I²C device name if
> > devname parameter is NULL. But instead the I²C driver name is assigned.
> > Fix this by actually using the i2c_client->name for the subdev name.
> > 
> > Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C
> > sub-device's name") Signed-off-by: Alexander Stein
> > <alexander.stein@ew.tq-group.com>
> > ---
> > Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
> > name is "ov9282" without this patch. With this patch applied it is
> > "ov9281". The i2c_client->name is "ov9281" in every case.
> > 
> >  drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-i2c.c
> > b/drivers/media/v4l2-core/v4l2-i2c.c index b4acca75644b..90c3c799317a
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-i2c.c
> > +++ b/drivers/media/v4l2-core/v4l2-i2c.c
> > @@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd,
> > 
> >  			      const char *devname, const char 
*postfix)
> >  
> >  {
> >  
> >  	if (!devname)
> > 
> > -		devname = client->dev.driver->name;
> > +		devname = client->name;
> 
> This would work OF somehow but not on ACPI. At the moment, at least, I think
> this information needs to come from the driver itself.

I've not much experience regarding ACPI, nor can I test anything. Which 
information from ACPI should be used as devname? AFAICS i2c_acpi_fill_info 
need to be modified to set info->type as well. This should fix the ACPI case.

Nevertheless currently either the documentation or the implementation needs a 
fix, they don't match. You can also opt to driver name in case client->name is 
empty. Is this acceptable?

Best regards,
Alexander

Re: Re: [PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Sakari Ailus]

Hi Alexander,

On Wed, Jul 20, 2022 at 09:54:40AM +0200, Alexander Stein wrote:
> Hi Sakari,
> 
> thanks for the feedback.
> 
> Am Montag, 18. Juli 2022, 20:39:27 CEST schrieb Sakari Ailus:
> > Hi Alexander,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Jul 18, 2022 at 02:01:51PM +0200, Alexander Stein wrote:
> > > Documentation to v4l2_i2c_subdev_set_name says I²C device name if
> > > devname parameter is NULL. But instead the I²C driver name is assigned.
> > > Fix this by actually using the i2c_client->name for the subdev name.
> > > 
> > > Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C
> > > sub-device's name") Signed-off-by: Alexander Stein
> > > <alexander.stein@ew.tq-group.com>
> > > ---
> > > Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
> > > name is "ov9282" without this patch. With this patch applied it is
> > > "ov9281". The i2c_client->name is "ov9281" in every case.
> > > 
> > >  drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-i2c.c
> > > b/drivers/media/v4l2-core/v4l2-i2c.c index b4acca75644b..90c3c799317a
> > > 100644
> > > --- a/drivers/media/v4l2-core/v4l2-i2c.c
> > > +++ b/drivers/media/v4l2-core/v4l2-i2c.c
> > > @@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd,
> > > 
> > >  			      const char *devname, const char 
> *postfix)
> > >  
> > >  {
> > >  
> > >  	if (!devname)
> > > 
> > > -		devname = client->dev.driver->name;
> > > +		devname = client->name;
> > 
> > This would work OF somehow but not on ACPI. At the moment, at least, I think
> > this information needs to come from the driver itself.
> 
> I've not much experience regarding ACPI, nor can I test anything. Which 
> information from ACPI should be used as devname? AFAICS i2c_acpi_fill_info 
> need to be modified to set info->type as well. This should fix the ACPI case.

Where would that information come from?

> 
> Nevertheless currently either the documentation or the implementation needs a 
> fix, they don't match. You can also opt to driver name in case client->name is 
> empty. Is this acceptable?

How would you "fix" it? It currently uses the driver name unless the driver
set the name for the device. This works reasonably well in all cases.

-- 
Regards,

Sakari Ailus

Re: Re: [PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Alexander Stein]

Hi Sakari,

Am Freitag, 22. Juli 2022, 08:28:55 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> On Wed, Jul 20, 2022 at 09:54:40AM +0200, Alexander Stein wrote:
> > Hi Sakari,
> > 
> > thanks for the feedback.
> > 
> > Am Montag, 18. Juli 2022, 20:39:27 CEST schrieb Sakari Ailus:
> > > Hi Alexander,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Mon, Jul 18, 2022 at 02:01:51PM +0200, Alexander Stein wrote:
> > > > Documentation to v4l2_i2c_subdev_set_name says I²C device name if
> > > > devname parameter is NULL. But instead the I²C driver name is
> > > > assigned.
> > > > Fix this by actually using the i2c_client->name for the subdev name.
> > > > 
> > > > Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C
> > > > sub-device's name") Signed-off-by: Alexander Stein
> > > > <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
> > > > name is "ov9282" without this patch. With this patch applied it is
> > > > "ov9281". The i2c_client->name is "ov9281" in every case.
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-i2c.c
> > > > b/drivers/media/v4l2-core/v4l2-i2c.c index b4acca75644b..90c3c799317a
> > > > 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-i2c.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-i2c.c
> > > > @@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev
> > > > *sd,
> > > > 
> > > >  			      const char *devname, const char
> > 
> > *postfix)
> > 
> > > >  {
> > > >  
> > > >  	if (!devname)
> > > > 
> > > > -		devname = client->dev.driver->name;
> > > > +		devname = client->name;
> > > 
> > > This would work OF somehow but not on ACPI. At the moment, at least, I
> > > think this information needs to come from the driver itself.
> > 
> > I've not much experience regarding ACPI, nor can I test anything. Which
> > information from ACPI should be used as devname? AFAICS i2c_acpi_fill_info
> > need to be modified to set info->type as well. This should fix the ACPI
> > case.
> Where would that information come from?

I don't have experience with ACPI. So you essentially are saying ACPI can't 
provide this kind of information?

> > Nevertheless currently either the documentation or the implementation
> > needs a fix, they don't match. You can also opt to driver name in case
> > client->name is empty. Is this acceptable?
> 
> How would you "fix" it? It currently uses the driver name unless the driver
> set the name for the device. This works reasonably well in all cases.

If ACPI can't provide the actual device model name, then you can't fix it, 
agreed.

Best regards,
Alexander

Re: Re: [PATCH 1/1] media: v4l2-core: v4l2-i2c: Fix default device name

[Sakari Ailus]

Hi Alexander,

On Fri, Jul 22, 2022 at 09:09:59AM +0200, Alexander Stein wrote:
> Hi Sakari,
> 
> Am Freitag, 22. Juli 2022, 08:28:55 CEST schrieb Sakari Ailus:
> > Hi Alexander,
> > 
> > On Wed, Jul 20, 2022 at 09:54:40AM +0200, Alexander Stein wrote:
> > > Hi Sakari,
> > > 
> > > thanks for the feedback.
> > > 
> > > Am Montag, 18. Juli 2022, 20:39:27 CEST schrieb Sakari Ailus:
> > > > Hi Alexander,
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > On Mon, Jul 18, 2022 at 02:01:51PM +0200, Alexander Stein wrote:
> > > > > Documentation to v4l2_i2c_subdev_set_name says I²C device name if
> > > > > devname parameter is NULL. But instead the I²C driver name is
> > > > > assigned.
> > > > > Fix this by actually using the i2c_client->name for the subdev name.
> > > > > 
> > > > > Fixes: 0658293012af ("media: v4l: subdev: Add a function to set an I²C
> > > > > sub-device's name") Signed-off-by: Alexander Stein
> > > > > <alexander.stein@ew.tq-group.com>
> > > > > ---
> > > > > Having a sensor node in DT with compatible = "ovti,ov9281" the subdev
> > > > > name is "ov9282" without this patch. With this patch applied it is
> > > > > "ov9281". The i2c_client->name is "ov9281" in every case.
> > > > > 
> > > > >  drivers/media/v4l2-core/v4l2-i2c.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-i2c.c
> > > > > b/drivers/media/v4l2-core/v4l2-i2c.c index b4acca75644b..90c3c799317a
> > > > > 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-i2c.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-i2c.c
> > > > > @@ -33,7 +33,7 @@ void v4l2_i2c_subdev_set_name(struct v4l2_subdev
> > > > > *sd,
> > > > > 
> > > > >  			      const char *devname, const char
> > > 
> > > *postfix)
> > > 
> > > > >  {
> > > > >  
> > > > >  	if (!devname)
> > > > > 
> > > > > -		devname = client->dev.driver->name;
> > > > > +		devname = client->name;
> > > > 
> > > > This would work OF somehow but not on ACPI. At the moment, at least, I
> > > > think this information needs to come from the driver itself.
> > > 
> > > I've not much experience regarding ACPI, nor can I test anything. Which
> > > information from ACPI should be used as devname? AFAICS i2c_acpi_fill_info
> > > need to be modified to set info->type as well. This should fix the ACPI
> > > case.
> > Where would that information come from?
> 
> I don't have experience with ACPI. So you essentially are saying ACPI can't 
> provide this kind of information?

In ACPI devices are associated with drivers by _HID and _CID objects
that are 8-character strings split evenly to vendor and device parts. I.e.
human-readable device names such as in DT aren't available.

-- 
Sakari Ailus