Re: [PATCH 1/3] dt-bindings: iio: imu: mpu6050: Add level shifter

[Jean-Baptiste Maneyrol]

Hello,

these are very old unsupported chips, thus this is not something that can be easily found. Even after doing some archaeology.

But when looking at register maps, it should only be used with MPU-9150 and not MPU-9250. I would feel much more comfortable to restrict this fix to MPU-9150 only (by testing chip_type against INV_MPU9150).

Thanks,
JB


From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Sent: Monday, September 25, 2023 15:21
To: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
Cc: Andreas Kemnade <andreas@xxxxxxxxxxxx>; jic23@xxxxxxxxxx <jic23@xxxxxxxxxx>; lars@xxxxxxxxxx <lars@xxxxxxxxxx>; robh+dt@xxxxxxxxxx <robh+dt@xxxxxxxxxx>; krzysztof.kozlowski+dt@xxxxxxxxxx <krzysztof.kozlowski+dt@xxxxxxxxxx>; conor+dt@xxxxxxxxxx <conor+dt@xxxxxxxxxx>; bcousson@xxxxxxxxxxxx <bcousson@xxxxxxxxxxxx>; tony@xxxxxxxxxxx <tony@xxxxxxxxxxx>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>; chenhuiz@xxxxxxxx <chenhuiz@xxxxxxxx>; andy.shevchenko@xxxxxxxxx <andy.shevchenko@xxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx <devicetree@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; linux-omap@xxxxxxxxxxxxxxx <linux-omap@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH 1/3] dt-bindings: iio: imu: mpu6050: Add level shifter 
 
On Mon, 25 Sep 2023 14: 24: 32 +0200 Krzysztof Kozlowski <krzysztof. kozlowski@ linaro. org> wrote: > On 25/09/2023 13: 02, Andreas Kemnade wrote: > > On Mon, 25 Sep 2023 11: 28: 52 +0100 > > Jonathan Cameron <Jonathan. Cameron@ Huawei. com> 
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender 
This message came from outside your organization. 
 
ZjQcmQRYFpfptBannerEnd
On Mon, 25 Sep 2023 14:24:32 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> On 25/09/2023 13:02, Andreas Kemnade wrote:
> > On Mon, 25 Sep 2023 11:28:52 +0100
> > Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> >   
> >> On Mon, 25 Sep 2023 08:54:08 +0200
> >> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>  
> >>> On 25/09/2023 00:25, Andreas Kemnade wrote:    
> >>>> Found in ancient platform data struct:
> >>>> level_shifter: 0: VLogic, 1: VDD
> >>>>
> >>>> Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> >>>> ---
> >>>>  .../devicetree/bindings/iio/imu/invensense,mpu6050.yaml         | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> >>>> index 1db6952ddca5e..6aae2272fa15c 100644
> >>>> --- a/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> >>>> +++ b/Documentation/devicetree/bindings/iio/imu/invensense,mpu6050.yaml
> >>>> @@ -48,6 +48,8 @@ properties:
> >>>>  
> >>>>    mount-matrix: true
> >>>>  
> >>>> +  invensense,level-shifter: true      
> >>>
> >>> It does not look like you tested the bindings, at least after quick
> >>> look. Please run `make dt_binding_check` (see
> >>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> >>> Maybe you need to update your dtschema and yamllint.
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>
> >>>     
> >>
> >> Also this one isn't obvious - give it a description in the binding doc.
> >>
> >> I'm not sure of the arguement for calling it level shift in general.
> >>  
> > I have no more descrption than the old source (see the citation from there)
> > https://invensense.tdk.com/wp-content/uploads/2015/02/MPU-9150-Register-Map.pdf  
> 
> I could not find any reference to level shift in this manual. To which
> page and part do you refer?
> 
> > 
> > does not list it. But that bit is needed to get things to work what also does the
> > vendor kernel do.
> > 
> > What could be a better descrption?  
> 
> I don't know, but something reasonable to you should be put there.

The text you have in the commit log seems better than nothing.
I suspect it's internally wiring VDD to VDDIO. Normally people just
connect both power supplies to same supply if they want to do that,
but maybe there was a chip variant that didn't have enough pins?

If you have the device, can you see it actually matches the packaging
types in the manual?

Jonathan

> 
> Best regards,
> Krzysztof
> 
>