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

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

On Tue, 22 Mar 2022 11:23:18 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 21/03/2022 18:42, Jonathan Cameron wrote:
> > On Mon, 21 Mar 2022 16:22:38 +0100
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >   
> >> On 21/03/2022 16:04, Jonathan Cameron wrote:  
> >>> 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.
> >>>     
> >>
> >> The please explain me how this patch (the compatible set I proposed)
> >> fails to work in such case? How a new board with icm20608 (not
> >> icm20608d!) fails to work?  
> > 
> > I'm confused.  An actual icm20608 would work.
> > I guess you mean an icm20608d via compatible "invensense,icm20608"?  
> 
> In your example, new board with old kernel (so old kernel not supporting
> icm20608d), icm20608d will work exactly the same. Meaning: not work. Old
> kernel does not support it, new kernel will weirdly try to read WHOAMI
> and return -EINVAL (or whatever is there). Same effect.

'work' that means 'not work' was the root of my confusion.
With that in mind I now understand what you meant.

+ as suggested we should possibly 'fix' at least the kernels we can to relax
to a warning in this case.

Jonathan

> 
> >   
> >>
> >> To remind, the compatible has a format of:
> >> comaptible = "new", "old"
> >> e.g.: "invensense,icm20608d", "invensense,icm20608"  
> > 
> > Old kernel fails to match invensense,icm20608d, matches on invensense,icm20608.
> > Checks the WHOAMI value and reports a missmatched value and fails the probe
> > as it has no idea what the part was so no idea how to support it.  
> 
> And old kernel fails in your solution as well, because it does not know
> the compatible and refuses to bind.
> 
> > 
> > Obviously it wouldn't work anyway with an old kernel, but
> > without the fallback compatible at least there would be no error message
> > saying that the device is not the icm20608 we expected to see.  
> 
> You said before:
> "...that won't work if someone uses..."
> so still please explain how does this "will not work" happens. It does
> not work with old kernel in both cases...
> 
> Best regards,
> Krzysztof