[PATCH 1/2] dt-bindings: iio: light: bh1750: Add device tree binding documentation

[PATCH 1/2] dt-bindings: iio: light: bh1750: Add device tree binding documentation

[ryang]

Document device tree bindings for ROHM BH1750 ambient light sensor driver.

Signed-off-by: ryang <decatf@gmail.com>
---
 .../devicetree/bindings/iio/light/bh1750.txt   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/bh1750.txt

diff --git a/Documentation/devicetree/bindings/iio/light/bh1750.txt b/Documentation/devicetree/bindings/iio/light/bh1750.txt
new file mode 100644
index 000000000000..468350cc7af6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/bh1750.txt
@@ -0,0 +1,18 @@
+ROHM BH1750 - ALS, Ambient light sensor
+
+Required properties:
+
+- compatible: Must be one of:
+    "rohm,bh1710"
+    "rohm,bh1715"
+    "rohm,bh1721"
+    "rohm,bh1750"
+    "rohm,bh1751"
+- reg: the I2C address of the sensor
+
+Example:
+
+bh1750@23 {
+	compatible = "rohm,bh1750";
+	reg = <0x23>;
+};
-- 
2.17.1

[PATCH 2/2] iio: light: bh1750: Add device tree support

[ryang]

Add device tree support for ROHM BH1750 series ambient light sensors.

Signed-off-by: ryang <decatf@gmail.com>
---
 drivers/iio/light/bh1750.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
index a814828e69f5..50b599abb383 100644
--- a/drivers/iio/light/bh1750.c
+++ b/drivers/iio/light/bh1750.c
@@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, bh1750_id);
 
+static const struct of_device_id bh1750_of_match[] = {
+	{ .compatible = "rohm,bh1710", },
+	{ .compatible = "rohm,bh1715", },
+	{ .compatible = "rohm,bh1721", },
+	{ .compatible = "rohm,bh1750", },
+	{ .compatible = "rohm,bh1751", },
+	{ }
+};
+
 static struct i2c_driver bh1750_driver = {
 	.driver = {
 		.name = "bh1750",
+		.of_match_table = of_match_ptr(bh1750_of_match),
 		.pm = BH1750_PM_OPS,
 	},
 	.probe = bh1750_probe,
-- 
2.17.1

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[Jonathan Cameron]

On Sat, 15 Sep 2018 13:42:14 -0400
ryang <decatf@gmail.com> wrote:

> Add device tree support for ROHM BH1750 series ambient light sensors.
> 
> Signed-off-by: ryang <decatf@gmail.com>
Just to check, how is it picking up on the enum value which is provided
in the i2c_device_id entries?

i.e. What is setting id->driver_data in the probe?
There may be something in the i2c core that I'm missing that will do the
association but I don't remember anything trying to do this.

Jonathan

> ---
>  drivers/iio/light/bh1750.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> index a814828e69f5..50b599abb383 100644
> --- a/drivers/iio/light/bh1750.c
> +++ b/drivers/iio/light/bh1750.c
> @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, bh1750_id);
>  
> +static const struct of_device_id bh1750_of_match[] = {
> +	{ .compatible = "rohm,bh1710", },
> +	{ .compatible = "rohm,bh1715", },
> +	{ .compatible = "rohm,bh1721", },
> +	{ .compatible = "rohm,bh1750", },
> +	{ .compatible = "rohm,bh1751", },
> +	{ }
> +};
> +
>  static struct i2c_driver bh1750_driver = {
>  	.driver = {
>  		.name = "bh1750",
> +		.of_match_table = of_match_ptr(bh1750_of_match),
>  		.pm = BH1750_PM_OPS,
>  	},
>  	.probe = bh1750_probe,

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[r yang]

On Sun, Sep 16, 2018 at 10:45:26AM +0100, Jonathan Cameron wrote:
> On Sat, 15 Sep 2018 13:42:14 -0400
> ryang <decatf@gmail.com> wrote:
> 
> > Add device tree support for ROHM BH1750 series ambient light sensors.
> > 
> > Signed-off-by: ryang <decatf@gmail.com>
> Just to check, how is it picking up on the enum value which is provided
> in the i2c_device_id entries?
> 
> i.e. What is setting id->driver_data in the probe?
> There may be something in the i2c core that I'm missing that will do the
> association but I don't remember anything trying to do this.
> 
> Jonathan
>

I've looked through it and it is indeed in the i2c core.
The i2c core probe function picks the id entry based on the i2c client
name.

The i2c client name is assigned based on the device tree compatible
string.

  of_i2c_register_device()
    of_i2c_get_board_info()
      of_modalias_node()

During probe the i2c_device_id is picked by matching the i2c
client->name to i2c_device_id->name in the i2c_match_id() function.

  i2c_device_probe()
    driver->probe(client, i2c_match_id(driver->id_table, client));

I've tested this on device. It's Samsung Galaxy Tab 10.1. It has a
BH1721.

> > ---
> >  drivers/iio/light/bh1750.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> > index a814828e69f5..50b599abb383 100644
> > --- a/drivers/iio/light/bh1750.c
> > +++ b/drivers/iio/light/bh1750.c
> > @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, bh1750_id);
> >  
> > +static const struct of_device_id bh1750_of_match[] = {
> > +	{ .compatible = "rohm,bh1710", },
> > +	{ .compatible = "rohm,bh1715", },
> > +	{ .compatible = "rohm,bh1721", },
> > +	{ .compatible = "rohm,bh1750", },
> > +	{ .compatible = "rohm,bh1751", },
> > +	{ }
> > +};
> > +
> >  static struct i2c_driver bh1750_driver = {
> >  	.driver = {
> >  		.name = "bh1750",
> > +		.of_match_table = of_match_ptr(bh1750_of_match),
> >  		.pm = BH1750_PM_OPS,
> >  	},
> >  	.probe = bh1750_probe,
> 

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[Tomasz Duszynski]

Comments inline.

On Sat, Sep 15, 2018 at 01:42:14PM -0400, ryang wrote:
> Add device tree support for ROHM BH1750 series ambient light sensors.
>
> Signed-off-by: ryang <decatf@gmail.com>
> ---
>  drivers/iio/light/bh1750.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> index a814828e69f5..50b599abb383 100644
> --- a/drivers/iio/light/bh1750.c
> +++ b/drivers/iio/light/bh1750.c
> @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, bh1750_id);
>
> +static const struct of_device_id bh1750_of_match[] = {
> +	{ .compatible = "rohm,bh1710", },
> +	{ .compatible = "rohm,bh1715", },
> +	{ .compatible = "rohm,bh1721", },
> +	{ .compatible = "rohm,bh1750", },
> +	{ .compatible = "rohm,bh1751", },
> +	{ }
> +};
> +

Hmm, I believe this should be exported additionally using
MODULE_DEVICE_TABLE(of, bh1750_of_match). Otherwise module loading
will not work as expected.

Moreover, we can do without DT and if you want to avoid
warning about unused variables you may consider enclosing everything
with:

#ifdef CONFIG_OF
...
#endif

>  static struct i2c_driver bh1750_driver = {
>  	.driver = {
>  		.name = "bh1750",
> +		.of_match_table = of_match_ptr(bh1750_of_match),
>  		.pm = BH1750_PM_OPS,

I've recently simplified setting pm ops so this would not apply to
iio/togreg cleanly.

>  	},
>  	.probe = bh1750_probe,
> --
> 2.17.1
>

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[r yang]

On Wed, Sep 19, 2018 at 09:22:39PM +0200, Tomasz Duszynski wrote:
> Comments inline.
> 
> On Sat, Sep 15, 2018 at 01:42:14PM -0400, ryang wrote:
> > Add device tree support for ROHM BH1750 series ambient light sensors.
> >
> > Signed-off-by: ryang <decatf@gmail.com>
> > ---
> >  drivers/iio/light/bh1750.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> > index a814828e69f5..50b599abb383 100644
> > --- a/drivers/iio/light/bh1750.c
> > +++ b/drivers/iio/light/bh1750.c
> > @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, bh1750_id);
> >
> > +static const struct of_device_id bh1750_of_match[] = {
> > +	{ .compatible = "rohm,bh1710", },
> > +	{ .compatible = "rohm,bh1715", },
> > +	{ .compatible = "rohm,bh1721", },
> > +	{ .compatible = "rohm,bh1750", },
> > +	{ .compatible = "rohm,bh1751", },
> > +	{ }
> > +};
> > +
> 
> Hmm, I believe this should be exported additionally using
> MODULE_DEVICE_TABLE(of, bh1750_of_match). Otherwise module loading
> will not work as expected.
> 
> Moreover, we can do without DT and if you want to avoid
> warning about unused variables you may consider enclosing everything
> with:
> 
> #ifdef CONFIG_OF
> ...
> #endif

The reason for this DT patch is because I've ported the Samsung Galaxy
Tab 10.1 to the mainline kernel. The device is working on the Tegra
platform with the right DTS file. Some peripherals on this device have
existing drivers such as this one which only need the DT bindings for
it be usuable.

> 
> >  static struct i2c_driver bh1750_driver = {
> >  	.driver = {
> >  		.name = "bh1750",
> > +		.of_match_table = of_match_ptr(bh1750_of_match),
> >  		.pm = BH1750_PM_OPS,
> 
> I've recently simplified setting pm ops so this would not apply to
> iio/togreg cleanly.
> 

Does this mean I should wait until that is merged before submitting a V2
patch?

> >  	},
> >  	.probe = bh1750_probe,
> > --
> > 2.17.1
> >

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[Tomasz Duszynski]

On Thu, Sep 20, 2018 at 09:37:21PM -0400, r yang wrote:
> On Wed, Sep 19, 2018 at 09:22:39PM +0200, Tomasz Duszynski wrote:
> > Comments inline.
> >
> > On Sat, Sep 15, 2018 at 01:42:14PM -0400, ryang wrote:
> > > Add device tree support for ROHM BH1750 series ambient light sensors.
> > >
> > > Signed-off-by: ryang <decatf@gmail.com>
> > > ---
> > >  drivers/iio/light/bh1750.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> > > index a814828e69f5..50b599abb383 100644
> > > --- a/drivers/iio/light/bh1750.c
> > > +++ b/drivers/iio/light/bh1750.c
> > > @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, bh1750_id);
> > >
> > > +static const struct of_device_id bh1750_of_match[] = {
> > > +	{ .compatible = "rohm,bh1710", },
> > > +	{ .compatible = "rohm,bh1715", },
> > > +	{ .compatible = "rohm,bh1721", },
> > > +	{ .compatible = "rohm,bh1750", },
> > > +	{ .compatible = "rohm,bh1751", },
> > > +	{ }
> > > +};
> > > +
> >
> > Hmm, I believe this should be exported additionally using
> > MODULE_DEVICE_TABLE(of, bh1750_of_match). Otherwise module loading
> > will not work as expected.
> >
> > Moreover, we can do without DT and if you want to avoid
> > warning about unused variables you may consider enclosing everything
> > with:
> >
> > #ifdef CONFIG_OF
> > ...
> > #endif
>
> The reason for this DT patch is because I've ported the Samsung Galaxy
> Tab 10.1 to the mainline kernel. The device is working on the Tegra
> platform with the right DTS file. Some peripherals on this device have
> existing drivers such as this one which only need the DT bindings for
> it be usuable.
>

Adding support for DT is fine. Just pointed a figner at two things
that attracted my attention.

> >
> > >  static struct i2c_driver bh1750_driver = {
> > >  	.driver = {
> > >  		.name = "bh1750",
> > > +		.of_match_table = of_match_ptr(bh1750_of_match),
> > >  		.pm = BH1750_PM_OPS,
> >
> > I've recently simplified setting pm ops so this would not apply to
> > iio/togreg cleanly.
> >
>
> Does this mean I should wait until that is merged before submitting a V2
> patch?
>

No need to wait. I guess you should rebase your work on iio/togreg
branch and post v2.

> > >  	},
> > >  	.probe = bh1750_probe,
> > > --
> > > 2.17.1
> > >

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[Jonathan Cameron]

On Fri, 21 Sep 2018 17:34:21 +0200
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> On Thu, Sep 20, 2018 at 09:37:21PM -0400, r yang wrote:
> > On Wed, Sep 19, 2018 at 09:22:39PM +0200, Tomasz Duszynski wrote:  
> > > Comments inline.
> > >
> > > On Sat, Sep 15, 2018 at 01:42:14PM -0400, ryang wrote:  
> > > > Add device tree support for ROHM BH1750 series ambient light sensors.
> > > >
> > > > Signed-off-by: ryang <decatf@gmail.com>
> > > > ---
> > > >  drivers/iio/light/bh1750.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> > > > index a814828e69f5..50b599abb383 100644
> > > > --- a/drivers/iio/light/bh1750.c
> > > > +++ b/drivers/iio/light/bh1750.c
> > > > @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, bh1750_id);
> > > >
> > > > +static const struct of_device_id bh1750_of_match[] = {
> > > > +	{ .compatible = "rohm,bh1710", },
> > > > +	{ .compatible = "rohm,bh1715", },
> > > > +	{ .compatible = "rohm,bh1721", },
> > > > +	{ .compatible = "rohm,bh1750", },
> > > > +	{ .compatible = "rohm,bh1751", },
> > > > +	{ }
> > > > +};
> > > > +  
> > >
> > > Hmm, I believe this should be exported additionally using
> > > MODULE_DEVICE_TABLE(of, bh1750_of_match). Otherwise module loading
> > > will not work as expected.
> > >
> > > Moreover, we can do without DT and if you want to avoid
> > > warning about unused variables you may consider enclosing everything
> > > with:
> > >
> > > #ifdef CONFIG_OF
> > > ...
> > > #endif  

Don't do that.  There is a significant oddity now around this in that
it is possible to use a magic ACPI device ID to probe via a devicetree
binding (without CONFIG_OF).  It's seriously odd, but to allow that to
work we need to not protect things with #ifdef CONFIG_OF.

(and this particular bit of fun strikes me as crazy too, but it's what
we have!)

https://www.kernel.org/doc/Documentation/acpi/enumeration.txt
(towards the bottom)

Having MODULE_DEVICE_TABLE is a good addition.

> >
> > The reason for this DT patch is because I've ported the Samsung Galaxy
> > Tab 10.1 to the mainline kernel. The device is working on the Tegra
> > platform with the right DTS file. Some peripherals on this device have
> > existing drivers such as this one which only need the DT bindings for
> > it be usuable.
> >  
> 
> Adding support for DT is fine. Just pointed a figner at two things
> that attracted my attention.
> 
> > >  
> > > >  static struct i2c_driver bh1750_driver = {
> > > >  	.driver = {
> > > >  		.name = "bh1750",
> > > > +		.of_match_table = of_match_ptr(bh1750_of_match),
> > > >  		.pm = BH1750_PM_OPS,  
> > >
> > > I've recently simplified setting pm ops so this would not apply to
> > > iio/togreg cleanly.
> > >  
> >
> > Does this mean I should wait until that is merged before submitting a V2
> > patch?
> >  
> 
> No need to wait. I guess you should rebase your work on iio/togreg
> branch and post v2.

That would be perfect as you are rerolling anyway. Let's me be lazy
and not do the merge ;)

Thanks,

Jonathan

> 
> > > >  	},
> > > >  	.probe = bh1750_probe,
> > > > --
> > > > 2.17.1
> > > >  

Re: [PATCH 2/2] iio: light: bh1750: Add device tree support

[Jonathan Cameron]

On Mon, 17 Sep 2018 21:57:33 -0400
r yang <decatf@gmail.com> wrote:

> On Sun, Sep 16, 2018 at 10:45:26AM +0100, Jonathan Cameron wrote:
> > On Sat, 15 Sep 2018 13:42:14 -0400
> > ryang <decatf@gmail.com> wrote:
> >   
> > > Add device tree support for ROHM BH1750 series ambient light sensors.
> > > 
> > > Signed-off-by: ryang <decatf@gmail.com>  
> > Just to check, how is it picking up on the enum value which is provided
> > in the i2c_device_id entries?
> > 
> > i.e. What is setting id->driver_data in the probe?
> > There may be something in the i2c core that I'm missing that will do the
> > association but I don't remember anything trying to do this.
> > 
> > Jonathan
> >  
> 
> I've looked through it and it is indeed in the i2c core.
> The i2c core probe function picks the id entry based on the i2c client
> name.
> 
> The i2c client name is assigned based on the device tree compatible
> string.
> 
>   of_i2c_register_device()
>     of_i2c_get_board_info()
>       of_modalias_node()
> 
> During probe the i2c_device_id is picked by matching the i2c
> client->name to i2c_device_id->name in the i2c_match_id() function.
> 
>   i2c_device_probe()
>     driver->probe(client, i2c_match_id(driver->id_table, client));
> 
Great!  Thanks for tracking that down.  I'll pick this up once
you've added the MODULE_DEVICE_TABLE as Tomasz suggested.


> I've tested this on device. It's Samsung Galaxy Tab 10.1. It has a
> BH1721.
> 
> > > ---
> > >  drivers/iio/light/bh1750.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> > > index a814828e69f5..50b599abb383 100644
> > > --- a/drivers/iio/light/bh1750.c
> > > +++ b/drivers/iio/light/bh1750.c
> > > @@ -315,9 +315,19 @@ static const struct i2c_device_id bh1750_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, bh1750_id);
> > >  
> > > +static const struct of_device_id bh1750_of_match[] = {
> > > +	{ .compatible = "rohm,bh1710", },
> > > +	{ .compatible = "rohm,bh1715", },
> > > +	{ .compatible = "rohm,bh1721", },
> > > +	{ .compatible = "rohm,bh1750", },
> > > +	{ .compatible = "rohm,bh1751", },
> > > +	{ }
> > > +};
> > > +
> > >  static struct i2c_driver bh1750_driver = {
> > >  	.driver = {
> > >  		.name = "bh1750",
> > > +		.of_match_table = of_match_ptr(bh1750_of_match),
> > >  		.pm = BH1750_PM_OPS,
> > >  	},
> > >  	.probe = bh1750_probe,  
> >