Re: [PATCH 1/3] spi: Revert "spi: Remove unused function spi_busnum_to_master()"

From: Hans de Goede <hdegoede@redhat.com>
To: Lucas tanure <tanureal@opensource.cirrus.com>,
	Andy Shevchenko <andriy.shevchenko@intel.com>,
	Stefan Binding <sbinding@opensource.cirrus.com>
Cc: Mark Brown <broonie@kernel.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Mark Gross <markgross@kernel.org>,
	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 1/3] spi: Revert "spi: Remove unused function spi_busnum_to_master()"
Date: Fri, 10 Dec 2021 19:22:46 +0100	[thread overview]
Message-ID: <628226e2-6e1c-a05d-fc00-82c7496da38d@redhat.com> (raw)
In-Reply-To: <85e9e11d-a4fc-44a9-55c2-3a4d2de7769d@opensource.cirrus.com>

Hi,

On 12/10/21 19:10, Lucas tanure wrote:
> On 12/3/21 11:14, Hans de Goede wrote:
>> Hi,
>>
>> On 12/3/21 12:06, Andy Shevchenko wrote:
>>> On Thu, Dec 02, 2021 at 04:24:19PM +0000, Stefan Binding wrote:
>>>> From: Lucas Tanure <tanureal@opensource.cirrus.com>
>>>>
>>>> Revert commit bdc7ca008e1f ("spi: Remove unused function
>>>> spi_busnum_to_master()")
>>>> This function is needed for the spi version of i2c multi
>>>> instantiate driver.
>>>
>>> I understand the intention, but I have no clue from this series (it lacks of
>>> proper cover letter, it lacks of much better and justified commit message in
>>> the patch 3) what is the actual issue. Without these to be provided it's no go
>>> for the series. Please, provide much better description what is the actual
>>> issue you are trying to solve (from patch 3 my guts telling me that this can
>>> be achieved differently without this code being involved).
>>
>> Yes I assume that eventually there will be a follow-up which will
>> actually add some new ACPI HIDs to the new bus-multi-instantiate.c file ?
>>
> Yes, we are developing an HDA sound driver for the HID CSC3551,
> which is used for laptops that use SPI or I2C.
> And in that series is where we plan to put a patch to add that HID.
> 
>> Can we please have (some of) those patches as part of the next
>> version, so that we can see how you will actually use this?
> The series is this one https://lkml.org/lkml/2021/11/23/723, but
> the SPI HID was not ready to be sent in that version, but will be
> part of the next submission.
> 
>>
>> Also I'm wondering is this actually about ACPI device's having multiple
>> SpiSerialBusV2 resources in a single _CRS resource list ?
> yes, a single _CRS with 2 or 4 SpiSerialBusV2 inside.
> 
>>
>> Or do you plan to use this for devices with only a single
>> I2cSerialBusV2 or SpiSerialBusV2 resource to e.g. share IRQ lookup
>> code between the 2 cases ?
> No, the minimum number SpiSerialBusV2 or I2cSerialBusV2 inside the
> _CRS is two.
> 
>>
>> If you plan to use this for devices with only a single
>> I2cSerialBusV2 or SpiSerialBusV2 resource, then I'm going to have to
>> nack this.
>>
>> Each ACPI HID which needs to be handled in this code also needs an
>> entry in the i2c_multi_instantiate_ids[] list in drivers/acpi/scan.c
>> which is code which ends up loaded on every single ACPI system, so
>> we really only want to add HIDs there for the special case of having
>> multiple I2cSerialBusV2 or SpiSerialBusV2 resources in a single
>> ACPI Device / ACPI fwnode.
>>
>> If you are looking to use this as a way to share code for other reasons
>> (which is a good goal to strive for!) please find another way, such
>> as e.g. adding some helper functions to drivers/gpio/gpiolib-acpi.c
>> (note there already are a couple of helpers there which you may use).
> No, we only want to multi instantiate SPI or I2C devices from a single _CRS.

Ok, that is fine, thank you for clarifying things.

Regards,

Hans