Re: [PATCH] iio: accel: bmc150: Add OF device ID table

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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).

> 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.

As stated above these are existing bindings just being formally stated.
We are stuck with them.  I have no problem adding additional bindings and
marking these as deprecated if we come up with some better ones.
+cc devicetree.  Rob / Mark let me know if you want to see these patches
in future.

Most devices are long done now anyway (I think) so the flow is not too
large!

> 
> > +	{ .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
> --
> 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