RE: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver

[Stefan Binding <sbinding@opensource.cirrus.com>]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 10 December 2021 16:55
> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> patches@opensource.cirrus.com
> Subject: Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
> 
> Hi Stefan,
> 
> On 12/10/21 16:40, Stefan Binding wrote:
> > Add support for SPI bus in the ic2-multi-instantiate driver as
> > upcoming laptops will need to multi instantiate SPI devices from a
> > single device node, which has multiple SpiSerialBus entries at the
> > ACPI table.
> >
> > With the new SPI support, i2c-multi-instantiate becomes
> > bus-multi-instantiate and is moved to the ACPI folder.
> >
> > The intention is to support the SPI bus by re-using the current I2C
> > multi instantiate, instead of creating a new SPI multi instantiate, to
> > make it possible for peripherals that can be controlled by I2C or SPI
> > to have the same HID at the ACPI table.
> >
> > The new driver (Bus multi instantiate, bmi) checks for the hard-coded
> > bus type and returns -ENODEV in case of zero devices found for that
> > bus. In the case of automatic bus detection, the driver will give
> > preference to I2C.
> >
> > The expectation is for a device node in the ACPI table to have
> > multiple I2cSerialBus only or multiple SpiSerialBus only, not a mix of
> > both; and for the case where there are both entries in one device
> > node, only the I2C ones would be probed.
> >
> > This new bus multi instantiate will be used in CS35L41 HDA new driver,
> > being upstreamed:
> > https://lkml.org/lkml/2021/11/23/723
> 
> Unfortunately you never really answered my questions about v1 of this
> series:
> 
> https://lore.kernel.org/platform-driver-x86/a1f546c2-5c63-573a-c032-
> 603c792f3f7c@redhat.com/
> 
> So looking at the linked CS35L41 HDA series there is a single ACPI device node
> with a HID of CLSA0100 which describes two CS35L41 amplifiers connected
> over I2C ?

Yes, the related series uses HID CLSA0100, which contains 2 I2C devices inside a
single node. This ID was mistakenly used for this laptop, and instead CSC3551 
has been used for subsequent laptops.

> 
> I assume you are doing this work because there are also designs where there
> is a similar CLSA0100 ACPI device which also describes two CS35L41 amplifiers
> but then connected over SPI ?

Yes, there are several laptop designs which use an equivalent ACPI which describes
2 or 4 CS35L41 amplifiers which are connected either via I2C or via SPI.
Both designs use the same ACPI design and have 2-4 devices (either I2C or SPI)
defined inside a single ACPI node for HID CSC3551.
Note that the devices inside the node can be either SPI or I2C, but never SPI
and I2C.

> 
> It would really help if you can:
> 
> 1. Answer my questions from v1

I hope my colleague Lucas has answered these questions now.

> 2. Provide a concrete example of a device where these changes will be
> necessary to make things work, preferably with a link to an actual ACPI DSDT
> of that device.

This is the CSC3551 node for a laptop with 4 SPI nodes, with a shared IRQ:

 Scope (_SB.PC00.SPI0)
    {
        Device (GSPK)
        {
            Name (_HID, "CSC3551")  // _HID: Hardware ID
            Method (AUID, 0, NotSerialized)
            {
                Return ("103C89C3")
            }

            Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
            {
                Return (AUID ())
            }

            Name (_UID, One)  // _UID: Unique ID
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (RBUF, ResourceTemplate ()
                {
                    SpiSerialBusV2 (0x0000, PolarityLow, FourWireMode, 0x08,
                        ControllerInitiated, 0x003D0900, ClockPolarityLow,
                        ClockPhaseFirst, "\\_SB.PC00.SPI0",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                    SpiSerialBusV2 (0x0001, PolarityLow, FourWireMode, 0x08,
                        ControllerInitiated, 0x003D0900, ClockPolarityLow,
                        ClockPhaseFirst, "\\_SB.PC00.SPI0",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                    SpiSerialBusV2 (0x0002, PolarityLow, FourWireMode, 0x08,
                        ControllerInitiated, 0x003D0900, ClockPolarityLow,
                        ClockPhaseFirst, "\\_SB.PC00.SPI0",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                    SpiSerialBusV2 (0x0003, PolarityLow, FourWireMode, 0x08,
                        ControllerInitiated, 0x003D0900, ClockPolarityLow,
                        ClockPhaseFirst, "\\_SB.PC00.SPI0",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                    GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                    GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                    GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                    GpioIo (Exclusive, PullDown, 0x0000, 0x0000, IoRestrictionOutputOnly,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                    GpioIo (Shared, PullUp, 0x0064, 0x0000, IoRestrictionInputOnly,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                    GpioInt (Edge, ActiveBoth, Shared, PullUp, 0x0064,
                        "\\_SB.GPI0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0000
                        }
                })
                CreateWordField (RBUF, 0xCA, ACS1)
                CreateWordField (RBUF, 0xA7, ACS2)
                CreateWordField (RBUF, 0xED, ACS3)
                CreateWordField (RBUF, 0x0110, ARST)
                CreateWordField (RBUF, 0x0133, AINT)
                CreateWordField (RBUF, 0x0156, AIN2)
                ACS1 = GNUM (0x090E0016)
                ACS2 = GNUM (0x090E0017)
                ACS3 = GNUM (0x090C0006)
                ARST = GNUM (0x09070017)
                AINT = GNUM (0x09070013)
                AIN2 = GNUM (0x09070013)
                Return (RBUF) /* \_SB_.PC00.SPI0.GSPK._CRS.RBUF */
            }

            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0F)
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
            }
            Name (_DSD, Package ()  // _DSD: Device-Specific Data
            {
                ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
                Package ()
                {
                    Package () { "cirrus,dev-index", Package () { 0, 1, 2, 3 } },
                    Package ()
                    {
                        "reset-gpios", Package ()
                        {
                            ^GSPK, 3, 0, 0,
                            ^GSPK, 3, 0, 0,
                            ^GSPK, 3, 0, 0,
                            ^GSPK, 3, 0, 0,
                        },
                    },
                    Package () { "cirrus,speaker-position",     Package () { 1, 0, 1, 0 } },
                    Package () { "cirrus,gpio1-func",           Package () { 3, 3, 3, 3 } },
                    Package () { "cirrus,gpio2-func",           Package () { 2, 2, 2, 2 } },
                    Package () { "cirrus,boost-ind-nanohenry",  Package () { 1000, 1000, 1000, 1000 } },
                    Package () { "cirrus,boost-peak-milliamp",  Package () { 4500, 4500, 4500, 4500 } },
                    Package () { "cirrus,boost-cap-microfarad", Package () { 24, 24, 24, 24 } },
                }
            })
        }
    }

This is just our node from the DSDT, we are working to obtain and share the full DSDT, if still required.

> 
> Until you can better clarify why this is necessary, this series gets a nack from
> me. The i2c-mult-instantiate code is a hack to deal with some rather sub-
> optimal choices made in DSDTs used on devices shipped with Windows and
> unless absolutely necessary I would rather not see this get expanded to SPI.
> 
> Regards,
> 
> Hans

Thanks,

Stefan