On 09/18/13 16:11, Grant Likely wrote: > On Wed, 18 Sep 2013 14:58:04 +0100, Mark Rutland <mark.rutland@xxxxxxx> wrote: >> On Tue, Sep 17, 2013 at 02:48:41PM +0100, Lee Jones wrote: >>> On Tue, 17 Sep 2013, Mark Rutland wrote: >>> >>>> On Tue, Sep 17, 2013 at 01:07:30PM +0100, Lee Jones wrote: >>>>> On Tue, 17 Sep 2013, Mark Rutland wrote: >>>>> >>>>>> On Tue, Sep 17, 2013 at 09:10:03AM +0100, Lee Jones wrote: >>>>>>>>> http://marc.info/?l=linux-iio&m=137881790217809&w=2 >>>>>>>> Looks reasonable. My only comment is '-' is preferred over '_'. >>>>>>> Ah, in the node name you mean. Yes, I can change that. >>>>>>> >>>>>>> The compatible string has to stay the same however, as it has to match >>>>>>> the device name which can't be changed due to possible userspace >>>>>>> breakage. >>>>>> I'm not sure I follow why the compatible string must be identical to the >>>>>> name appearing to userspace. >>>>> It doesn't. It has to be identical to the device name, which I'm >>>>> guessing is used when populating sysfs. >>>> I still don't understand why the device name must be identical to the >>>> compatible string. >>>> >>>> The string "st,lsm303dlh_magn" (with the "st," prefix) appears nowhere >>>> in v3.12-rc1 >>> You're looking at an old patchset. We found out that the Snowball >>> schematic was actually incorrect and we have a LSM303DLHC. >>> >>> git grep -i lsm303dlhc_magn v3.12-rc1 >>> drivers/iio/magnetometer/st_magn.h: >>> #define LSM303DLHC_MAGN_DEV_NAME "lsm303dlhc_magn" >>> >>>> and the compatible string in the dt doesn't seem to get >>>> assigned to the device in the i2c core or in >>>> drivers/iio/magnetometer/st_magn_i2c.c. >>> It does: >>> drivers/i2c/busses/*.c: of_i2c_register_devices(); >>> drivers/of/of_i2c.c: |-> of_modalias_node(); >>> |-> i2c_new_device(); >> Ah, I see. >> >> I thought the return of of_modalias_node was only used to *guess* the >> module which might need to be loaded, rather than also being exposed to >> userspace as the device name. >> >> We've got a leak of Linux implementation details into the dt here, and I >> don't like it. Thanks to the combination of of_i2c_register device's use >> of of_modalias_node, and i2c_device_match using the id table after >> failing to use dt, any driver with a i2c_device_id name implicitly >> defines a dt binding for "${ARBITRARY_PREFIX},name". The same is true >> for SPI drivers using spi_device_id. This turns what looks like a Linux >> internal value into an ABI facing bootloader software. >> >> Judging by a quick and dirty grep [1], as of v3.12-rc1 there are ~930 >> suffixes which *may* be accepted with an arbitrary prefix as an I2C >> compatible string, of which ~750 do not appear in >> Documentation/devicetree. For SPI there are just over 400, of which just >> under 400 aren't documented. Both these counts include drivers that may >> never be built for DT platforms, as I've not filtered the results. >> >> Some drivers have an of_match_table (e.g. i2c-hid has a single >> "hid-over-i2c" entry), but if the I2C core fails to match in the >> of_match_table it'll still carry on and match the stripped compatible >> against the i2c_device_id table (where it'll happily match >> "${VENDOR},hid"). I think if a driver has an of_match_table and we fail >> to match with of_match_device we should fail rather than falling back to >> the id table. >> >> In the i2c-hid case, I can't see how the "hid-over-i2c" variant can ever >> work -- of_i2c_register_devices will skip over devices where the >> compatible string doesn't contain a ','. So our documented compatible >> string isn't usable, while an undocumented one is... > I do appologies for the i2c of_modalias_node mess. At the time is was a > quick hack to make it each to bind nodes to i2c devices without changing > the drivers. At the time there wasn't much support for device tree. Now > it is just painfull. My understanding from this discussion is that we don't really have a way to back out the existing naming (with _ instead of the preferred -) from these bindings without breaking existing ABI. Hence, shall I apply the patches that at least document that the ABI we are stuck with? (e.g. http://www.spinics.net/lists/devicetree/msg04808.html [PATCH 07/20] Documentation: dt: iio: Add binding for LPS001WP ) Jonathan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
