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

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

[Hans de Goede]

Hi,

On 04-12-17 10:44, Jonathan Cameron wrote:
> On Mon, 4 Dec 2017 09:29:38 +0100
> Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>
>>>    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

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

[Javier Martinez Canillas]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>
>>>   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

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

[Jonathan Cameron]

On Mon, 4 Dec 2017 11:24:40 +0100
Javier Martinez Canillas <javierm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>>
> >>>   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,

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html