Re: [PATCH v3 4/6] iio: imu: bmi270: Add support for BMI260

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Oct 2024 19:54:03 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Oct 22, 2024 at 08:50:43AM -0700, Justin Weiss wrote:
> > Justin Weiss <justin@justinweiss.com> writes:  
> 
> ...
> 
> > The ACPI IDs with device pointers are here:
> >   
> > > +static const struct acpi_device_id bmi270_acpi_match[] = {
> > > +	/* OrangePi NEO */
> > > +	{ "BMI0260",  (kernel_ulong_t)&bmi260_chip_info },
> > > +	/* GPD Win Mini, Aya Neo AIR Pro, OXP Mini Pro, etc. */
> > > +	{ "BMI0160",  (kernel_ulong_t)&bmi260_chip_info },
> > > +	/* GPD Win Max 2 */
> > > +	{ "10EC5280", (kernel_ulong_t)&bmi260_chip_info },
> > > +	{ }  
> 
> Cool! But please, keep them alphabetically ordered by ID.
> 
> Can we push OrangePI NED to go and fix ACPI IDs eventually?
> 
> > > +};  
> > 
> > I pulled DSDT device excerpts for the GPD Win Mini (which uses the
> > BMI0160 ACPI ID, even though it has a bmi260) and the OrangePi NEO
> > (which uses the BMI0260 ACPI ID).
> > 
> > I couldn't find a shipping device with a bmi260 using the 10EC5280 ACPI
> > ID. Some prototype devices with the bmi260 may have used them:
> > https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/
> > 
> > I can remove that ID from this changeset for now.  
> 
> Yes, please do not add anything that has no evidence of existence in the wild
> or approved vendor allocated ID.
> 
> > GPD Win Mini:  
> 
> Add short parts of these to the commit message, or better split these to two
> patches each of them adding a new ID to the table.
> 
> See below what I do want to see there (no need to have everything),
> i.e. I removed unneeded lines:
> 
> > Device (BMI2)
> > {
> >     Name (_ADR, Zero)  // _ADR: Address  
> 
> My gosh, can this be fixed (seems rhetorical)? The _ADR must NOT be present
> together with _HID.  It's against the ACPI specifications.
> 
> >     Name (_HID, "BMI0160")  // _HID: Hardware ID
> >     Name (_CID, "BMI0160")  // _CID: Compatible ID
> >     Name (_DDN, "Accelerometer")  // _DDN: DOS Device Name
> >     Name (_UID, One)  // _UID: Unique ID
> >     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >     {
> >         Name (RBUF, ResourceTemplate ()
> >         {
> >             I2cSerialBusV2 (0x0068, ControllerInitiated, 0x00061A80,
> >                 AddressingMode7Bit, "\\_SB.I2CB",
> >                 0x00, ResourceConsumer, , Exclusive,
> >                 )
> >             GpioInt (Edge, ActiveLow, Exclusive, PullDefault, 0x0000,
> >                 "\\_SB.GPIO", 0x00, ResourceConsumer, ,
> >                 )
> >                 {   // Pin list
> >                     0x008B
> >                 }
> >         })
> >         Return (RBUF) /* \_SB_.I2CB.BMI2._CRS.RBUF */
> >     }  
>       ...
> > }
> >   
> 
> > OrangePi NEO:  
> 
> Same comments for this device.
> 
> ...
> 
> > > +static const struct acpi_device_id bmi270_acpi_match[] = {
> > > +	{ "BOSC0260",  (kernel_ulong_t)&bmi260_chip_info },
> > > +	{ }
> > > +};  
> > 
> > I can't find any evidence of BOSC0260 being used, besides existing in
> > the Windows driver. As suggested in an earlier review, I added it here
> > to encourage people looking at this driver in the future to use the
> > correct ACPI ID.  
> 
> Are you official representative of Bosch or do you have a proof by the vendor
> that they allocated this ID? Otherwise we may NOT allocate IDs on their behalf
> and has not to be added.
Fair point. The provenance of the driver is a little disconnected from Bosch:
https://ayaneo-1305909189.cos.accelerate.myqcloud.com/ayaneo_com/download/2023/UMDF2.0_BMI260_V1.0.23_5ID_signed_20H1.zip

Justin, if you have contacts at ayaneo, maybe they can confirm if the IDs come
from Bosch. Or maybe we can find someone at Bosch?

Jonathan


>