| Message ID | 20200412183658.6755-4-mani@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Reset and Wakeup support for CCS811 | expand |
On Mon, 13 Apr 2020 00:06:58 +0530 mani@kernel.org wrote: > From: Manivannan Sadhasivam <mani@kernel.org> > > Add devicetree OF match table support for CCS811 VOC sensor. > > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org> Hi, A few small things to clean up inline Thanks, Jonathan > --- > drivers/iio/chemical/ccs811.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c > index 6cd92c49c348..313931208f61 100644 > --- a/drivers/iio/chemical/ccs811.c > +++ b/drivers/iio/chemical/ccs811.c > @@ -24,6 +24,7 @@ > #include <linux/iio/triggered_buffer.h> > #include <linux/iio/trigger_consumer.h> > #include <linux/module.h> > +#include <linux/of.h> You are including this just to get things in mod_devicetable.h so include that directly instead. > > #define CCS811_STATUS 0x00 > #define CCS811_MEAS_MODE 0x01 > @@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, ccs811_id); > > +static const struct of_device_id ccs811_dt_ids[] = { > + { .compatible = "ams,ccs811" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids); > + > static struct i2c_driver ccs811_driver = { > .driver = { > .name = "ccs811", > + .of_match_table = of_match_ptr(ccs811_dt_ids), No need for the of_match_ptr macro. It has several issues. 1) Blocks PRP001 ACPI magic device types being used to instantiate this using the device tree binding but under ACPI. 2) Will give warnings about ccs811_dt_ids being unused on no device tree builds. > }, > .probe = ccs811_probe, > .remove = ccs811_remove,
On Mon, Apr 13, 2020 at 10:23 AM <mani@kernel.org> wrote: > > From: Manivannan Sadhasivam <mani@kernel.org> > > Add devicetree OF match table support for CCS811 VOC sensor. ... > +#include <linux/of.h> Why? ... > +static const struct of_device_id ccs811_dt_ids[] = { > + { .compatible = "ams,ccs811" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids); Since it has no ugly ifdeffery... > static struct i2c_driver ccs811_driver = { > .driver = { > .name = "ccs811", > + .of_match_table = of_match_ptr(ccs811_dt_ids), ...use of of_match_ptr() brings a compiler warning. Drop of_match_ptr() for good. And thus drop redundant of.h. > },
On Mon, Apr 13, 2020 at 05:50:54PM +0100, Jonathan Cameron wrote: > On Mon, 13 Apr 2020 00:06:58 +0530 > mani@kernel.org wrote: > > > From: Manivannan Sadhasivam <mani@kernel.org> > > > > Add devicetree OF match table support for CCS811 VOC sensor. > > > > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org> > > Hi, > > A few small things to clean up inline > > Thanks, > > Jonathan > > > --- > > drivers/iio/chemical/ccs811.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c > > index 6cd92c49c348..313931208f61 100644 > > --- a/drivers/iio/chemical/ccs811.c > > +++ b/drivers/iio/chemical/ccs811.c > > @@ -24,6 +24,7 @@ > > #include <linux/iio/triggered_buffer.h> > > #include <linux/iio/trigger_consumer.h> > > #include <linux/module.h> > > +#include <linux/of.h> > > You are including this just to get things in mod_devicetable.h > so include that directly instead. > I added this include for of_match_ptr. Since it is not needed anymore, I'll drop this. > > > > #define CCS811_STATUS 0x00 > > #define CCS811_MEAS_MODE 0x01 > > @@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, ccs811_id); > > > > +static const struct of_device_id ccs811_dt_ids[] = { > > + { .compatible = "ams,ccs811" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids); > > + > > static struct i2c_driver ccs811_driver = { > > .driver = { > > .name = "ccs811", > > + .of_match_table = of_match_ptr(ccs811_dt_ids), > No need for the of_match_ptr macro. It has several issues. > > 1) Blocks PRP001 ACPI magic device types being used to instantiate > this using the device tree binding but under ACPI. > 2) Will give warnings about ccs811_dt_ids being unused on no > device tree builds. > Okay, will use ccs811_dt_ids directly. Thanks, Mani > > }, > > .probe = ccs811_probe, > > .remove = ccs811_remove, >
On Mon, Apr 13, 2020 at 11:23:03PM +0300, Andy Shevchenko wrote: > On Mon, Apr 13, 2020 at 10:23 AM <mani@kernel.org> wrote: > > > > From: Manivannan Sadhasivam <mani@kernel.org> > > > > Add devicetree OF match table support for CCS811 VOC sensor. > > ... > > > +#include <linux/of.h> > > Why? > As replied to Jonathan, will drop this and of_match_ptr. Thanks, Mani > ... > > > +static const struct of_device_id ccs811_dt_ids[] = { > > + { .compatible = "ams,ccs811" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids); > > Since it has no ugly ifdeffery... > > > static struct i2c_driver ccs811_driver = { > > .driver = { > > .name = "ccs811", > > + .of_match_table = of_match_ptr(ccs811_dt_ids), > > ...use of of_match_ptr() brings a compiler warning. > > Drop of_match_ptr() for good. And thus drop redundant of.h. > > > }, > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c index 6cd92c49c348..313931208f61 100644 --- a/drivers/iio/chemical/ccs811.c +++ b/drivers/iio/chemical/ccs811.c @@ -24,6 +24,7 @@ #include <linux/iio/triggered_buffer.h> #include <linux/iio/trigger_consumer.h> #include <linux/module.h> +#include <linux/of.h> #define CCS811_STATUS 0x00 #define CCS811_MEAS_MODE 0x01 @@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = { }; MODULE_DEVICE_TABLE(i2c, ccs811_id); +static const struct of_device_id ccs811_dt_ids[] = { + { .compatible = "ams,ccs811" }, + { } +}; +MODULE_DEVICE_TABLE(of, ccs811_dt_ids); + static struct i2c_driver ccs811_driver = { .driver = { .name = "ccs811", + .of_match_table = of_match_ptr(ccs811_dt_ids), }, .probe = ccs811_probe, .remove = ccs811_remove,