* [PATCH] iio: accel: bmc150: Add OF device ID table @ 2017-12-01 11:10 Javier Martinez Canillas 2017-12-02 12:02 ` Jonathan Cameron 2017-12-04 8:29 ` Hans de Goede 0 siblings, 2 replies; 9+ messages in thread From: Javier Martinez Canillas @ 2017-12-01 11:10 UTC (permalink / raw) To: linux-kernel Cc: Javier Martinez Canillas, Hartmut Knaack, Hans de Goede, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler The driver doesn't have a struct of_device_id table but supported devices are registered via Device Trees. This is working on the assumption that a I2C device registered via OF will always match a legacy I2C device ID and that the MODALIAS reported will always be of the form i2c:<device>. But this could change in the future so the correct approach is to have an OF device ID table if the devices are registered via OF. The I2C device ID table entries have the .driver_data field set, but they are not used in the driver so weren't set in the OF device table entries. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c index f85014fbaa12..8ffc308d5fd0 100644 --- a/drivers/iio/accel/bmc150-accel-i2c.c +++ b/drivers/iio/accel/bmc150-accel-i2c.c @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); +static const struct of_device_id bmc150_accel_of_match[] = { + { .compatible = "bosch,bmc150_accel" }, + { .compatible = "bosch,bmi055_accel" }, + { .compatible = "bosch,bma255" }, + { .compatible = "bosch,bma250e" }, + { .compatible = "bosch,bma222e" }, + { .compatible = "bosch,bma280" }, + { }, +}; +MODULE_DEVICE_TABLE(of, bmc150_accel_of_match); + static struct i2c_driver bmc150_accel_driver = { .driver = { .name = "bmc150_accel_i2c", + .of_match_table = bmc150_accel_of_match, .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match), .pm = &bmc150_accel_pm_ops, }, -- 2.14.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-01 11:10 [PATCH] iio: accel: bmc150: Add OF device ID table Javier Martinez Canillas @ 2017-12-02 12:02 ` Jonathan Cameron 2017-12-03 1:11 ` Javier Martinez Canillas 2017-12-04 8:29 ` Hans de Goede 1 sibling, 1 reply; 9+ messages in thread From: Jonathan Cameron @ 2017-12-02 12:02 UTC (permalink / raw) To: Javier Martinez Canillas Cc: linux-kernel, Hartmut Knaack, Hans de Goede, linux-iio, Lars-Peter Clausen, Peter Meerwald-Stadler On Fri, 1 Dec 2017 12:10:58 +0100 Javier Martinez Canillas <javierm@redhat.com> wrote: > The driver doesn't have a struct of_device_id table but supported devices > are registered via Device Trees. This is working on the assumption that a > I2C device registered via OF will always match a legacy I2C device ID and > that the MODALIAS reported will always be of the form i2c:<device>. > > But this could change in the future so the correct approach is to have an > OF device ID table if the devices are registered via OF. > > The I2C device ID table entries have the .driver_data field set, but they > are not used in the driver so weren't set in the OF device table entries. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Would be nice to do the spi counterpart at somepoint, but as this is clearly an improvement on nothing I applied this one as step 1. Good point about the data fields though - we should probably clean those out as misleading. Thanks, Jonathan > --- > > drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index f85014fbaa12..8ffc308d5fd0 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { > > MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); > > +static const struct of_device_id bmc150_accel_of_match[] = { > + { .compatible = "bosch,bmc150_accel" }, > + { .compatible = "bosch,bmi055_accel" }, > + { .compatible = "bosch,bma255" }, > + { .compatible = "bosch,bma250e" }, > + { .compatible = "bosch,bma222e" }, > + { .compatible = "bosch,bma280" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, bmc150_accel_of_match); > + > static struct i2c_driver bmc150_accel_driver = { > .driver = { > .name = "bmc150_accel_i2c", > + .of_match_table = bmc150_accel_of_match, > .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match), > .pm = &bmc150_accel_pm_ops, > }, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-02 12:02 ` Jonathan Cameron @ 2017-12-03 1:11 ` Javier Martinez Canillas 0 siblings, 0 replies; 9+ messages in thread From: Javier Martinez Canillas @ 2017-12-03 1:11 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-kernel, Hartmut Knaack, Hans de Goede, linux-iio, Lars-Peter Clausen, Peter Meerwald-Stadler Hello Jonathan, On 12/02/2017 01:02 PM, Jonathan Cameron wrote: > On Fri, 1 Dec 2017 12:10:58 +0100 > Javier Martinez Canillas <javierm@redhat.com> wrote: > >> The driver doesn't have a struct of_device_id table but supported devices >> are registered via Device Trees. This is working on the assumption that a >> I2C device registered via OF will always match a legacy I2C device ID and >> that the MODALIAS reported will always be of the form i2c:<device>. >> >> But this could change in the future so the correct approach is to have an >> OF device ID table if the devices are registered via OF. >> >> The I2C device ID table entries have the .driver_data field set, but they >> are not used in the driver so weren't set in the OF device table entries. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Applied to the togreg branch of iio.git and pushed out as testing > for the autobuilders to play with it. > > Would be nice to do the spi counterpart at somepoint, but as this is > clearly an improvement on nothing I applied this one as step 1. > Ok, I can do the same for SPI. I'll post a patch for it next week. > Good point about the data fields though - we should probably clean those > out as misleading. > Same for this, I can post a cleanup patch to get rid of the .data fields. > Thanks, > > Jonathan Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-01 11:10 [PATCH] iio: accel: bmc150: Add OF device ID table Javier Martinez Canillas 2017-12-02 12:02 ` Jonathan Cameron @ 2017-12-04 8:29 ` Hans de Goede 2017-12-04 9:01 ` Javier Martinez Canillas [not found] ` <20171204092259.00006250@huawei.com> 1 sibling, 2 replies; 9+ messages in thread From: Hans de Goede @ 2017-12-04 8:29 UTC (permalink / raw) To: Javier Martinez Canillas, linux-kernel Cc: Hartmut Knaack, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler Hi, On 01-12-17 12:10, Javier Martinez Canillas wrote: > The driver doesn't have a struct of_device_id table but supported devices > are registered via Device Trees. This is working on the assumption that a > I2C device registered via OF will always match a legacy I2C device ID and > that the MODALIAS reported will always be of the form i2c:<device>. > > But this could change in the future so the correct approach is to have an > OF device ID table if the devices are registered via OF. > > The I2C device ID table entries have the .driver_data field set, but they > are not used in the driver so weren't set in the OF device table entries. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index f85014fbaa12..8ffc308d5fd0 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { > > MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); > > +static const struct of_device_id bmc150_accel_of_match[] = { > + { .compatible = "bosch,bmc150_accel" }, > + { .compatible = "bosch,bmi055_accel" }, These look a bit weird, there is no reason to mirror the i2c_device_ids here and typically for devicetree / of we only list the chip model without some postfix like _accel. Also if you're doing this you should probably add a: Documentation/devicetree/bindings/iio/accel/bmc150.txt file documenting the compatible strings, and Cc: devicetree@vger.kernel.org for the next version, so that the devicetree maintainers get a chance to review this. > + { .compatible = "bosch,bma255" }, > + { .compatible = "bosch,bma250e" }, > + { .compatible = "bosch,bma222e" }, > + { .compatible = "bosch,bma280" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, bmc150_accel_of_match); > + > static struct i2c_driver bmc150_accel_driver = { > .driver = { > .name = "bmc150_accel_i2c", > + .of_match_table = bmc150_accel_of_match, > .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match), > .pm = &bmc150_accel_pm_ops, > }, > Otherwise looks good to me, Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-04 8:29 ` Hans de Goede @ 2017-12-04 9:01 ` Javier Martinez Canillas 2017-12-04 9:36 ` Hans de Goede [not found] ` <20171204092259.00006250@huawei.com> 1 sibling, 1 reply; 9+ messages in thread From: Javier Martinez Canillas @ 2017-12-04 9:01 UTC (permalink / raw) To: Hans de Goede, linux-kernel Cc: Hartmut Knaack, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler Hi Hans, Thanks a lot for your feedback. On 12/04/2017 09:29 AM, Hans de Goede wrote: > Hi, > > On 01-12-17 12:10, Javier Martinez Canillas wrote: >> The driver doesn't have a struct of_device_id table but supported devices >> are registered via Device Trees. This is working on the assumption that a >> I2C device registered via OF will always match a legacy I2C device ID and >> that the MODALIAS reported will always be of the form i2c:<device>. >> >> But this could change in the future so the correct approach is to have an >> OF device ID table if the devices are registered via OF. >> >> The I2C device ID table entries have the .driver_data field set, but they >> are not used in the driver so weren't set in the OF device table entries. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >> index f85014fbaa12..8ffc308d5fd0 100644 >> --- a/drivers/iio/accel/bmc150-accel-i2c.c >> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >> @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { >> >> MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); >> >> +static const struct of_device_id bmc150_accel_of_match[] = { >> + { .compatible = "bosch,bmc150_accel" }, >> + { .compatible = "bosch,bmi055_accel" }, > > These look a bit weird, there is no reason to mirror the i2c_device_ids The reason why I changed this is to make sure that module auto-loading will not regress after the following patch is merged: https://patchwork.kernel.org/patch/10089425/ It's true that only DT nodes with compatible "bosch,bma250" and "bosch,bma250e" are used in Device Tree source files in mainline, but I didn't know if others could be used by out-of-tree DTB. > here and typically for devicetree / of we only list > the chip model without some postfix like _accel. > Right, I also wondered about that, but then I saw that there's a DT binding for another chips with the same model but a different function ("bosch,bmc150_magn"). Documentation/devicetree/bindings/iio/magnetometer/bmc150_magn.txt So I thought it could be a convention for bosch chips. > Also if you're doing this you should probably add a: > Documentation/devicetree/bindings/iio/accel/bmc150.txt > file documenting the compatible strings, and Cc: > devicetree@vger.kernel.org for the next version, > so that the devicetree maintainers get a chance to review > this. > Indeed, sorry for missing that. I posted patches for several drivers so I forgot to add the DT binding docs. My main goal was just to prevent the driver to use the I2C device table to match the driver when the devices are registered via OF and to have proper OF module aliases in the driver module so udev could autoload the module. I preferred someone familiar with the device to write the DT binding document. >> + { .compatible = "bosch,bma255" }, >> + { .compatible = "bosch,bma250e" }, >> + { .compatible = "bosch,bma222e" }, >> + { .compatible = "bosch,bma280" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, bmc150_accel_of_match); >> + >> static struct i2c_driver bmc150_accel_driver = { >> .driver = { >> .name = "bmc150_accel_i2c", >> + .of_match_table = bmc150_accel_of_match, >> .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match), >> .pm = &bmc150_accel_pm_ops, >> }, >> > > Otherwise looks good to me, > > Regards, > > Hans > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-04 9:01 ` Javier Martinez Canillas @ 2017-12-04 9:36 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-12-04 9:36 UTC (permalink / raw) To: Javier Martinez Canillas, linux-kernel Cc: Hartmut Knaack, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler Hi, On 04-12-17 10:01, Javier Martinez Canillas wrote: > Hi Hans, > > Thanks a lot for your feedback. > > On 12/04/2017 09:29 AM, Hans de Goede wrote: >> Hi, >> >> On 01-12-17 12:10, Javier Martinez Canillas wrote: >>> The driver doesn't have a struct of_device_id table but supported devices >>> are registered via Device Trees. This is working on the assumption that a >>> I2C device registered via OF will always match a legacy I2C device ID and >>> that the MODALIAS reported will always be of the form i2c:<device>. >>> >>> But this could change in the future so the correct approach is to have an >>> OF device ID table if the devices are registered via OF. >>> >>> The I2C device ID table entries have the .driver_data field set, but they >>> are not used in the driver so weren't set in the OF device table entries. >>> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >>> index f85014fbaa12..8ffc308d5fd0 100644 >>> --- a/drivers/iio/accel/bmc150-accel-i2c.c >>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >>> @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { >>> >>> MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); >>> >>> +static const struct of_device_id bmc150_accel_of_match[] = { >>> + { .compatible = "bosch,bmc150_accel" }, >>> + { .compatible = "bosch,bmi055_accel" }, >> >> These look a bit weird, there is no reason to mirror the i2c_device_ids > > The reason why I changed this is to make sure that module auto-loading will not > regress after the following patch is merged: > > https://patchwork.kernel.org/patch/10089425/ > > It's true that only DT nodes with compatible "bosch,bma250" and "bosch,bma250e" > are used in Device Tree source files in mainline, but I didn't know if others > could be used by out-of-tree DTB. > >> here and typically for devicetree / of we only list >> the chip model without some postfix like _accel. >> > > Right, I also wondered about that, but then I saw that there's a DT binding for > another chips with the same model but a different function ("bosch,bmc150_magn"). > > Documentation/devicetree/bindings/iio/magnetometer/bmc150_magn.txt > > So I thought it could be a convention for bosch chips. Ah good point, at least for the bcm150 which really is a cluster of i2c chips at different addresses the postfix does make sense, you're right. >> Also if you're doing this you should probably add a: >> Documentation/devicetree/bindings/iio/accel/bmc150.txt >> file documenting the compatible strings, and Cc: >> devicetree@vger.kernel.org for the next version, >> so that the devicetree maintainers get a chance to review >> this. >> > > Indeed, sorry for missing that. I posted patches for several drivers so I forgot > to add the DT binding docs. My main goal was just to prevent the driver to use > the I2C device table to match the driver when the devices are registered via OF > and to have proper OF module aliases in the driver module so udev could autoload > the module. > > I preferred someone familiar with the device to write the DT binding document. Ok and I see this has already be merged, which is fine by me, so lets just keep this as is for now :) Regards, Hans > >>> + { .compatible = "bosch,bma255" }, >>> + { .compatible = "bosch,bma250e" }, >>> + { .compatible = "bosch,bma222e" }, >>> + { .compatible = "bosch,bma280" }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(of, bmc150_accel_of_match); >>> + >>> static struct i2c_driver bmc150_accel_driver = { >>> .driver = { >>> .name = "bmc150_accel_i2c", >>> + .of_match_table = bmc150_accel_of_match, >>> .acpi_match_table = ACPI_PTR(bmc150_accel_acpi_match), >>> .pm = &bmc150_accel_pm_ops, >>> }, >>> >> >> Otherwise looks good to me, >> >> Regards, >> >> Hans >> > > Best regards, > ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20171204092259.00006250@huawei.com>]
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table [not found] ` <20171204092259.00006250@huawei.com> @ 2017-12-04 9:47 ` Hans de Goede 2017-12-04 10:24 ` Javier Martinez Canillas 1 sibling, 0 replies; 9+ messages in thread From: Hans de Goede @ 2017-12-04 9:47 UTC (permalink / raw) To: Jonathan Cameron Cc: Javier Martinez Canillas, linux-kernel, Hartmut Knaack, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler, devicetree, Wolfram Sang Hi, On 04-12-17 10:44, Jonathan Cameron wrote: > On Mon, 4 Dec 2017 09:29:38 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 01-12-17 12:10, Javier Martinez Canillas wrote: >>> The driver doesn't have a struct of_device_id table but supported devices >>> are registered via Device Trees. This is working on the assumption that a >>> I2C device registered via OF will always match a legacy I2C device ID and >>> that the MODALIAS reported will always be of the form i2c:<device>. >>> >>> But this could change in the future so the correct approach is to have an >>> OF device ID table if the devices are registered via OF. >>> >>> The I2C device ID table entries have the .driver_data field set, but they >>> are not used in the driver so weren't set in the OF device table entries. >>> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >>> index f85014fbaa12..8ffc308d5fd0 100644 >>> --- a/drivers/iio/accel/bmc150-accel-i2c.c >>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >>> @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { >>> >>> MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); >>> >>> +static const struct of_device_id bmc150_accel_of_match[] = { >>> + { .compatible = "bosch,bmc150_accel" }, >>> + { .compatible = "bosch,bmi055_accel" }, >> >> These look a bit weird, there is no reason to mirror the i2c_device_ids > > There has been a steady move for a long time to add these IDs with the plan > that we would stop automatically matching against the manufacturer free > i2c IDs. Mostly on the basis that was a hack that brought a lot > of effectively unreviewed device tree bindings. As I understand it the > eventual plan is to be able to get rid of that old path entirely... > +CC Wolfram to see what his view is on this. > >> here and typically for devicetree / of we only list >> the chip model without some postfix like _accel. >> > > There is a reason for this and we've been round the houses a few times before > with the (admittedly horrible) conclusion that we don't really have a better way. > > These are multiple chips in one package wired to the same i2c bus > there is no sensible way of telling the kernel that we actually > have two separate devices with the same part number. We could just declare > that we will only support them under the IDs of the individual chips but, > without scraping datasheets it's very difficult to tell which two parts > have been combined in a given SKU (some manufacturers document this - some > don't and we just have to figure it out). Ack, Javier pointed this out to me too and you're both right :) Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table [not found] ` <20171204092259.00006250@huawei.com> 2017-12-04 9:47 ` Hans de Goede @ 2017-12-04 10:24 ` Javier Martinez Canillas 2017-12-10 16:12 ` Jonathan Cameron 1 sibling, 1 reply; 9+ messages in thread From: Javier Martinez Canillas @ 2017-12-04 10:24 UTC (permalink / raw) To: Jonathan Cameron, Hans de Goede Cc: linux-kernel, Hartmut Knaack, linux-iio, Lars-Peter Clausen, Jonathan Cameron, Peter Meerwald-Stadler, devicetree, Wolfram Sang Hello Jonathan, On 12/04/2017 10:44 AM, Jonathan Cameron wrote: > On Mon, 4 Dec 2017 09:29:38 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 01-12-17 12:10, Javier Martinez Canillas wrote: >>> The driver doesn't have a struct of_device_id table but supported devices >>> are registered via Device Trees. This is working on the assumption that a >>> I2C device registered via OF will always match a legacy I2C device ID and >>> that the MODALIAS reported will always be of the form i2c:<device>. >>> >>> But this could change in the future so the correct approach is to have an >>> OF device ID table if the devices are registered via OF. >>> >>> The I2C device ID table entries have the .driver_data field set, but they >>> are not used in the driver so weren't set in the OF device table entries. >>> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> >>> drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >>> index f85014fbaa12..8ffc308d5fd0 100644 >>> --- a/drivers/iio/accel/bmc150-accel-i2c.c >>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >>> @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { >>> >>> MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); >>> >>> +static const struct of_device_id bmc150_accel_of_match[] = { >>> + { .compatible = "bosch,bmc150_accel" }, >>> + { .compatible = "bosch,bmi055_accel" }, >> >> These look a bit weird, there is no reason to mirror the i2c_device_ids > > There has been a steady move for a long time to add these IDs with the plan > that we would stop automatically matching against the manufacturer free > i2c IDs. Mostly on the basis that was a hack that brought a lot Matching using OF IDs have been working for some time (since v4.10 AFAICT) after the following commit: da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices"). The only remaining problem is with module auto-loading. > of effectively unreviewed device tree bindings. As I understand it the > eventual plan is to be able to get rid of that old path entirely... > +CC Wolfram to see what his view is on this. > I don't think we can get rid of the old path entirely since are valid use cases for it. For example when the I2C devices are registered with the i2c_new_device interface where the bus and address are declared in a struct i2c_board_info (ie: old platforms that still use board files or devices with an embedded I2C chip). What I think though is that drivers should only be required to define the device table for the firmware interface used to instantiate them. For example, a driver for a device that's DT-only should only have an OF device ID table just like a driver for an ACPI-only device only requires to have an ACPI ID table. Conversely, a driver for a device that's only instantiated using platform data should only have an I2C device ID table. If a driver supports both DT and legacy platforms, then it's OK to have both ID tables defined. What is not correct is to require OF-only drivers to have an I2C device ID table just as a workaround to have their modules auto-loading working. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio: accel: bmc150: Add OF device ID table 2017-12-04 10:24 ` Javier Martinez Canillas @ 2017-12-10 16:12 ` Jonathan Cameron 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2017-12-10 16:12 UTC (permalink / raw) To: Javier Martinez Canillas Cc: Jonathan Cameron, Hans de Goede, linux-kernel, Hartmut Knaack, linux-iio, Lars-Peter Clausen, Peter Meerwald-Stadler, devicetree, Wolfram Sang On Mon, 4 Dec 2017 11:24:40 +0100 Javier Martinez Canillas <javierm@redhat.com> wrote: > Hello Jonathan, > > On 12/04/2017 10:44 AM, Jonathan Cameron wrote: > > On Mon, 4 Dec 2017 09:29:38 +0100 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> Hi, > >> > >> On 01-12-17 12:10, Javier Martinez Canillas wrote: > >>> The driver doesn't have a struct of_device_id table but supported devices > >>> are registered via Device Trees. This is working on the assumption that a > >>> I2C device registered via OF will always match a legacy I2C device ID and > >>> that the MODALIAS reported will always be of the form i2c:<device>. > >>> > >>> But this could change in the future so the correct approach is to have an > >>> OF device ID table if the devices are registered via OF. > >>> > >>> The I2C device ID table entries have the .driver_data field set, but they > >>> are not used in the driver so weren't set in the OF device table entries. > >>> > >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > >>> --- > >>> > >>> drivers/iio/accel/bmc150-accel-i2c.c | 12 ++++++++++++ > >>> 1 file changed, 12 insertions(+) > >>> > >>> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > >>> index f85014fbaa12..8ffc308d5fd0 100644 > >>> --- a/drivers/iio/accel/bmc150-accel-i2c.c > >>> +++ b/drivers/iio/accel/bmc150-accel-i2c.c > >>> @@ -81,9 +81,21 @@ static const struct i2c_device_id bmc150_accel_id[] = { > >>> > >>> MODULE_DEVICE_TABLE(i2c, bmc150_accel_id); > >>> > >>> +static const struct of_device_id bmc150_accel_of_match[] = { > >>> + { .compatible = "bosch,bmc150_accel" }, > >>> + { .compatible = "bosch,bmi055_accel" }, > >> > >> These look a bit weird, there is no reason to mirror the i2c_device_ids > > > > There has been a steady move for a long time to add these IDs with the plan > > that we would stop automatically matching against the manufacturer free > > i2c IDs. Mostly on the basis that was a hack that brought a lot > > Matching using OF IDs have been working for some time (since v4.10 AFAICT) > after the following commit: > > da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices"). > > The only remaining problem is with module auto-loading. > > > of effectively unreviewed device tree bindings. As I understand it the > > eventual plan is to be able to get rid of that old path entirely... > > +CC Wolfram to see what his view is on this. > > > > I don't think we can get rid of the old path entirely since are valid use cases > for it. For example when the I2C devices are registered with the i2c_new_device > interface where the bus and address are declared in a struct i2c_board_info (ie: > old platforms that still use board files or devices with an embedded I2C chip). Agreed. I only meant the use of that path when matching device tree IDs. There are still reasons to use it otherwise - including the ones you mention and indeed manually adding the device - commonly done with various sensors supported by lm-sensors on x86 boards. These are often not described in any way at all. > > What I think though is that drivers should only be required to define the device > table for the firmware interface used to instantiate them. For example, a driver > for a device that's DT-only should only have an OF device ID table just like a > driver for an ACPI-only device only requires to have an ACPI ID table. > > Conversely, a driver for a device that's only instantiated using platform data > should only have an I2C device ID table. > A lot of drivers are used on both ACPI and DT platforms. For newer cases we perhaps don't need the i2c table. > If a driver supports both DT and legacy platforms, then it's OK to have both ID > tables defined. What is not correct is to require OF-only drivers to have an I2C > device ID table just as a workaround to have their modules auto-loading working. Absolutely agree. Jonathan > > Best regards, ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-10 16:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-01 11:10 [PATCH] iio: accel: bmc150: Add OF device ID table Javier Martinez Canillas
2017-12-02 12:02 ` Jonathan Cameron
2017-12-03 1:11 ` Javier Martinez Canillas
2017-12-04 8:29 ` Hans de Goede
2017-12-04 9:01 ` Javier Martinez Canillas
2017-12-04 9:36 ` Hans de Goede
[not found] ` <20171204092259.00006250@huawei.com>
2017-12-04 9:47 ` Hans de Goede
2017-12-04 10:24 ` Javier Martinez Canillas
2017-12-10 16:12 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).