| Message ID | 387c145949692c0318f90d98426be95c82ea2e58.1537033044.git.decatf@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: iio: light: bh1750: Add device tree binding documentation | expand |
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,
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, >
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 >
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 > >
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 > > >
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 > > > >
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, > >
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,
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(+)