Hi, > -----Original Message----- > From: Hans de Goede <hdegoede@xxxxxxxxxx> > Sent: 10 December 2021 16:55 > To: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>; Mark Brown > <broonie@xxxxxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>; Len Brown > <lenb@xxxxxxxxxx>; Mark Gross <markgross@xxxxxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux- > acpi@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; > patches@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxx/ > > 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
