Re: [PATCH 1/2] dt-bindings: iio: imu: mpu6050: Document invensense,icm20608d

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

On Mon, 21 Mar 2022 09:04:11 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 20/03/2022 16:12, Jonathan Cameron wrote:
> > On Thu, 10 Mar 2022 22:24:03 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote:
> >   
> >> On 10/03/2022 19:56, Michael Srba wrote:  
> >>> Hi,
> >>> the thing is, the only reason the different compatible is needed at all
> >>> is that the chip presents a different WHOAMI, and the invensense,icm20608
> >>> compatible seems to imply the non-D WHOAMI value.    
> >>
> >> But this is a driver implementation issue, not related to bindings.
> >> Bindings describe the hardware.  
> > 
> > Indeed, but the key thing here is the WHOAMI register is hardware.
> >   
> >>  
> >>> I'm not sure how the driver would react to both compatibles being present,
> >>> and looking at the driver code, it seems that icm20608d is not the only
> >>> fully icm20608-compatible (to the extent of features supported by
> >>> the driver, and excluding the WHOAMI value) invensense IC, yet none
> >>> of these other ICs add the invensense,icm20608 compatible, so I guess I
> >>> don't see a good reason to do something different.    
> >>
> >> Probably my question should be asked earlier, when these other
> >> compatibles were added in such way.
> >>
> >> Skipping the DMP core, the new device is fully backwards compatible with
> >> icm20608.  
> > 
> > No. It is 'nearly' compatible...  The different WHOAMI value (used
> > to check the chip is the one we expect) makes it incompatible.  Now we
> > could change the driver to allow for that bit of incompatibility and
> > some other drivers do (often warning when the whoami is wrong but continuing
> > anyway).   
> 
> Different value of HW register within the same programming model does
> not make him incompatible. Quite contrary - it is compatible and to
> differentiate variants you do not need specific compatibles.

Whilst I don't personally agree with the definition of "compatible"
and think you are making false distinctions between hardware and software...

I'll accept Rob's statement of best practice.  However we can't just
add a compatible that won't work if someone uses it on a new board
that happens to run an old kernel.

Jonathan


> 
> Using arguments how driver behaves is wrong. Driver does not determine
> hardware/bindings.
> 
> >   
> >> Therefore extending the compatible makes sense. This is not
> >> only correct from devicetree point of view, but also is friendly towards
> >> out of tree users of bindings.
> >>
> >> The Linux driver behavior about whoami register does not matter here.
> >> Not mentioning that it would be easy for driver to accept multiple
> >> values of whoami.  
> > 
> > I disagree entirely. Any driver that makes use of the whoami will not
> > be compatible with this new part.  
> 
> Driver implementation is not related to bindings, does not matter. You
> cannot use driver implementation as argument in discussion about
> bindings and compatibility. Implementation differs, is limited, can be
> changed.
> 
> >   It's a driver design choice on whether
> > to make use of that, but it's a perfectly valid one to refuse to probe
> > if it doesn't detect that the device is the one it expects.  
> 
> Still not argument about bindings and compatibility but about driver. 
> 
> > + There is code out there today doing this so inherently it is not
> > compatible.  
> 
> Still code of driver, not bindings/DTS/hardware.
> 
> > 
> > So no, a fall back compatible is not suitable here because it simply
> > is not compatible.
> > 
> > Now, if intent was to provide a backwards compatible path from this
> > more advanced part then the behaviour of every register defined for
> > the simpler part, must be identical on the more advanced part.  
> 
> There is no backwards compatibility of advanced path, so the DMP core.
> The device (not driver, we do not talk here about driver) is compatible
> with basic version fully. 100%. Only this part you need to keep always
> compatible between each other,
> 
> > Extra functionality could only make use of fields in registers marked
> > reserved, or of new registers that didn't exist on the simpler device.  
> 
> Extra functionality is for new, extended compatible. See
> Documentation/devicetree/bindings/ABI.rst which exactly explains this case.
> 
> 
> 
> Best regards,
> Krzysztof