Re: All device tree bindings need maintainer ack or just the 'interesting' ones.

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

On Sat, Sep 21, 2013 at 03:57:42PM +0100, Jonathan Cameron wrote:
> On 09/18/13 16:11, Grant Likely wrote:
> > On Wed, 18 Sep 2013 14:58:04 +0100, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> 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 )

It does seem like that's the only thing we can do here. Yes please.

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