From: Hans de Goede <hdegoede@redhat.com>
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
Date: Wed, 15 Dec 2021 11:22:46 +0100 [thread overview]
Message-ID: <fd960dc4-2702-6031-f628-6140289f0f4b@redhat.com> (raw)
In-Reply-To: <002401d7f016$3bb4f260$b31ed720$@opensource.cirrus.com>
Hi,
On 12/13/21 12:40, Stefan Binding wrote:
> 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.
Yes, thank you .
>> 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.
Thanks, there is no need to share the full DSDT, this is exactly what I was looking for.
With the provided info I believe that this indeed is the right approach
and I no longer have any objections against this approach.
I will try to make some time to review this series soon-ish.
Regards,
Hans
next prev parent reply other threads:[~2021-12-15 10:22 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
2021-12-10 15:40 ` [PATCH v2 1/6] spi: Export acpi_spi_find_controller_by_adev to be used externally Stefan Binding
2021-12-10 15:40 ` [PATCH v2 2/6] spi: Make spi_alloc_device and spi_add_device public again Stefan Binding
2021-12-10 15:40 ` [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder Stefan Binding
2021-12-10 17:59 ` Mark Brown
2021-12-15 10:26 ` Hans de Goede
2021-12-10 15:40 ` [PATCH v2 4/6] ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name Stefan Binding
2021-12-10 15:40 ` [PATCH v2 5/6] ACPI: bus-multi-instantiate: Reorganize I2C functions Stefan Binding
2021-12-10 15:40 ` [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support Stefan Binding
2021-12-21 18:32 ` Hans de Goede
2021-12-21 19:22 ` Hans de Goede
2022-01-10 14:36 ` Stefan Binding
2022-01-11 9:20 ` Hans de Goede
2021-12-10 16:54 ` [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Hans de Goede
2021-12-13 11:40 ` Stefan Binding
2021-12-15 10:22 ` Hans de Goede [this message]
2021-12-21 18:16 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fd960dc4-2702-6031-f628-6140289f0f4b@redhat.com \
--to=hdegoede@redhat.com \
--cc=broonie@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=sbinding@opensource.cirrus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).