[PATCH 1/2] iio: bh1780: dereference the client properly

[PATCH 1/2] iio: bh1780: dereference the client properly

[Linus Walleij]

The code in runtime_[suspend|resume] was assuming that the
i2c client data was the bh1780 state container, but it contains
the IIO device. So first dereference the IIO device from the
i2c client, then get the state container using the iio_priv()
call.

Fixes: 1f0477f18306 ("iio: light: new driver for the ROHM BH1780")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I have no idea how this screwed up in the submission, but I
guess I managed to test it with CONFIG_PM disabled or something.
Sorry!
---
 drivers/iio/light/bh1780.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
index f83595334ff1..5fd432df2c8f 100644
--- a/drivers/iio/light/bh1780.c
+++ b/drivers/iio/light/bh1780.c
@@ -226,7 +226,8 @@ static int bh1780_remove(struct i2c_client *client)
 static int bh1780_runtime_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct bh1780_data *bh1780 = i2c_get_clientdata(client);
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct bh1780_data *bh1780 = iio_priv(indio_dev);
 	int ret;
 
 	ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_POFF);
@@ -241,7 +242,8 @@ static int bh1780_runtime_suspend(struct device *dev)
 static int bh1780_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct bh1780_data *bh1780 = i2c_get_clientdata(client);
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct bh1780_data *bh1780 = iio_priv(indio_dev);
 	int ret;
 
 	ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
-- 
2.4.11

[PATCH 2/2] iio: light: bh1780: assign a static name

[Linus Walleij]

Using the struct i2c_device->id field for naming the light sensor
is a bad idea: when booting from the pure device tree this is NULL
and that causes the device not to have the "name" property in
sysfs and that in turn confuses the "lsiio" command to stop listing
devices.

So instead of using the device .id, use the hard string "bh1780",
which works just fine.

Fixes: 1f0477f18306 ("iio: light: new driver for the ROHM BH1780")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/light/bh1780.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
index 5fd432df2c8f..b54dcba05a82 100644
--- a/drivers/iio/light/bh1780.c
+++ b/drivers/iio/light/bh1780.c
@@ -187,7 +187,7 @@ static int bh1780_probe(struct i2c_client *client,
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &bh1780_info;
-	indio_dev->name = id->name;
+	indio_dev->name = "bh1780";
 	indio_dev->channels = bh1780_channels;
 	indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
-- 
2.4.11

Re: [PATCH 2/2] iio: light: bh1780: assign a static name

[Jonathan Cameron]

On 25/05/16 08:40, Linus Walleij wrote:
> Using the struct i2c_device->id field for naming the light sensor
> is a bad idea: when booting from the pure device tree this is NULL
> and that causes the device not to have the "name" property in
> sysfs and that in turn confuses the "lsiio" command to stop listing
> devices.
> 
> So instead of using the device .id, use the hard string "bh1780",
> which works just fine.
> 
> Fixes: 1f0477f18306 ("iio: light: new driver for the ROHM BH1780")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied to the fixes-togreg-post-rc1 and marked for stable.
Thanks,

Jonathan
> ---
>  drivers/iio/light/bh1780.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
> index 5fd432df2c8f..b54dcba05a82 100644
> --- a/drivers/iio/light/bh1780.c
> +++ b/drivers/iio/light/bh1780.c
> @@ -187,7 +187,7 @@ static int bh1780_probe(struct i2c_client *client,
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &bh1780_info;
> -	indio_dev->name = id->name;
> +	indio_dev->name = "bh1780";
>  	indio_dev->channels = bh1780_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> 

Re: [PATCH 2/2] iio: light: bh1780: assign a static name

[Jonathan Cameron]

On 29/05/16 19:53, Jonathan Cameron wrote:
> On 25/05/16 08:40, Linus Walleij wrote:
>> Using the struct i2c_device->id field for naming the light sensor
>> is a bad idea: when booting from the pure device tree this is NULL
>> and that causes the device not to have the "name" property in
>> sysfs and that in turn confuses the "lsiio" command to stop listing
>> devices.
>>
>> So instead of using the device .id, use the hard string "bh1780",
>> which works just fine.
>>
>> Fixes: 1f0477f18306 ("iio: light: new driver for the ROHM BH1780")
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Applied to the fixes-togreg-post-rc1 and marked for stable.
> Thanks,
> 
> Jonathan

Hmm. Just revisiting this whilst reviewing another driver.  What the heck
is the 'right' way of getting hold of the registered name in this case?
I guess getting it from an of lookup if one has been used?
So of_device_match and then the name field?


Anyhow, would just like to get the right answer in my head for future
reviews.  The fixed value here is fine if the driver only supports
one part.

Thanks,

Jonathan

>> ---
>>  drivers/iio/light/bh1780.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
>> index 5fd432df2c8f..b54dcba05a82 100644
>> --- a/drivers/iio/light/bh1780.c
>> +++ b/drivers/iio/light/bh1780.c
>> @@ -187,7 +187,7 @@ static int bh1780_probe(struct i2c_client *client,
>>  
>>  	indio_dev->dev.parent = &client->dev;
>>  	indio_dev->info = &bh1780_info;
>> -	indio_dev->name = id->name;
>> +	indio_dev->name = "bh1780";
>>  	indio_dev->channels = bh1780_channels;
>>  	indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
>>  	indio_dev->modes = INDIO_DIRECT_MODE;
>>
> 
> --
> 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: [PATCH 2/2] iio: light: bh1780: assign a static name

[Linus Walleij]

On Sun, Jun 19, 2016 at 1:25 PM, Jonathan Cameron <jic23@kernel.org> wrote:

>>> Using the struct i2c_device->id field for naming the light sensor
>>> is a bad idea: when booting from the pure device tree this is NULL

> Hmm. Just revisiting this whilst reviewing another driver.  What the heck
> is the 'right' way of getting hold of the registered name in this case?
> I guess getting it from an of lookup if one has been used?
> So of_device_match and then the name field?

Good question.

Some code like drivers/iio/magnetometer/ak8975.c uses this:
dev_name(&client->dev)

Which will give a silly bus name like this (from /proc/interrupts):

234:          0          0    pm8xxx 224 Edge      0-000c

"0-000c" (device 0x0c on i2c bus 0)

It will be unique but not very human readable.

Yours,
Linus Walleij

Re: [PATCH 2/2] iio: light: bh1780: assign a static name

[Jonathan Cameron]

On 19/06/16 19:04, Linus Walleij wrote:
> On Sun, Jun 19, 2016 at 1:25 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
>>>> Using the struct i2c_device->id field for naming the light sensor
>>>> is a bad idea: when booting from the pure device tree this is NULL
> 
>> Hmm. Just revisiting this whilst reviewing another driver.  What the heck
>> is the 'right' way of getting hold of the registered name in this case?
>> I guess getting it from an of lookup if one has been used?
>> So of_device_match and then the name field?
> 
> Good question.
> 
> Some code like drivers/iio/magnetometer/ak8975.c uses this:
> dev_name(&client->dev)
> 
> Which will give a silly bus name like this (from /proc/interrupts):
> 
> 234:          0          0    pm8xxx 224 Edge      0-000c
> 
> "0-000c" (device 0x0c on i2c bus 0)
> 
> It will be unique but not very human readable.
The name field was always really intended to just be the part number.
There are lots of other ways of finding out 'where' it is.

Jonathan
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH 1/2] iio: bh1780: dereference the client properly

[Jonathan Cameron]

On 25/05/16 08:40, Linus Walleij wrote:
> The code in runtime_[suspend|resume] was assuming that the
> i2c client data was the bh1780 state container, but it contains
> the IIO device. So first dereference the IIO device from the
> i2c client, then get the state container using the iio_priv()
> call.
> 
> Fixes: 1f0477f18306 ("iio: light: new driver for the ROHM BH1780")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied to the fixes-togreg-post-rc1 branch of iio.git and marked for
stable.  Thanks,

Jonathan
> ---
> I have no idea how this screwed up in the submission, but I
> guess I managed to test it with CONFIG_PM disabled or something.
> Sorry!
meh, happens to us all from time to time ;)
> ---
>  drivers/iio/light/bh1780.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
> index f83595334ff1..5fd432df2c8f 100644
> --- a/drivers/iio/light/bh1780.c
> +++ b/drivers/iio/light/bh1780.c
> @@ -226,7 +226,8 @@ static int bh1780_remove(struct i2c_client *client)
>  static int bh1780_runtime_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> -	struct bh1780_data *bh1780 = i2c_get_clientdata(client);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct bh1780_data *bh1780 = iio_priv(indio_dev);
>  	int ret;
>  
>  	ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_POFF);
> @@ -241,7 +242,8 @@ static int bh1780_runtime_suspend(struct device *dev)
>  static int bh1780_runtime_resume(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> -	struct bh1780_data *bh1780 = i2c_get_clientdata(client);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct bh1780_data *bh1780 = iio_priv(indio_dev);
>  	int ret;
>  
>  	ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
>