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... We don't appear to have any clashes yet (with "vendor-a,device" and "vendor-b,device"), so maybe that's not an issue -- we can add of_match_table entries as required to disambiguate, though we have to be careful to capture all the prefixes already in use. The current use of of_modalias_node also only allows for the first entry in the compatible list to be used to request a compatible module. I think it would be better for the DT core to request modules somehow when iterating over compatible lists, but I'm not familiar with MODULE_DEVICE_TABLE() and friends, so maybe that's not as easy as it sounds. That doesn't solve exporting a sensible modalias to userspace, but it would somewhat decouple the compatible list and modaliases, and allow for fallback entries in the compatible list to load modules if required. I see that some macintosh drivers have i2c_device_id values in device tree style (e.g. "MAC,fcu"), without an of_match_table. Assuming that "MAC,fcu" is the full string rather than "AAPL,MAC,fcu", I don't see how those get matched to drivers. The I2C core will drop the leading "MAC,", leaving only "fcu" in the i2c_board_info, and then that won't match any driver modaliases. I'm probably missing something here. Thanks, Mark. [1] $ git grep -hW i2c_device_id.\*= -- drivers | grep {.\*} | grep -o "\"[^\"]\+\"" | grep -o "[^\"]\+" | sort -u | xargs -I '{}' sh -c 'git grep -q "{}" -- Documentation/devicetree || echo Undocumented binding \"*,{}\"' -- 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
