[PATCH] iio: tmp006: Set correct iio name

[PATCH] iio: tmp006: Set correct iio name

[Yong Li]

When load the driver using the below command:
echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device

In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
they are inconsistent. With this patch,
the iio name will be the same as the i2c device name

Signed-off-by: Yong Li <sdliyong@gmail.com>
---
 drivers/iio/temperature/tmp006.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index 18c9b43..17c8413 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -221,7 +221,7 @@ static int tmp006_probe(struct i2c_client *client,
 	data->client = client;
 
 	indio_dev->dev.parent = &client->dev;
-	indio_dev->name = dev_name(&client->dev);
+	indio_dev->name = id->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &tmp006_info;
 
-- 
2.5.0

Re: [PATCH] iio: tmp006: Set correct iio name

[Jonathan Cameron]

On 22/04/16 04:43, Yong Li wrote:
> When load the driver using the below command:
> echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device
> 
> In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
> they are inconsistent. With this patch,
> the iio name will be the same as the i2c device name
> 
> Signed-off-by: Yong Li <sdliyong@gmail.com>
Peter, this looks right to me, but could you take a quick look as I guess
there might be a reason you did this in an unusual way originally?

Jonathan
> ---
>  drivers/iio/temperature/tmp006.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> index 18c9b43..17c8413 100644
> --- a/drivers/iio/temperature/tmp006.c
> +++ b/drivers/iio/temperature/tmp006.c
> @@ -221,7 +221,7 @@ static int tmp006_probe(struct i2c_client *client,
>  	data->client = client;
>  
>  	indio_dev->dev.parent = &client->dev;
> -	indio_dev->name = dev_name(&client->dev);
> +	indio_dev->name = id->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &tmp006_info;
>  
> 

Re: [PATCH] iio: tmp006: Set correct iio name

[Crestez Dan Leonard]

On 04/25/2016 10:33 PM, Jonathan Cameron wrote:
> On 22/04/16 04:43, Yong Li wrote:
>> When load the driver using the below command:
>> echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device
>>
>> In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
>> they are inconsistent. With this patch,
>> the iio name will be the same as the i2c device name
>>
>> Signed-off-by: Yong Li <sdliyong@gmail.com>
> Peter, this looks right to me, but could you take a quick look as I guess
> there might be a reason you did this in an unusual way originally?
>
Is there a "correct" or "usual" way to set indio_dev->name? Some quick 
grepping shows no clear standard:

$ git grep -h 'indio_dev->name =' drivers/iio/ | wc -l
148
$ git grep -h 'indio_dev->name =' drivers/iio/ | grep id | wc -l
52
$ git grep -h 'indio_dev->name =' drivers/iio/ | grep dev_name | wc -l
20
$ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i drv | wc -l
19
$ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i driver | wc -l
15

It seems that many devices use dev_name(&i2c_client->dev) or otherwise 
some sort of "ABC123_DRIVER_NAME" constant.

It's also not clear what this "name" field is for. Is it more than just 
a cosmetic sysfs attribute? It seems to me that names don't have to be 
unique so it would be wrong to use them for identification.

-- 
Regards,
Leonard

Re: [PATCH] iio: tmp006: Set correct iio name

[Jonathan Cameron]

On 25/04/16 21:59, Crestez Dan Leonard wrote:
> On 04/25/2016 10:33 PM, Jonathan Cameron wrote:
>> On 22/04/16 04:43, Yong Li wrote:
>>> When load the driver using the below command:
>>> echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device
>>>
>>> In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
>>> they are inconsistent. With this patch,
>>> the iio name will be the same as the i2c device name
>>>
>>> Signed-off-by: Yong Li <sdliyong@gmail.com>
>> Peter, this looks right to me, but could you take a quick look as I guess
>> there might be a reason you did this in an unusual way originally?
>>
> Is there a "correct" or "usual" way to set indio_dev->name? Some quick grepping shows no clear standard:
> 
> $ git grep -h 'indio_dev->name =' drivers/iio/ | wc -l
> 148
> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep id | wc -l
> 52
> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep dev_name | wc -l
> 20
> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i drv | wc -l
> 19
> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i driver | wc -l
> 15
> 
> It seems that many devices use dev_name(&i2c_client->dev) or
> otherwise some sort of "ABC123_DRIVER_NAME" constant.
> 
> It's also not clear what this "name" field is for. Is it more than
> just a cosmetic sysfs attribute? It seems to me that names don't have
> to be unique so it would be wrong to use them for identification.
> 
It's a convenience field really as there is no clear standard for where else
to find out what a part actually is (i.e. if it is an i2c part you can look
in the name attribute i2c supplies - but otherwise you are on your own).

For i2c drivers it should typically be the id->name.  Which is what this
patch changes it to for this part.

The constant case should only be used for drivers that only support one
part (which is quite a few of them!)

Jonathan

Re: [PATCH] iio: tmp006: Set correct iio name

[Lars-Peter Clausen]

On 04/25/2016 11:11 PM, Jonathan Cameron wrote:
> On 25/04/16 21:59, Crestez Dan Leonard wrote:
>> On 04/25/2016 10:33 PM, Jonathan Cameron wrote:
>>> On 22/04/16 04:43, Yong Li wrote:
>>>> When load the driver using the below command:
>>>> echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device
>>>>
>>>> In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
>>>> they are inconsistent. With this patch,
>>>> the iio name will be the same as the i2c device name
>>>>
>>>> Signed-off-by: Yong Li <sdliyong@gmail.com>
>>> Peter, this looks right to me, but could you take a quick look as I guess
>>> there might be a reason you did this in an unusual way originally?
>>>
>> Is there a "correct" or "usual" way to set indio_dev->name? Some quick grepping shows no clear standard:
>>
>> $ git grep -h 'indio_dev->name =' drivers/iio/ | wc -l
>> 148
>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep id | wc -l
>> 52
>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep dev_name | wc -l
>> 20
>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i drv | wc -l
>> 19
>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i driver | wc -l
>> 15
>>
>> It seems that many devices use dev_name(&i2c_client->dev) or
>> otherwise some sort of "ABC123_DRIVER_NAME" constant.
>>
>> It's also not clear what this "name" field is for. Is it more than
>> just a cosmetic sysfs attribute? It seems to me that names don't have
>> to be unique so it would be wrong to use them for identification.
>>
> It's a convenience field really as there is no clear standard for where else
> to find out what a part actually is (i.e. if it is an i2c part you can look
> in the name attribute i2c supplies - but otherwise you are on your own).

I'd like to argue that it is supposed to be the device type allowing the
application to identify which kind of device they are talking to. In which
case the dev_name() initialization is wrong. Unfortunately there seem quite
a few drivers which use it, especially the the SoC ADC drivers. But we can't
really change this for existing drivers since there might be applications
relying on the name.

We should pay more attention to this for new driver submissions and make
sure we don't get any more of this.

As a side note if you want to know the dev_name() of the parent device you
can do a readlink() on the iio device in /sys/bus/iio/devices/. The second
last entry in the path name is the name of the parent device.

Re: [PATCH] iio: tmp006: Set correct iio name

[Yong Li]

Thanks for your mails. Just FYI, we are testing this tmp006 sensor on
our system. and our application framework is using these device names,
so I submitted this patch.

Yong

2016-04-26 18:57 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 04/25/2016 11:11 PM, Jonathan Cameron wrote:
>> On 25/04/16 21:59, Crestez Dan Leonard wrote:
>>> On 04/25/2016 10:33 PM, Jonathan Cameron wrote:
>>>> On 22/04/16 04:43, Yong Li wrote:
>>>>> When load the driver using the below command:
>>>>> echo tmp006 0x40 > /sys/bus/i2c/devices/i2c-0/new_device
>>>>>
>>>>> In sysfs, the i2c name is tmp006, however the iio name is 0-0040,
>>>>> they are inconsistent. With this patch,
>>>>> the iio name will be the same as the i2c device name
>>>>>
>>>>> Signed-off-by: Yong Li <sdliyong@gmail.com>
>>>> Peter, this looks right to me, but could you take a quick look as I guess
>>>> there might be a reason you did this in an unusual way originally?
>>>>
>>> Is there a "correct" or "usual" way to set indio_dev->name? Some quick grepping shows no clear standard:
>>>
>>> $ git grep -h 'indio_dev->name =' drivers/iio/ | wc -l
>>> 148
>>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep id | wc -l
>>> 52
>>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep dev_name | wc -l
>>> 20
>>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i drv | wc -l
>>> 19
>>> $ git grep -h 'indio_dev->name =' drivers/iio/ | grep -i driver | wc -l
>>> 15
>>>
>>> It seems that many devices use dev_name(&i2c_client->dev) or
>>> otherwise some sort of "ABC123_DRIVER_NAME" constant.
>>>
>>> It's also not clear what this "name" field is for. Is it more than
>>> just a cosmetic sysfs attribute? It seems to me that names don't have
>>> to be unique so it would be wrong to use them for identification.
>>>
>> It's a convenience field really as there is no clear standard for where else
>> to find out what a part actually is (i.e. if it is an i2c part you can look
>> in the name attribute i2c supplies - but otherwise you are on your own).
>
> I'd like to argue that it is supposed to be the device type allowing the
> application to identify which kind of device they are talking to. In which
> case the dev_name() initialization is wrong. Unfortunately there seem quite
> a few drivers which use it, especially the the SoC ADC drivers. But we can't
> really change this for existing drivers since there might be applications
> relying on the name.
>
> We should pay more attention to this for new driver submissions and make
> sure we don't get any more of this.
>
> As a side note if you want to know the dev_name() of the parent device you
> can do a readlink() on the iio device in /sys/bus/iio/devices/. The second
> last entry in the path name is the name of the parent device.

Re: [PATCH] iio: tmp006: Set correct iio name

[Lars-Peter Clausen]

On 04/26/2016 01:47 PM, Yong Li wrote:
> Thanks for your mails. Just FYI, we are testing this tmp006 sensor on
> our system. and our application framework is using these device names,
> so I submitted this patch.

Your patch is certainly correct. But the problem is, or rather the question
is, will this break any existing applications that rely on the wrong behavior?

- Lars

Re: [PATCH] iio: tmp006: Set correct iio name

[Yong Li]

I am thinking if there is any application is using this incorrect
name, the application should be fix too

Thanks,
Yong
2016-04-26 20:01 GMT+08:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 04/26/2016 01:47 PM, Yong Li wrote:
>> Thanks for your mails. Just FYI, we are testing this tmp006 sensor on
>> our system. and our application framework is using these device names,
>> so I submitted this patch.
>
> Your patch is certainly correct. But the problem is, or rather the question
> is, will this break any existing applications that rely on the wrong behavior?
>
> - Lars

Re: [PATCH] iio: tmp006: Set correct iio name

[Daniel Baluta]

On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <sdliyong@gmail.com> wrote:
> I am thinking if there is any application is using this incorrect
> name, the application should be fix too

The rule is: "Don't break the userspace ABI". So, if we got this wrong
from the beginning we are stuck with this name.

The only thing that can save the situation is to know that there is no
application relying on the name :).

Re: [PATCH] iio: tmp006: Set correct iio name

[Yong Li]

Thanks for your mails. Is it possible to just merge this patch, then
test if there is any application is using it?  Considering almost all
other I2C devices are using the correct ID name, it should be low
risky

Yong

2016-04-26 23:21 GMT+08:00 Daniel Baluta <daniel.baluta@gmail.com>:
> On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <sdliyong@gmail.com> wrote:
>> I am thinking if there is any application is using this incorrect
>> name, the application should be fix too
>
> The rule is: "Don't break the userspace ABI". So, if we got this wrong
> from the beginning we are stuck with this name.
>
> The only thing that can save the situation is to know that there is no
> application relying on the name :).

Re: [PATCH] iio: tmp006: Set correct iio name

[Crestez Dan Leonard]

On 04/26/2016 06:21 PM, Daniel Baluta wrote:
> On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <sdliyong@gmail.com> wrote:
>> I am thinking if there is any application is using this incorrect
>> name, the application should be fix too
> 
> The rule is: "Don't break the userspace ABI". So, if we got this wrong
> from the beginning we are stuck with this name.
> 
> The only thing that can save the situation is to know that there is no
> application relying on the name :).
> 
But if iio_dev->name is supposed to be the "model name" then setting it
to the i2c dev_name is just plain wrong, right? Correcting this could be
considered a bugfix.

There are also other ways to deal with this in userspace. Perhaps you
could look at $(basename $(readlink /sys/bus/i2c/devices/*/driver))?

-- 
Regards,
Leonard

Re: [PATCH] iio: tmp006: Set correct iio name

[Lars-Peter Clausen]

On 04/27/2016 06:58 PM, Crestez Dan Leonard wrote:
> On 04/26/2016 06:21 PM, Daniel Baluta wrote:
>> On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <sdliyong@gmail.com> wrote:
>>> I am thinking if there is any application is using this incorrect
>>> name, the application should be fix too
>>
>> The rule is: "Don't break the userspace ABI". So, if we got this wrong
>> from the beginning we are stuck with this name.
>>
>> The only thing that can save the situation is to know that there is no
>> application relying on the name :).
>>
> But if iio_dev->name is supposed to be the "model name" then setting it
> to the i2c dev_name is just plain wrong, right? Correcting this could be
> considered a bugfix.

It's clearly wrong. But the problem is there might be an application that
depends on the wrong behavior, the driver has been around for 2.5 years. So
it's difficult to fix. We might just go ahead in this case and take the
chance that nobody will complain. But if somebody complains this will bring
us the wrath of the Linus.

Re: [PATCH] iio: tmp006: Set correct iio name

[One Thousand Gnomes]

> It's clearly wrong. But the problem is there might be an application that
> depends on the wrong behavior, the driver has been around for 2.5 years. So
> it's difficult to fix. We might just go ahead in this case and take the
> chance that nobody will complain. But if somebody complains this will bring
> us the wrath of the Linus.

Not if you put it into next, test it, then into a new release as early as
possible (for -rc1), clearly document that it's got a user visible change
that should not matter with instructions if anyone hits this as a
bisection for their app failing to email so you know and can revert it.

Alan

Re: [PATCH] iio: tmp006: Set correct iio name

[Peter Meerwald-Stadler]

> > It's clearly wrong. But the problem is there might be an application that
> > depends on the wrong behavior, the driver has been around for 2.5 years. So
> > it's difficult to fix. We might just go ahead in this case and take the
> > chance that nobody will complain. But if somebody complains this will bring
> > us the wrath of the Linus.
> 
> Not if you put it into next, test it, then into a new release as early as
> possible (for -rc1), clearly document that it's got a user visible change
> that should not matter with instructions if anyone hits this as a
> bisection for their app failing to email so you know and can revert it.

is this the only driver doing it wrong?

pmeerw@pmeerw:/var/git/linux/drivers/iio$ rgrep "indio_dev->name = dev_name" . 
./imu/inv_mpu6050/inv_mpu_core.c:		indio_dev->name = dev_name(dev);
./light/lm3533-als.c:	indio_dev->name = dev_name(&pdev->dev);
./dac/vf610_dac.c:	indio_dev->name = dev_name(&pdev->dev);
./dac/stx104.c:	indio_dev->name = dev_name(dev);
./dac/lpc18xx_dac.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/mcp3422.c:	indio_dev->name = dev_name(&client->dev);
./adc/at91-sama5d2_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/vf610_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/ti_am335x_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/nau7802.c:	indio_dev->name = dev_name(&client->dev);
./adc/da9150-gpadc.c:	indio_dev->name = dev_name(dev);
./adc/lpc18xx_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/rockchip_saradc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/imx7d_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/cc10001_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/berlin2-adc.c:	indio_dev->name = dev_name(&pdev->dev);
./adc/exynos_adc.c:	indio_dev->name = dev_name(&pdev->dev);
./temperature/tmp006.c:	indio_dev->name = dev_name(&client->dev);
./chemical/ams-iaq-core.c:	indio_dev->name = dev_name(&client->dev);
./chemical/vz89x.c:	indio_dev->name = dev_name(&client->dev);
./humidity/si7005.c:	indio_dev->name = dev_name(&client->dev);
./humidity/hdc100x.c:	indio_dev->name = dev_name(&client->dev);
./humidity/si7020.c:	indio_dev->name = dev_name(&client->dev);

regards, p.

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH] iio: tmp006: Set correct iio name

[Lars-Peter Clausen]

On 04/28/2016 03:24 PM, One Thousand Gnomes wrote:
>> It's clearly wrong. But the problem is there might be an application that
>> depends on the wrong behavior, the driver has been around for 2.5 years. So
>> it's difficult to fix. We might just go ahead in this case and take the
>> chance that nobody will complain. But if somebody complains this will bring
>> us the wrath of the Linus.
> 
> Not if you put it into next, test it, then into a new release as early as
> possible (for -rc1), clearly document that it's got a user visible change
> that should not matter with instructions if anyone hits this as a
> bisection for their app failing to email so you know and can revert it.

I don't expend application developers to run -rc kernels just to check
whether their application still works. You'd get such a report 6 month after
the kernel has been released once the change has trickled down.

Re: [PATCH] iio: tmp006: Set correct iio name

[Lars-Peter Clausen]

On 04/28/2016 03:30 PM, Peter Meerwald-Stadler wrote:
> 
>>> It's clearly wrong. But the problem is there might be an application that
>>> depends on the wrong behavior, the driver has been around for 2.5 years. So
>>> it's difficult to fix. We might just go ahead in this case and take the
>>> chance that nobody will complain. But if somebody complains this will bring
>>> us the wrath of the Linus.
>>
>> Not if you put it into next, test it, then into a new release as early as
>> possible (for -rc1), clearly document that it's got a user visible change
>> that should not matter with instructions if anyone hits this as a
>> bisection for their app failing to email so you know and can revert it.
> 
> is this the only driver doing it wrong?
> 
> pmeerw@pmeerw:/var/git/linux/drivers/iio$ rgrep "indio_dev->name = dev_name" . 
> ./imu/inv_mpu6050/inv_mpu_core.c:		indio_dev->name = dev_name(dev);
> ./light/lm3533-als.c:	indio_dev->name = dev_name(&pdev->dev);
> ./dac/vf610_dac.c:	indio_dev->name = dev_name(&pdev->dev);
> ./dac/stx104.c:	indio_dev->name = dev_name(dev);
> ./dac/lpc18xx_dac.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/mcp3422.c:	indio_dev->name = dev_name(&client->dev);
> ./adc/at91-sama5d2_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/vf610_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/ti_am335x_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/nau7802.c:	indio_dev->name = dev_name(&client->dev);
> ./adc/da9150-gpadc.c:	indio_dev->name = dev_name(dev);
> ./adc/lpc18xx_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/rockchip_saradc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/imx7d_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/cc10001_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/berlin2-adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./adc/exynos_adc.c:	indio_dev->name = dev_name(&pdev->dev);
> ./temperature/tmp006.c:	indio_dev->name = dev_name(&client->dev);
> ./chemical/ams-iaq-core.c:	indio_dev->name = dev_name(&client->dev);
> ./chemical/vz89x.c:	indio_dev->name = dev_name(&client->dev);
> ./humidity/si7005.c:	indio_dev->name = dev_name(&client->dev);
> ./humidity/hdc100x.c:	indio_dev->name = dev_name(&client->dev);
> ./humidity/si7020.c:	indio_dev->name = dev_name(&client->dev);

Yes, they are all wrong. Mostly of it is just copy'n'paste. We need to be
more careful to catch these in the future.

Re: [PATCH] iio: tmp006: Set correct iio name

[Jonathan Cameron]

On 28/04/16 15:19, Lars-Peter Clausen wrote:
> On 04/28/2016 03:30 PM, Peter Meerwald-Stadler wrote:
>>
>>>> It's clearly wrong. But the problem is there might be an application that
>>>> depends on the wrong behavior, the driver has been around for 2.5 years. So
>>>> it's difficult to fix. We might just go ahead in this case and take the
>>>> chance that nobody will complain. But if somebody complains this will bring
>>>> us the wrath of the Linus.
>>>
>>> Not if you put it into next, test it, then into a new release as early as
>>> possible (for -rc1), clearly document that it's got a user visible change
>>> that should not matter with instructions if anyone hits this as a
>>> bisection for their app failing to email so you know and can revert it.
>>
>> is this the only driver doing it wrong?
>>
>> pmeerw@pmeerw:/var/git/linux/drivers/iio$ rgrep "indio_dev->name = dev_name" . 
>> ./imu/inv_mpu6050/inv_mpu_core.c:		indio_dev->name = dev_name(dev);
>> ./light/lm3533-als.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./dac/vf610_dac.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./dac/stx104.c:	indio_dev->name = dev_name(dev);
>> ./dac/lpc18xx_dac.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/mcp3422.c:	indio_dev->name = dev_name(&client->dev);
>> ./adc/at91-sama5d2_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/vf610_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/ti_am335x_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/nau7802.c:	indio_dev->name = dev_name(&client->dev);
>> ./adc/da9150-gpadc.c:	indio_dev->name = dev_name(dev);
>> ./adc/lpc18xx_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/rockchip_saradc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/imx7d_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/cc10001_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/berlin2-adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./adc/exynos_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>> ./temperature/tmp006.c:	indio_dev->name = dev_name(&client->dev);
>> ./chemical/ams-iaq-core.c:	indio_dev->name = dev_name(&client->dev);
>> ./chemical/vz89x.c:	indio_dev->name = dev_name(&client->dev);
>> ./humidity/si7005.c:	indio_dev->name = dev_name(&client->dev);
>> ./humidity/hdc100x.c:	indio_dev->name = dev_name(&client->dev);
>> ./humidity/si7020.c:	indio_dev->name = dev_name(&client->dev);
> 
> Yes, they are all wrong. Mostly of it is just copy'n'paste. We need to be
> more careful to catch these in the future.
> 
<expletive deleted>  Lars is unfortunately right, I don't think
we can unwind this mess now.  If it had been just one driver I'd
have crossed my fingers and taken it - unfortunately this is way
too many.

Only way we could clean this up would be define new ABI with the
intended value and deprecate the old one over a very very long time.
Do we want to do this?

[was iio: tmp006: Set correct iio name] how to support multiple instances of same device type

[Gregor Boirie]

Just to let you know that I'm facing a problem to support multiple 
instances of
the ms5607 barometer onto the same board.

Referencing (i2c) id->name from indio_dev->name is not suitable in this 
case as
libiio and kernel iio tools such as generic_buffer all rely upon a 
unique iio
name to distinguish devices. Here is why.

generic_buffer expects a device name passed as argument to -n option
to get a reference to a device. In this case, it is not possible to
distinguish between devices holding the same name.

When libiio based apps use iio_context_find_device() to get a reference
to a device (by name), names should be unique across devices as above.
When retrieving a device reference by index through iio_context_get_device()
it is not possible to uniquely identify devices in a consistent way either.
Indeed, as indices assignment is automatically done at boot time, it is 
highly
dependent on module / driver loading ordering, devices / daughter board 
presence,
etc...
As a result, apps cannot assume indices are persistent across reboots.

Hence, using dev_name(&client->dev) to name a device is most certainly 
an acceptable
option despites inconsistencies it introduces with i2c name space.

I re-use device tree names to do the job. However, it will not work if 
not compiled
in (and needs patching). What would be a robust and generic naming 
solution then ?

On 05/01/2016 09:34 PM, Jonathan Cameron wrote:
> On 28/04/16 15:19, Lars-Peter Clausen wrote:
>> On 04/28/2016 03:30 PM, Peter Meerwald-Stadler wrote:
>>>>> It's clearly wrong. But the problem is there might be an application that
>>>>> depends on the wrong behavior, the driver has been around for 2.5 years. So
>>>>> it's difficult to fix. We might just go ahead in this case and take the
>>>>> chance that nobody will complain. But if somebody complains this will bring
>>>>> us the wrath of the Linus.
>>>> Not if you put it into next, test it, then into a new release as early as
>>>> possible (for -rc1), clearly document that it's got a user visible change
>>>> that should not matter with instructions if anyone hits this as a
>>>> bisection for their app failing to email so you know and can revert it.
>>> is this the only driver doing it wrong?
>>>
>>> pmeerw@pmeerw:/var/git/linux/drivers/iio$ rgrep "indio_dev->name = dev_name" .
>>> ./imu/inv_mpu6050/inv_mpu_core.c:		indio_dev->name = dev_name(dev);
>>> ./light/lm3533-als.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./dac/vf610_dac.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./dac/stx104.c:	indio_dev->name = dev_name(dev);
>>> ./dac/lpc18xx_dac.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/mcp3422.c:	indio_dev->name = dev_name(&client->dev);
>>> ./adc/at91-sama5d2_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/vf610_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/ti_am335x_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/nau7802.c:	indio_dev->name = dev_name(&client->dev);
>>> ./adc/da9150-gpadc.c:	indio_dev->name = dev_name(dev);
>>> ./adc/lpc18xx_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/rockchip_saradc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/imx7d_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/cc10001_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/berlin2-adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./adc/exynos_adc.c:	indio_dev->name = dev_name(&pdev->dev);
>>> ./temperature/tmp006.c:	indio_dev->name = dev_name(&client->dev);
>>> ./chemical/ams-iaq-core.c:	indio_dev->name = dev_name(&client->dev);
>>> ./chemical/vz89x.c:	indio_dev->name = dev_name(&client->dev);
>>> ./humidity/si7005.c:	indio_dev->name = dev_name(&client->dev);
>>> ./humidity/hdc100x.c:	indio_dev->name = dev_name(&client->dev);
>>> ./humidity/si7020.c:	indio_dev->name = dev_name(&client->dev);
>> Yes, they are all wrong. Mostly of it is just copy'n'paste. We need to be
>> more careful to catch these in the future.
>>
> <expletive deleted>  Lars is unfortunately right, I don't think
> we can unwind this mess now.  If it had been just one driver I'd
> have crossed my fingers and taken it - unfortunately this is way
> too many.
>
> Only way we could clean this up would be define new ABI with the
> intended value and deprecate the old one over a very very long time.
> Do we want to do this?
>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [was iio: tmp006: Set correct iio name] how to support multiple instances of same device type

[Crestez Dan Leonard]

On 05/03/2016 12:43 PM, Gregor Boirie wrote:
> Just to let you know that I'm facing a problem to support multiple
> instances of
> the ms5607 barometer onto the same board.
> 
> Referencing (i2c) id->name from indio_dev->name is not suitable in this
> case as
> libiio and kernel iio tools such as generic_buffer all rely upon a
> unique iio
> name to distinguish devices. Here is why.
> 
> generic_buffer expects a device name passed as argument to -n option
> to get a reference to a device. In this case, it is not possible to
> distinguish between devices holding the same name.
> 
> When libiio based apps use iio_context_find_device() to get a reference
> to a device (by name), names should be unique across devices as above.
> When retrieving a device reference by index through
> iio_context_get_device()
> it is not possible to uniquely identify devices in a consistent way either.
> Indeed, as indices assignment is automatically done at boot time, it is
> highly
> dependent on module / driver loading ordering, devices / daughter board
> presence,
> etc...
> As a result, apps cannot assume indices are persistent across reboots.
> 
> Hence, using dev_name(&client->dev) to name a device is most certainly
> an acceptable
> option despites inconsistencies it introduces with i2c name space.
> 
> I re-use device tree names to do the job. However, it will not work if
> not compiled
> in (and needs patching). What would be a robust and generic naming
> solution then ?
> 
Identifying iio devices by numeric ID seems to be the correct solution.
These are also the minor device numbers, right? Perhaps you can create
devices nodes with pretty names through udev rules. In particular
OF_FULLNAME seems to be available as a property. Example:

# ssh -t local2222 udevadm info /sys/bus/iio/devices/iio:device0
DEVNAME=/dev/iio:device0
DEVTYPE=iio_device
MAJOR=250
MINOR=0
OF_COMPATIBLE_0=inv,mpu6500
OF_COMPATIBLE_N=1
OF_FULLNAME=/spi/mpu6500@0
OF_NAME=mpu6500
SUBSYSTEM=iio

-- 
Regards
Leonard