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

[Andy Shevchenko]

On Tue, Oct 22, 2024 at 08:50:43AM -0700, Justin Weiss wrote:
> Justin Weiss <justin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
> 
> 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.

-- 
With Best Regards,
Andy Shevchenko