[PATCH 0/2] iio: imu: inv_mpu6050: Add support for ICM-20608-D

[PATCH 0/2] iio: imu: inv_mpu6050: Add support for ICM-20608-D

[michael.srba]

From: Michael Srba <Michael.Srba@seznam.cz>

This series copies the invensense icm20608 support in the inv_mpu6050
driver for icm20608d, which is for all intents and purposes identical,
except for the inclusion of a DMP (Digital Motion Processor), which
is deemed significant enough to change the WHOAMI value, thereby making
the driver fail if the invensense,icm20608 compatible is specified.

Since the driver doesn't currently acknowledge that there is such thing
as a DMP core, all that is needed is to copy the icm20608 support and
change the WHOAMI value.

Michael Srba (2):
  dt-bindings: iio: imu: mpu6050: Document invensense,icm20608d
  iio: imu: inv_mpu6050: Add support for ICM-20608-D

 .../devicetree/bindings/iio/imu/invensense,mpu6050.yaml  | 1 +
 drivers/iio/imu/inv_mpu6050/Kconfig                      | 4 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c               | 9 +++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c                | 6 ++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h                | 2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c                | 5 +++++
 6 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.34.1

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

[michael.srba]

From: Michael Srba <Michael.Srba@seznam.cz>

ICM-20608-D differs from the other ICM-20608 variants by having
a DMP (Digital Motion Processor) core tacked on.
Despite having a different WHOAMI register, this variant is
completely interchangeable with the other ICM-20608 variants
by simply pretending the DMP core doesn't exist.

Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
---
 .../devicetree/bindings/iio/imu/invensense,mpu6050.yaml          | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
index d69595a524c1..6784cc140323 100644
--- a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
+++ b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
@@ -17,6 +17,7 @@ properties:
     enum:
       - invensense,iam20680
       - invensense,icm20608
+      - invensense,icm20608d
       - invensense,icm20609
       - invensense,icm20689
       - invensense,icm20602
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 10/03/2022 14:39, michael.srba@seznam.cz wrote:
> From: Michael Srba <Michael.Srba@seznam.cz>
> 
> ICM-20608-D differs from the other ICM-20608 variants by having
> a DMP (Digital Motion Processor) core tacked on.
> Despite having a different WHOAMI register, this variant is
> completely interchangeable with the other ICM-20608 variants
> by simply pretending the DMP core doesn't exist.

I wonder now why not using generic invensense,icm20608 compatible as
fallback? Why only having one specific compatible?

> 
> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
> ---
>  .../devicetree/bindings/iio/imu/invensense,mpu6050.yaml          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> index d69595a524c1..6784cc140323 100644
> --- a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> +++ b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> @@ -17,6 +17,7 @@ properties:
>      enum:
>        - invensense,iam20680
>        - invensense,icm20608
> +      - invensense,icm20608d
>        - invensense,icm20609
>        - invensense,icm20689
>        - invensense,icm20602


Best regards,
Krzysztof

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

[Michael Srba]

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

Regards,
Michael

On 10. 03. 22 17:34, Krzysztof Kozlowski wrote:
> On 10/03/2022 14:39, michael.srba@seznam.cz wrote:
>> From: Michael Srba <Michael.Srba@seznam.cz>
>>
>> ICM-20608-D differs from the other ICM-20608 variants by having
>> a DMP (Digital Motion Processor) core tacked on.
>> Despite having a different WHOAMI register, this variant is
>> completely interchangeable with the other ICM-20608 variants
>> by simply pretending the DMP core doesn't exist.
> I wonder now why not using generic invensense,icm20608 compatible as
> fallback? Why only having one specific compatible?
>
>> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
>> ---
>>   .../devicetree/bindings/iio/imu/invensense,mpu6050.yaml          | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
>> index d69595a524c1..6784cc140323 100644
>> --- a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
>> +++ b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
>> @@ -17,6 +17,7 @@ properties:
>>       enum:
>>         - invensense,iam20680
>>         - invensense,icm20608
>> +      - invensense,icm20608d
>>         - invensense,icm20609
>>         - invensense,icm20689
>>         - invensense,icm20602
>
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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.

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


Best regards,
Krzysztof

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

[Jonathan Cameron]

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

> 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.  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.
+ There is code out there today doing this so inherently it is not
compatible.

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.
Extra functionality could only make use of fields in registers marked
reserved, or of new registers that didn't exist on the simpler device.

There are other ways of handling backwards compatibility but they all
require statements in the simpler device spec about how you can tell
for future more complicated devices that they are compatible with this
spec. E.g. Feature registers, version registers etc.

Jonathan
> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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.

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

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

[Jonathan Cameron]

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

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

[Krzysztof Kozlowski]

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?

To remind, the compatible has a format of:
comaptible = "new", "old"
e.g.: "invensense,icm20608d", "invensense,icm20608"

Best regards,
Krzysztof

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

[Jonathan Cameron]

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"?

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

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.

Jonathan

> 
> Best regards,
> Krzysztof

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

[Michael Srba]

On 21. 03. 22 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"?
>
>> 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.
>
> 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.
I'm not sure if that's really an issue?
The old kernel is clearly not handling the compatible "correctly",
since the compatible says that the interface is a superset of
the icm20608 interface, and that using the icm20608
interface will work.
If the driver makes the incorrect assumption that
the WHOAMI being different means the interface cannot
be icm20608 compatible, then that seems like an issue
with the driver?
And I believe the single reason for why catering to
a broken driver would ever be considered is if not doing
so would result in breaking the devicetree ABI promise,
which doesn't seem to happen here.

btw, when this is resolved, I will be sending a v3 with
fixed dt-schema errors now that I managed to reproduce
those errors locally.

Regards,
Michael.
> Jonathan
>
>> Best regards,
>> Krzysztof

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

[Jonathan Cameron]

On Mon, 21 Mar 2022 19:07:26 +0100
Michael Srba <Michael.Srba@seznam.cz> wrote:

> On 21. 03. 22 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"?
> >  
> >> 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.
> >
> > 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.  
> I'm not sure if that's really an issue?
> The old kernel is clearly not handling the compatible "correctly",
> since the compatible says that the interface is a superset of
> the icm20608 interface, and that using the icm20608
> interface will work.
> If the driver makes the incorrect assumption that
> the WHOAMI being different means the interface cannot
> be icm20608 compatible, then that seems like an issue
> with the driver?
> And I believe the single reason for why catering to
> a broken driver would ever be considered is if not doing
> so would result in breaking the devicetree ABI promise,
> which doesn't seem to happen here.

I'll be honest I no longer care that much either way.

If someone would point me to clear documentation of that
DT ABI promise and how it describes things as being compatible
that would be great and provide me with a clear statement
to point others to in the future.
Perhaps I've just been missing that documentation or it
needs writing.

I think that having to ignore a WHOAMI value that
is unknown to the driver because there might be a future part
which is compatible is a very bad way to support
devices in a reliable fashion and going to lead to annoyed
users and bug reports. This is different to electing to
using a shared compatible when two parts are introduced at
the same time and we are doing detection in the driver of
which variant we have.

I mentioned earlier that we have this type of defensive coding
precisely because we have had false assessments about
compatibility in the past. This manufacturer does not in
general document compatibility across parts. I have no idea if
they do for this particular part as there doesn't seem to be
a public datasheet.

It didn't work before, now it won't work and will complain about it
which may lead to some bug reports that won't be resolved but
I'll adopt the majority opinion which seems to be that we
don't care about that.  I'd also be happy to see us reduce
the problem scope here by having a 'fix' for that rejection
of unknown IDs that we can push back to stable kernels.
Relaxing it to a warning should be sufficient, though we probably
want to screen out whatever comes back from the bus if there
is no device present at all as the WHOAMI check is also
providing that protection.

Jonathan



> 
> btw, when this is resolved, I will be sending a v3 with
> fixed dt-schema errors now that I managed to reproduce
> those errors locally.
> 
> Regards,
> Michael.
> > Jonathan
> >  
> >> Best regards,
> >> Krzysztof  
> 

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

[Krzysztof Kozlowski]

On 22/03/2022 11:19, Jonathan Cameron wrote:
>>> 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.  
>> I'm not sure if that's really an issue?
>> The old kernel is clearly not handling the compatible "correctly",
>> since the compatible says that the interface is a superset of
>> the icm20608 interface, and that using the icm20608
>> interface will work.
>> If the driver makes the incorrect assumption that
>> the WHOAMI being different means the interface cannot
>> be icm20608 compatible, then that seems like an issue
>> with the driver?
>> And I believe the single reason for why catering to
>> a broken driver would ever be considered is if not doing
>> so would result in breaking the devicetree ABI promise,
>> which doesn't seem to happen here.
> 
> I'll be honest I no longer care that much either way.
> 
> If someone would point me to clear documentation of that
> DT ABI promise 

Documentation/devicetree/bindings/ABI.rst

> and how it describes things as being compatible
> that would be great and provide me with a clear statement
> to point others to in the future.

It's very concise, so let me decipher the first paragraph. It is safe to
add new compatibles to the chain (so exactly like here "icm20608d,
icm20608") because the system can bind:
1. against new compatible bringing all new features,
2. old compatible, working with "old" or limited set of features.

What I was explaining you in mails before, which you responded to with:
"Whilst I don't personally agree with the definition of "compatible"
and think you are making false distinctions between hardware and
software..."
we do not talk here about software, as in device driver. We talk about
bindings which describe the hardware, therefore the compatible should be
rather understood in the hardware model, not driver model.

The compatible field does not mean that one driver is compatible with
this or other hardware. It means that devices have a compatible
programming model or interface.

Now driver should be implemented in that way. If driver handles
"icm20608" compatible, it should nicely handle only icm20608 features,
regardless whether device is icm20608 or icm20608d.

Now let's imagine, that icm20608d is slightly different than icm20608 in
the basic feature set. Than it's not compatible and should deserve
another separate binding entry, regardless how driver handles it.

Keep in mind what Rob said - driver implementation can changed, but
device compatibility in bindings should stay the same. Specially that
bindings are used in other operating systems (*BSD) and software pieces
(u-boot).

> Perhaps I've just been missing that documentation or it
> needs writing.
> 
> I think that having to ignore a WHOAMI value that
> is unknown to the driver because there might be a future part
> which is compatible is a very bad way to support
> devices in a reliable fashion and going to lead to annoyed
> users and bug reports.

I see your point. It's a safer choice than just accepting any device.
However it's a designer/programmers fault to provide a DTB with a
matching compatible for a non-compatible device. Not driver programmer
fault. Usually you do not have to protect the driver from it.

> This is different to electing to
> using a shared compatible when two parts are introduced at
> the same time and we are doing detection in the driver of
> which variant we have.
> 
> I mentioned earlier that we have this type of defensive coding
> precisely because we have had false assessments about
> compatibility in the past. This manufacturer does not in
> general document compatibility across parts. I have no idea if
> they do for this particular part as there doesn't seem to be
> a public datasheet.

Kind of continuing my previous thought also here - it's not a problem of
driver developer, but DTB developer. If the devices are not compatible
(thus driver will not work correctly), the person using that compatible
in DTB made mistake. Bug reports should be sent to that person, not to
driver developer, not to you.

> It didn't work before, now it won't work and will complain about it
> which may lead to some bug reports that won't be resolved but
> I'll adopt the majority opinion which seems to be that we
> don't care about that.

Yes, we don't care but the DTB/DTS person should. :)

>  I'd also be happy to see us reduce
> the problem scope here by having a 'fix' for that rejection
> of unknown IDs that we can push back to stable kernels.
> Relaxing it to a warning should be sufficient, though we probably
> want to screen out whatever comes back from the bus if there
> is no device present at all as the WHOAMI check is also
> providing that protection.

A dev_warn() with a disclaimer might be actually better approach. Unless
it might be a safety-critical device.

Best regards,
Krzysztof

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

[Jonathan Cameron]

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

> On 22/03/2022 11:19, Jonathan Cameron wrote:
> >>> 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.    
> >> I'm not sure if that's really an issue?
> >> The old kernel is clearly not handling the compatible "correctly",
> >> since the compatible says that the interface is a superset of
> >> the icm20608 interface, and that using the icm20608
> >> interface will work.
> >> If the driver makes the incorrect assumption that
> >> the WHOAMI being different means the interface cannot
> >> be icm20608 compatible, then that seems like an issue
> >> with the driver?
> >> And I believe the single reason for why catering to
> >> a broken driver would ever be considered is if not doing
> >> so would result in breaking the devicetree ABI promise,
> >> which doesn't seem to happen here.  
> > 
> > I'll be honest I no longer care that much either way.
> > 
> > If someone would point me to clear documentation of that
> > DT ABI promise   
> 
> Documentation/devicetree/bindings/ABI.rst

Hi Krzyztof,

I am not willfully ignoring your reference. I simply disagree
that it says what you understand it to.  This may be an aspect of
the intended meaning but it is sufficiently vague that I read
that reference several times and did not come to the same
conclusion as you have.

If you want to take your description and propose it as additional
documentation I would be happy to review it.

I'm not keen to continue this discussion because we are talking
cross purposes and going over the same ground with no fundamental
change of opinions, so it is not productive use of time.

Jonathan

> 
> > and how it describes things as being compatible
> > that would be great and provide me with a clear statement
> > to point others to in the future.  
> 
> It's very concise, so let me decipher the first paragraph.
> It is safe to
> add new compatibles to the chain (so exactly like here "icm20608d,
> icm20608") because the system can bind:
> 1. against new compatible bringing all new features,
> 2. old compatible, working with "old" or limited set of features.
> 
> What I was explaining you in mails before, which you responded to with:
> "Whilst I don't personally agree with the definition of "compatible"
> and think you are making false distinctions between hardware and
> software..."
> we do not talk here about software, as in device driver. We talk about
> bindings which describe the hardware, therefore the compatible should be
> rather understood in the hardware model, not driver model.
> 
> The compatible field does not mean that one driver is compatible with
> this or other hardware. It means that devices have a compatible
> programming model or interface.
> 
> Now driver should be implemented in that way. If driver handles
> "icm20608" compatible, it should nicely handle only icm20608 features,
> regardless whether device is icm20608 or icm20608d.
> 
> Now let's imagine, that icm20608d is slightly different than icm20608 in
> the basic feature set. Than it's not compatible and should deserve
> another separate binding entry, regardless how driver handles it.
> 
> Keep in mind what Rob said - driver implementation can changed, but
> device compatibility in bindings should stay the same. Specially that
> bindings are used in other operating systems (*BSD) and software pieces
> (u-boot).
> 
> > Perhaps I've just been missing that documentation or it
> > needs writing.
> > 
> > I think that having to ignore a WHOAMI value that
> > is unknown to the driver because there might be a future part
> > which is compatible is a very bad way to support
> > devices in a reliable fashion and going to lead to annoyed
> > users and bug reports.  
> 
> I see your point. It's a safer choice than just accepting any device.
> However it's a designer/programmers fault to provide a DTB with a
> matching compatible for a non-compatible device. Not driver programmer
> fault. Usually you do not have to protect the driver from it.
> 
> > This is different to electing to
> > using a shared compatible when two parts are introduced at
> > the same time and we are doing detection in the driver of
> > which variant we have.
> > 
> > I mentioned earlier that we have this type of defensive coding
> > precisely because we have had false assessments about
> > compatibility in the past. This manufacturer does not in
> > general document compatibility across parts. I have no idea if
> > they do for this particular part as there doesn't seem to be
> > a public datasheet.  
> 
> Kind of continuing my previous thought also here - it's not a problem of
> driver developer, but DTB developer. If the devices are not compatible
> (thus driver will not work correctly), the person using that compatible
> in DTB made mistake. Bug reports should be sent to that person, not to
> driver developer, not to you.
> 
> > It didn't work before, now it won't work and will complain about it
> > which may lead to some bug reports that won't be resolved but
> > I'll adopt the majority opinion which seems to be that we
> > don't care about that.  
> 
> Yes, we don't care but the DTB/DTS person should. :)
> 
> >  I'd also be happy to see us reduce
> > the problem scope here by having a 'fix' for that rejection
> > of unknown IDs that we can push back to stable kernels.
> > Relaxing it to a warning should be sufficient, though we probably
> > want to screen out whatever comes back from the bus if there
> > is no device present at all as the WHOAMI check is also
> > providing that protection.  
> 
> A dev_warn() with a disclaimer might be actually better approach. Unless
> it might be a safety-critical device.
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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.

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

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

[Jonathan Cameron]

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

[PATCH 2/2] iio: imu: inv_mpu6050: Add support for ICM-20608-D

[michael.srba]

From: Michael Srba <Michael.Srba@seznam.cz>

The difference between the ICM-20608-D and the other ICM-20608
variants is the addition of a DMP (Digital Motion Processor) core.
This difference is deemed substantial enough to change the WHOAMI
register value.
Since this driver doesn't currently acknowledge the exisence of
something like a DMP core, simply copy ICM-20608 except for the
aforementioned WHOAMI register.

Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
---
 drivers/iio/imu/inv_mpu6050/Kconfig        | 4 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 9 +++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 6 ++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  | 2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c  | 5 +++++
 5 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/Kconfig b/drivers/iio/imu/inv_mpu6050/Kconfig
index 9c625517173a..3636b1bc90f1 100644
--- a/drivers/iio/imu/inv_mpu6050/Kconfig
+++ b/drivers/iio/imu/inv_mpu6050/Kconfig
@@ -16,7 +16,7 @@ config INV_MPU6050_I2C
 	select REGMAP_I2C
 	help
 	  This driver supports the Invensense MPU6050/9150,
-	  MPU6500/6515/6880/9250/9255, ICM20608/20609/20689, ICM20602/ICM20690
+	  MPU6500/6515/6880/9250/9255, ICM20608(D)/20609/20689, ICM20602/ICM20690
 	  and IAM20680 motion tracking devices over I2C.
 	  This driver can be built as a module. The module will be called
 	  inv-mpu6050-i2c.
@@ -28,7 +28,7 @@ config INV_MPU6050_SPI
 	select REGMAP_SPI
 	help
 	  This driver supports the Invensense MPU6000,
-	  MPU6500/6515/6880/9250/9255, ICM20608/20609/20689, ICM20602/ICM20690
+	  MPU6500/6515/6880/9250/9255, ICM20608(D)/20609/20689, ICM20602/ICM20690
 	  and IAM20680 motion tracking devices over SPI.
 	  This driver can be built as a module. The module will be called
 	  inv-mpu6050-spi.
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 597768c29a72..86fbbe904050 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -217,6 +217,15 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 		.startup_time = {INV_MPU6500_GYRO_STARTUP_TIME, INV_MPU6500_ACCEL_STARTUP_TIME},
 	},
+	{
+		.whoami = INV_ICM20608D_WHOAMI_VALUE,
+		.name = "ICM20608D",
+		.reg = &reg_set_6500,
+		.config = &chip_config_6500,
+		.fifo_size = 512,
+		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
+		.startup_time = {INV_MPU6500_GYRO_STARTUP_TIME, INV_MPU6500_ACCEL_STARTUP_TIME},
+	},
 	{
 		.whoami = INV_ICM20609_WHOAMI_VALUE,
 		.name = "ICM20609",
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index fe03707ec2d3..ed52b27409ac 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -29,6 +29,7 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev)
 
 	switch (st->chip_type) {
 	case INV_ICM20608:
+	case INV_ICM20608D:
 	case INV_ICM20609:
 	case INV_ICM20689:
 	case INV_ICM20602:
@@ -182,6 +183,7 @@ static const struct i2c_device_id inv_mpu_id[] = {
 	{"mpu9250", INV_MPU9250},
 	{"mpu9255", INV_MPU9255},
 	{"icm20608", INV_ICM20608},
+	{"icm20608d", INV_ICM20608D},
 	{"icm20609", INV_ICM20609},
 	{"icm20689", INV_ICM20689},
 	{"icm20602", INV_ICM20602},
@@ -225,6 +227,10 @@ static const struct of_device_id inv_of_match[] = {
 		.compatible = "invensense,icm20608",
 		.data = (void *)INV_ICM20608
 	},
+	{
+		.compatible = "invensense,icm20608d",
+		.data = (void *)INV_ICM20608D
+	},
 	{
 		.compatible = "invensense,icm20609",
 		.data = (void *)INV_ICM20609
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index c6aa36ee966a..8e14f20b1314 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -76,6 +76,7 @@ enum inv_devices {
 	INV_MPU9250,
 	INV_MPU9255,
 	INV_ICM20608,
+	INV_ICM20608D,
 	INV_ICM20609,
 	INV_ICM20689,
 	INV_ICM20602,
@@ -394,6 +395,7 @@ struct inv_mpu6050_state {
 #define INV_MPU9255_WHOAMI_VALUE		0x73
 #define INV_MPU6515_WHOAMI_VALUE		0x74
 #define INV_ICM20608_WHOAMI_VALUE		0xAF
+#define INV_ICM20608D_WHOAMI_VALUE		0xAE
 #define INV_ICM20609_WHOAMI_VALUE		0xA6
 #define INV_ICM20689_WHOAMI_VALUE		0x98
 #define INV_ICM20602_WHOAMI_VALUE		0x12
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
index 6800356b25fb..ce8ab6db2bf2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
@@ -74,6 +74,7 @@ static const struct spi_device_id inv_mpu_id[] = {
 	{"mpu9250", INV_MPU9250},
 	{"mpu9255", INV_MPU9255},
 	{"icm20608", INV_ICM20608},
+	{"icm20608d", INV_ICM20608D},
 	{"icm20609", INV_ICM20609},
 	{"icm20689", INV_ICM20689},
 	{"icm20602", INV_ICM20602},
@@ -113,6 +114,10 @@ static const struct of_device_id inv_of_match[] = {
 		.compatible = "invensense,icm20608",
 		.data = (void *)INV_ICM20608
 	},
+	{
+		.compatible = "invensense,icm20608d",
+		.data = (void *)INV_ICM20608D
+	},
 	{
 		.compatible = "invensense,icm20609",
 		.data = (void *)INV_ICM20609
-- 
2.34.1

Re: [PATCH 2/2] iio: imu: inv_mpu6050: Add support for ICM-20608-D

[Jean-Baptiste Maneyrol]

Hello,

you're all right, thanks for the patch.

Acked-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>


From: michael.srba@seznam.cz <michael.srba@seznam.cz>
Sent: Thursday, March 10, 2022 14:39
To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring <robh+dt@kernel.org>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; Michael Srba <Michael.Srba@seznam.cz>
Subject: [PATCH 2/2] iio: imu: inv_mpu6050: Add support for ICM-20608-D 
 
 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.

From: Michael Srba <Michael.Srba@seznam.cz>

The difference between the ICM-20608-D and the other ICM-20608
variants is the addition of a DMP (Digital Motion Processor) core.
This difference is deemed substantial enough to change the WHOAMI
register value.
Since this driver doesn't currently acknowledge the exisence of
something like a DMP core, simply copy ICM-20608 except for the
aforementioned WHOAMI register.

Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
---
 drivers/iio/imu/inv_mpu6050/Kconfig        | 4 ++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 9 +++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 6 ++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  | 2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c  | 5 +++++
 5 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/Kconfig b/drivers/iio/imu/inv_mpu6050/Kconfig
index 9c625517173a..3636b1bc90f1 100644
--- a/drivers/iio/imu/inv_mpu6050/Kconfig
+++ b/drivers/iio/imu/inv_mpu6050/Kconfig
@@ -16,7 +16,7 @@ config INV_MPU6050_I2C
         select REGMAP_I2C
         help
           This driver supports the Invensense MPU6050/9150,
-         MPU6500/6515/6880/9250/9255, ICM20608/20609/20689, ICM20602/ICM20690
+         MPU6500/6515/6880/9250/9255, ICM20608(D)/20609/20689, ICM20602/ICM20690
           and IAM20680 motion tracking devices over I2C.
           This driver can be built as a module. The module will be called
           inv-mpu6050-i2c.
@@ -28,7 +28,7 @@ config INV_MPU6050_SPI
         select REGMAP_SPI
         help
           This driver supports the Invensense MPU6000,
-         MPU6500/6515/6880/9250/9255, ICM20608/20609/20689, ICM20602/ICM20690
+         MPU6500/6515/6880/9250/9255, ICM20608(D)/20609/20689, ICM20602/ICM20690
           and IAM20680 motion tracking devices over SPI.
           This driver can be built as a module. The module will be called
           inv-mpu6050-spi.
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 597768c29a72..86fbbe904050 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -217,6 +217,15 @@ static const struct inv_mpu6050_hw hw_info[] = {
                 .temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
                 .startup_time = {INV_MPU6500_GYRO_STARTUP_TIME, INV_MPU6500_ACCEL_STARTUP_TIME},
         },
+       {
+               .whoami = INV_ICM20608D_WHOAMI_VALUE,
+               .name = "ICM20608D",
+               .reg = &reg_set_6500,
+               .config = &chip_config_6500,
+               .fifo_size = 512,
+               .temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
+               .startup_time = {INV_MPU6500_GYRO_STARTUP_TIME, INV_MPU6500_ACCEL_STARTUP_TIME},
+       },
         {
                 .whoami = INV_ICM20609_WHOAMI_VALUE,
                 .name = "ICM20609",
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index fe03707ec2d3..ed52b27409ac 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -29,6 +29,7 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev)
 
         switch (st->chip_type) {
         case INV_ICM20608:
+       case INV_ICM20608D:
         case INV_ICM20609:
         case INV_ICM20689:
         case INV_ICM20602:
@@ -182,6 +183,7 @@ static const struct i2c_device_id inv_mpu_id[] = {
         {"mpu9250", INV_MPU9250},
         {"mpu9255", INV_MPU9255},
         {"icm20608", INV_ICM20608},
+       {"icm20608d", INV_ICM20608D},
         {"icm20609", INV_ICM20609},
         {"icm20689", INV_ICM20689},
         {"icm20602", INV_ICM20602},
@@ -225,6 +227,10 @@ static const struct of_device_id inv_of_match[] = {
                 .compatible = "invensense,icm20608",
                 .data = (void *)INV_ICM20608
         },
+       {
+               .compatible = "invensense,icm20608d",
+               .data = (void *)INV_ICM20608D
+       },
         {
                 .compatible = "invensense,icm20609",
                 .data = (void *)INV_ICM20609
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index c6aa36ee966a..8e14f20b1314 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -76,6 +76,7 @@ enum inv_devices {
         INV_MPU9250,
         INV_MPU9255,
         INV_ICM20608,
+       INV_ICM20608D,
         INV_ICM20609,
         INV_ICM20689,
         INV_ICM20602,
@@ -394,6 +395,7 @@ struct inv_mpu6050_state {
 #define INV_MPU9255_WHOAMI_VALUE                0x73
 #define INV_MPU6515_WHOAMI_VALUE                0x74
 #define INV_ICM20608_WHOAMI_VALUE               0xAF
+#define INV_ICM20608D_WHOAMI_VALUE             0xAE
 #define INV_ICM20609_WHOAMI_VALUE               0xA6
 #define INV_ICM20689_WHOAMI_VALUE               0x98
 #define INV_ICM20602_WHOAMI_VALUE               0x12
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
index 6800356b25fb..ce8ab6db2bf2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
@@ -74,6 +74,7 @@ static const struct spi_device_id inv_mpu_id[] = {
         {"mpu9250", INV_MPU9250},
         {"mpu9255", INV_MPU9255},
         {"icm20608", INV_ICM20608},
+       {"icm20608d", INV_ICM20608D},
         {"icm20609", INV_ICM20609},
         {"icm20689", INV_ICM20689},
         {"icm20602", INV_ICM20602},
@@ -113,6 +114,10 @@ static const struct of_device_id inv_of_match[] = {
                 .compatible = "invensense,icm20608",
                 .data = (void *)INV_ICM20608
         },
+       {
+               .compatible = "invensense,icm20608d",
+               .data = (void *)INV_ICM20608D
+       },
         {
                 .compatible = "invensense,icm20609",
                 .data = (void *)INV_ICM20609
-- 
2.34.1