Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1384678 > unrolled thread

[PATCH] iio: tmp006: Set correct iio name

Back to article view | Back to linux.kernel

Contents

  [PATCH] iio: tmp006: Set correct iio name Yong Li <sdliyong@gmail.com> - 2016-04-22 05:50 +0200
    Re: [PATCH] iio: tmp006: Set correct iio name Jonathan Cameron <jic23@kernel.org> - 2016-04-25 21:40 +0200
      Re: [PATCH] iio: tmp006: Set correct iio name Crestez Dan Leonard <cdleonard@gmail.com> - 2016-04-25 23:10 +0200
        Re: [PATCH] iio: tmp006: Set correct iio name Jonathan Cameron <jic23@kernel.org> - 2016-04-25 23:20 +0200
          Re: [PATCH] iio: tmp006: Set correct iio name Lars-Peter Clausen <lars@metafoo.de> - 2016-04-26 13:00 +0200
            Re: [PATCH] iio: tmp006: Set correct iio name Yong Li <sdliyong@gmail.com> - 2016-04-26 13:50 +0200
              Re: [PATCH] iio: tmp006: Set correct iio name Lars-Peter Clausen <lars@metafoo.de> - 2016-04-26 14:10 +0200
                Re: [PATCH] iio: tmp006: Set correct iio name Yong Li <sdliyong@gmail.com> - 2016-04-26 15:20 +0200
                  Re: [PATCH] iio: tmp006: Set correct iio name Daniel Baluta <daniel.baluta@gmail.com> - 2016-04-26 17:30 +0200
                    Re: [PATCH] iio: tmp006: Set correct iio name Yong Li <sdliyong@gmail.com> - 2016-04-27 05:50 +0200
                    Re: [PATCH] iio: tmp006: Set correct iio name Crestez Dan Leonard <leonard.crestez@intel.com> - 2016-04-27 19:00 +0200
                      Re: [PATCH] iio: tmp006: Set correct iio name Lars-Peter Clausen <lars@metafoo.de> - 2016-04-28 10:30 +0200
                        Re: [PATCH] iio: tmp006: Set correct iio name One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-04-28 15:30 +0200
                          Re: [PATCH] iio: tmp006: Set correct iio name Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2016-04-28 15:40 +0200
                            Re: [PATCH] iio: tmp006: Set correct iio name Lars-Peter Clausen <lars@metafoo.de> - 2016-04-28 16:20 +0200
                              Re: [PATCH] iio: tmp006: Set correct iio name Jonathan Cameron <jic23@kernel.org> - 2016-05-01 23:30 +0200
                          Re: [PATCH] iio: tmp006: Set correct iio name Lars-Peter Clausen <lars@metafoo.de> - 2016-04-28 16:50 +0200

#1384678 — [PATCH] iio: tmp006: Set correct iio name

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

[toc] | [next] | [standalone]

#1386792

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

[toc] | [prev] | [next] | [standalone]

#1386895

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

[toc] | [prev] | [next] | [standalone]

#1386896

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

[toc] | [prev] | [next] | [standalone]

#1387365

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.

[toc] | [prev] | [next] | [standalone]

#1387402

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.

[toc] | [prev] | [next] | [standalone]

#1387413

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

[toc] | [prev] | [next] | [standalone]

#1387468

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

[toc] | [prev] | [next] | [standalone]

#1387624

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 :).

[toc] | [prev] | [next] | [standalone]

#1388648

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 :).

[toc] | [prev] | [next] | [standalone]

#1389372

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

[toc] | [prev] | [next] | [standalone]

#1389860

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.

[toc] | [prev] | [next] | [standalone]

#1390188

> 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

[toc] | [prev] | [next] | [standalone]

#1390194

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

[toc] | [prev] | [next] | [standalone]

#1390240

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.

[toc] | [prev] | [next] | [standalone]

#1391901

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?

[toc] | [prev] | [next] | [standalone]

#1390269

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web