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 <[email protected]>
---
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
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 <[email protected]>
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;
>
>
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 <[email protected]>
> 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
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 <[email protected]>
>> 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
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 <[email protected]>
>>> 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.
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 <[email protected]>:
> 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 <[email protected]>
>>>> 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.
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
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 <[email protected]>:
> 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
On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <[email protected]> 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 :).
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 <[email protected]>:
> On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <[email protected]> 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 :).
On 04/26/2016 06:21 PM, Daniel Baluta wrote:
> On Tue, Apr 26, 2016 at 4:14 PM, Yong Li <[email protected]> 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
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 <[email protected]> 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.
> 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
> > 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)
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.
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.