| Message ID | 20211202162421.7628-1-sbinding@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] spi: Revert "spi: Remove unused function spi_busnum_to_master()" | expand |
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. If we're going to restore this API we should rename it to _controller() while we're at it.
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).
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 ? 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? Also I'm wondering is this actually about ACPI device's having multiple SpiSerialBusV2 resources in a single _CRS resource list ? 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 ? 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). Regards, Hans
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. > > Regards, > > Hans > We sent a request to the laptop vendor about releasing the SPI DSDT, and after that gets cleared, we will send it to you for review. That will likely be next. Thanks Lucas Tanure
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
diff --git a/Documentation/spi/spi-summary.rst b/Documentation/spi/spi-summary.rst index aab5d07cb3d7..d4239025461d 100644 --- a/Documentation/spi/spi-summary.rst +++ b/Documentation/spi/spi-summary.rst @@ -336,6 +336,14 @@ certainly includes SPI devices hooked up through the card connectors! Non-static Configurations ^^^^^^^^^^^^^^^^^^^^^^^^^ +Developer boards often play by different rules than product boards, and one +example is the potential need to hotplug SPI devices and/or controllers. + +For those cases you might need to use spi_busnum_to_master() to look +up the spi bus master, and will likely need spi_new_device() to provide the +board info based on the board that was hotplugged. Of course, you'd later +call at least spi_unregister_device() when that board is removed. + When Linux includes support for MMC/SD/SDIO/DataFlash cards through SPI, those configurations will also be dynamic. Fortunately, such devices all support basic device identification probes, so they should hotplug normally. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8726309b3eaf..7c81173edb0c 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3145,6 +3145,41 @@ int spi_controller_resume(struct spi_controller *ctlr) } EXPORT_SYMBOL_GPL(spi_controller_resume); +static int __spi_controller_match(struct device *dev, const void *data) +{ + struct spi_controller *ctlr; + const u16 *bus_num = data; + + ctlr = container_of(dev, struct spi_controller, dev); + return ctlr->bus_num == *bus_num; +} + +/** + * spi_busnum_to_master - look up master associated with bus_num + * @bus_num: the master's bus number + * Context: can sleep + * + * This call may be used with devices that are registered after + * arch init time. It returns a refcounted pointer to the relevant + * spi_controller (which the caller must release), or NULL if there is + * no such master registered. + * + * Return: the SPI master structure on success, else NULL. + */ +struct spi_controller *spi_busnum_to_master(u16 bus_num) +{ + struct device *dev; + struct spi_controller *ctlr = NULL; + + dev = class_find_device(&spi_master_class, NULL, &bus_num, + __spi_controller_match); + if (dev) + ctlr = container_of(dev, struct spi_controller, dev); + /* reference got in class_find_device */ + return ctlr; +} +EXPORT_SYMBOL_GPL(spi_busnum_to_master); + /*-------------------------------------------------------------------------*/ /* Core methods for spi_message alterations */ diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index eb7ac8a1e03c..5f2781cb750f 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -759,6 +759,8 @@ extern int devm_spi_register_controller(struct device *dev, struct spi_controller *ctlr); extern void spi_unregister_controller(struct spi_controller *ctlr); +extern struct spi_controller *spi_busnum_to_master(u16 busnum); + /* * SPI resource management while processing a SPI message */