| Message ID | 20211202162421.7628-1-sbinding@opensource.cirrus.com |
|---|---|
| State | New |
| 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. 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 Stefan, On 12/2/21 17:24, Stefan Binding wrote: > Add support for spi bus in i2c-multi-instantiate driver > and rename it for a multiple purpose driver name > By adding spi support into this driver enables devices > to use the same _HID string for i2c and spi uses and > minimize the support for two drivers doing the same thing > for different busses > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > --- > MAINTAINERS | 4 +- > drivers/acpi/scan.c | 19 +- > drivers/platform/x86/Kconfig | 14 +- > drivers/platform/x86/Makefile | 2 +- > drivers/platform/x86/bus-multi-instantiate.c | 432 +++++++++++++++++++ > drivers/platform/x86/i2c-multi-instantiate.c | 174 -------- git's rename detection is failing here. Please split this in 2 patches: 1. Just the rename without any code changes (outside of Kconfig + Makefile) 2. The actual code changes This will make it much easier to review what has actually changed and will also make git log --follow work properly. I have one more remark below. > 6 files changed, 452 insertions(+), 193 deletions(-) > create mode 100644 drivers/platform/x86/bus-multi-instantiate.c > delete mode 100644 drivers/platform/x86/i2c-multi-instantiate.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5b1e79f8e3d8..f75600d917bc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -387,11 +387,11 @@ L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) > S: Maintained > F: drivers/acpi/arm64 > > -ACPI I2C MULTI INSTANTIATE DRIVER > +ACPI BUS MULTI INSTANTIATE DRIVER > M: Hans de Goede <hdegoede@redhat.com> > L: platform-driver-x86@vger.kernel.org > S: Maintained > -F: drivers/platform/x86/i2c-multi-instantiate.c > +F: drivers/platform/x86/bus-multi-instantiate.c > > ACPI PCC(Platform Communication Channel) MAILBOX DRIVER > M: Sudeep Holla <sudeep.holla@arm.com> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 5991dddbc9ce..2f7da1a08112 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -1696,14 +1696,15 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device) > struct list_head resource_list; > bool is_serial_bus_slave = false; > /* > - * These devices have multiple I2cSerialBus resources and an i2c-client > - * must be instantiated for each, each with its own i2c_device_id. > - * Normally we only instantiate an i2c-client for the first resource, > - * using the ACPI HID as id. These special cases are handled by the > - * drivers/platform/x86/i2c-multi-instantiate.c driver, which knows > - * which i2c_device_id to use for each resource. > + * These devices have multiple I2cSerialBus/SpiSerialBusV2 resources > + * and an (i2c/spi)-client must be instantiated for each, each with > + * its own i2c_device_id/spi_device_id. > + * Normally we only instantiate an (i2c/spi)-client for the first > + * resource, using the ACPI HID as id. These special cases are handled > + * by the drivers/platform/x86/bus-multi-instantiate.c driver, which > + * knows which i2c_device_id or spi_device_id to use for each resource. > */ > - static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > + static const struct acpi_device_id bus_multi_instantiate_ids[] = { > {"BSG1160", }, > {"BSG2150", }, > {"INT33FE", }, > @@ -1721,8 +1722,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device) > fwnode_property_present(&device->fwnode, "baud"))) > return true; > > - /* Instantiate a pdev for the i2c-multi-instantiate drv to bind to */ > - if (!acpi_match_device_ids(device, i2c_multi_instantiate_ids)) > + /* Instantiate a pdev for the bus-multi-instantiate drv to bind to */ > + if (!acpi_match_device_ids(device, bus_multi_instantiate_ids)) > return false; > > INIT_LIST_HEAD(&resource_list); > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 97e87628eb35..5a413b123c01 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -958,16 +958,16 @@ config TOPSTAR_LAPTOP > > If you have a Topstar laptop, say Y or M here. > > -config I2C_MULTI_INSTANTIATE > - tristate "I2C multi instantiate pseudo device driver" > - depends on I2C && ACPI > +config BUS_MULTI_INSTANTIATE > + tristate "Bus multi instantiate pseudo device driver" > + depends on I2C && SPI && ACPI > help > - Some ACPI-based systems list multiple i2c-devices in a single ACPI > - firmware-node. This driver will instantiate separate i2c-clients > - for each device in the firmware-node. > + Some ACPI-based systems list multiple i2c/spi devices in a > + single ACPI firmware-node. This driver will instantiate separate > + i2c-clients or spi-devices for each device in the firmware-node. > > To compile this driver as a module, choose M here: the module > - will be called i2c-multi-instantiate. > + will be called bus-multi-instantiate. > > config MLX_PLATFORM > tristate "Mellanox Technologies platform support" > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 219478061683..639a50af0bec 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -108,7 +108,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > > # Platform drivers > obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > -obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > +obj-$(CONFIG_BUS_MULTI_INSTANTIATE) += bus-multi-instantiate.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o > obj-$(CONFIG_WIRELESS_HOTKEY) += wireless-hotkey.o > diff --git a/drivers/platform/x86/bus-multi-instantiate.c b/drivers/platform/x86/bus-multi-instantiate.c > new file mode 100644 > index 000000000000..1b55380a2057 > --- /dev/null > +++ b/drivers/platform/x86/bus-multi-instantiate.c > @@ -0,0 +1,432 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Bus multi-instantiate driver, pseudo driver to instantiate multiple > + * i2c-clients or spi-devices from a single fwnode. > + * > + * Copyright 2018 Hans de Goede <hdegoede@redhat.com> > + */ > + > +#include <linux/acpi.h> > +#include <linux/bits.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > +#include <linux/spi/spi.h> > +#include <linux/types.h> > + > +#define IRQ_RESOURCE_TYPE GENMASK(1, 0) > +#define IRQ_RESOURCE_NONE 0 > +#define IRQ_RESOURCE_GPIO 1 > +#define IRQ_RESOURCE_APIC 2 > +#define MAX_RESOURCE_SOURCE_CHAR 30 > + > +struct inst_data { > + const char *type; > + unsigned int flags; > + int irq_idx; > +}; > + > +struct multi_inst_data { > + int i2c_num; > + int spi_num; > + struct spi_device **spi_devs; > + struct i2c_client **i2c_devs; > +}; > + > +struct spi_acpi_data { > + char resource_source[MAX_RESOURCE_SOURCE_CHAR]; > + struct acpi_resource_spi_serialbus sb; > +}; > + > +struct spi_serialbus_acpi_data { > + int count; > + struct spi_acpi_data acpi_data[]; > +}; > + > +static int spi_count(struct acpi_resource *ares, void *data) > +{ > + struct acpi_resource_spi_serialbus *sb; > + int *count = data; > + > + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) > + return 1; > + > + sb = &ares->data.spi_serial_bus; > + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI) > + return 1; > + > + *count = *count + 1; > + return 1; > +} > + > +static int spi_count_resources(struct acpi_device *adev) > +{ > + LIST_HEAD(r); > + int count = 0; > + int ret; > + > + ret = acpi_dev_get_resources(adev, &r, spi_count, &count); > + if (ret < 0) > + return ret; > + > + acpi_dev_free_resource_list(&r); > + return count; > +} > + > +static int spi_save_res(struct acpi_resource *ares, void *data) > +{ > + struct acpi_resource_spi_serialbus *sb; > + struct spi_serialbus_acpi_data *resources = data; > + > + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) > + return 1; > + > + sb = &ares->data.spi_serial_bus; > + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI) > + return 1; > + > + memcpy(&resources->acpi_data[resources->count].sb, sb, sizeof(*sb)); > + strscpy(resources->acpi_data[resources->count].resource_source, > + sb->resource_source.string_ptr, > + sizeof(resources->acpi_data[resources->count].resource_source)); > + resources->count++; > + > + return 1; > +} > + > +static struct spi_serialbus_acpi_data *spi_get_resources(struct device *dev, > + struct acpi_device *adev, int count) > +{ > + struct spi_serialbus_acpi_data *resources; > + LIST_HEAD(r); > + int ret; > + > + resources = kmalloc(struct_size(resources, acpi_data, count), GFP_KERNEL); > + if (!resources) > + return NULL; > + > + ret = acpi_dev_get_resources(adev, &r, spi_save_res, resources); > + if (ret < 0) > + goto error; > + > + acpi_dev_free_resource_list(&r); > + > + return resources; > + > +error: > + kfree(resources); > + return NULL; > +} > + > +static struct spi_controller *find_spi_controller(char *path) > +{ > + struct spi_controller *ctlr; > + acpi_handle parent_handle; > + acpi_status status; > + int i; > + > + status = acpi_get_handle(NULL, path, &parent_handle); > + if (ACPI_FAILURE(status)) > + return NULL; > + > + /* There will be not more than 10 spi controller for a device */ > + for (i = 0 ; i < 10 ; i++) { > + ctlr = spi_busnum_to_master(i); > + if (ctlr && ACPI_HANDLE(ctlr->dev.parent) == parent_handle) > + return ctlr; > + } > + > + return NULL; > +} > + > +static int spi_multi_inst_probe(struct platform_device *pdev, struct acpi_device *adev, > + const struct inst_data *inst_data, int count) > +{ > + struct spi_serialbus_acpi_data *acpi_data; > + struct device *dev = &pdev->dev; > + struct multi_inst_data *multi; > + struct spi_controller *ctlr; > + struct spi_device *spi_dev; > + char name[50]; > + int i, ret; > + > + multi = devm_kzalloc(dev, sizeof(*multi), GFP_KERNEL); > + if (!multi) > + return -ENOMEM; > + > + multi->spi_devs = devm_kcalloc(dev, count, sizeof(*multi->spi_devs), GFP_KERNEL); > + if (!multi->spi_devs) > + return -ENOMEM; > + > + acpi_data = spi_get_resources(dev, adev, count); > + if (!acpi_data) > + return -ENOMEM; > + > + for (i = 0; i < count && inst_data[i].type; i++) { > + ctlr = find_spi_controller(acpi_data->acpi_data[i].resource_source); > + if (!ctlr) { > + ret = -EPROBE_DEFER; > + goto probe_error; > + } > + > + spi_dev = spi_alloc_device(ctlr); > + if (!spi_dev) { > + dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n", > + dev_name(&adev->dev)); > + ret = -ENOMEM; > + goto probe_error; > + } > + > + strscpy(spi_dev->modalias, inst_data[i].type, sizeof(spi_dev->modalias)); > + > + if (ctlr->fw_translate_cs) { > + int cs = ctlr->fw_translate_cs(ctlr, > + acpi_data->acpi_data[i].sb.device_selection); > + if (cs < 0) { > + ret = cs; > + goto probe_error; > + } > + spi_dev->chip_select = cs; > + } else { > + spi_dev->chip_select = acpi_data->acpi_data[i].sb.device_selection; > + } > + > + spi_dev->max_speed_hz = acpi_data->acpi_data[i].sb.connection_speed; > + spi_dev->bits_per_word = acpi_data->acpi_data[i].sb.data_bit_length; > + > + if (acpi_data->acpi_data[i].sb.clock_phase == ACPI_SPI_SECOND_PHASE) > + spi_dev->mode |= SPI_CPHA; > + if (acpi_data->acpi_data[i].sb.clock_polarity == ACPI_SPI_START_HIGH) > + spi_dev->mode |= SPI_CPOL; > + if (acpi_data->acpi_data[i].sb.device_polarity == ACPI_SPI_ACTIVE_HIGH) > + spi_dev->mode |= SPI_CS_HIGH; > + > + switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) { > + case IRQ_RESOURCE_GPIO: > + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Error requesting irq at index %d: %d\n", > + inst_data[i].irq_idx, ret); > + goto probe_error; > + } > + spi_dev->irq = ret; > + break; > + case IRQ_RESOURCE_APIC: > + ret = platform_get_irq(pdev, inst_data[i].irq_idx); > + if (ret < 0) { > + dev_dbg(dev, "Error requesting irq at index %d: %d\n", > + inst_data[i].irq_idx, ret); > + goto probe_error; > + } > + spi_dev->irq = ret; > + break; > + default: > + spi_dev->irq = 0; > + break; > + } This switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) { } block is duplicated with the i2c code, please put this in some get_irq helper, so that you end up writing: spi_dev->irq = multi_inst_get_irq(pdev, &inst_data[i]); Here. > + > + snprintf(name, sizeof(name), "%s.%u-%s-%s.%d", dev_name(&ctlr->dev), > + spi_dev->chip_select, dev_name(dev), inst_data[i].type, i); > + spi_dev->dev.init_name = name; > + > + if (spi_add_device(spi_dev)) { > + dev_err(&ctlr->dev, "failed to add SPI device %s from ACPI\n", > + dev_name(&adev->dev)); > + spi_dev_put(spi_dev); > + goto probe_error; > + } > + > + multi->spi_devs[i] = spi_dev; > + multi->spi_num++; > + } > + > + if (multi->spi_num < count) { > + dev_err(dev, "Error finding driver, idx %d\n", i); > + ret = -ENODEV; > + goto probe_error; > + } > + > + dev_info(dev, "Instantiate %d devices.\n", multi->spi_num); > + platform_set_drvdata(pdev, multi); > + kfree(acpi_data); > + > + return 0; > + > +probe_error: > + while (--i >= 0) > + spi_unregister_device(multi->spi_devs[i]); > + > + kfree(acpi_data); > + return ret; > +} > + > +static int i2c_multi_inst_probe(struct platform_device *pdev, struct acpi_device *adev, > + const struct inst_data *inst_data, int count) > +{ > + struct i2c_board_info board_info = {}; > + struct device *dev = &pdev->dev; > + struct multi_inst_data *multi; > + char name[32]; > + int i, ret; > + > + multi = devm_kzalloc(dev, sizeof(*multi), GFP_KERNEL); > + if (!multi) > + return -ENOMEM; > + > + multi->i2c_devs = devm_kcalloc(dev, count, sizeof(*multi->i2c_devs), GFP_KERNEL); > + if (!multi->i2c_devs) > + return -ENOMEM; > + > + for (i = 0; i < count && inst_data[i].type; i++) { > + memset(&board_info, 0, sizeof(board_info)); > + strscpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); > + snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst_data[i].type, i); > + board_info.dev_name = name; > + switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) { > + case IRQ_RESOURCE_GPIO: > + ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Error requesting irq at index %d: %d\n", > + inst_data[i].irq_idx, ret); > + goto error; > + } > + board_info.irq = ret; > + break; > + case IRQ_RESOURCE_APIC: > + ret = platform_get_irq(pdev, inst_data[i].irq_idx); > + if (ret < 0) { > + dev_dbg(dev, "Error requesting irq at index %d: %d\n", > + inst_data[i].irq_idx, ret); > + goto error; > + } > + board_info.irq = ret; > + break; > + default: > + board_info.irq = 0; > + break; > + } > + multi->i2c_devs[i] = i2c_acpi_new_device(dev, i, &board_info); > + if (IS_ERR(multi->i2c_devs[i])) { > + ret = dev_err_probe(dev, PTR_ERR(multi->i2c_devs[i]), > + "Error creating i2c-client, idx %d\n", i); > + goto error; > + } > + multi->i2c_num++; > + } > + if (multi->i2c_num < count) { > + dev_err(dev, "Error finding driver, idx %d\n", i); > + ret = -ENODEV; > + goto error; > + } > + > + dev_info(dev, "Instantiate %d devices.\n", multi->i2c_num); > + platform_set_drvdata(pdev, multi); > + > + return 0; > + > +error: > + while (--i >= 0) > + i2c_unregister_device(multi->i2c_devs[i]); > + > + return ret; > +} > + > +static int bus_multi_inst_probe(struct platform_device *pdev) > +{ > + const struct inst_data *inst_data; > + struct device *dev = &pdev->dev; > + struct acpi_device *adev; > + int count; > + > + inst_data = device_get_match_data(dev); > + if (!inst_data) { > + dev_err(dev, "Error ACPI match data is missing\n"); > + return -ENODEV; > + } > + > + adev = ACPI_COMPANION(dev); You should check that adev != NULL here, the old code relied on i2c_acpi_client_count() returning an error for adev == NULL, but since you are now just warning when that fails an explicit check for this would be good. > + > + /* Count number of i2c clients to instantiate */ > + count = i2c_acpi_client_count(adev); > + if (count > 0) > + return i2c_multi_inst_probe(pdev, adev, inst_data, count); What if an ACPI device has both i2c and spi resources ? Regards, Hans > + else if (count < 0) > + dev_warn(dev, "I2C multi instantiate error %d\n", count); > + > + /* Count number of spi devices to instantiate */ > + count = spi_count_resources(adev); > + if (count > 0) > + return spi_multi_inst_probe(pdev, adev, inst_data, count); > + else if (count < 0) > + dev_warn(dev, "SPI multi instantiate error %d\n", count); > + > + return -ENODEV; > +} > + > +static int bus_multi_inst_remove(struct platform_device *pdev) > +{ > + struct multi_inst_data *multi = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < multi->i2c_num; i++) > + i2c_unregister_device(multi->i2c_devs[i]); > + > + for (i = 0; i < multi->spi_num; i++) > + spi_unregister_device(multi->spi_devs[i]); > + > + return 0; > +} > + > +static const struct inst_data bsg1160_data[] = { > + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 }, > + { "bmc150_magn" }, > + { "bmg160" }, > + {} > +}; > + > +static const struct inst_data bsg2150_data[] = { > + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 }, > + { "bmc150_magn" }, > + /* The resources describe a 3th client, but it is not really there. */ > + { "bsg2150_dummy_dev" }, > + {} > +}; > + > +static const struct inst_data int3515_data[] = { > + { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > + { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > + { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > + { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > + {} > +}; > + > +/* > + * Note new device-ids must also be added to bus_multi_instantiate_ids in > + * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). > + */ > +static const struct acpi_device_id bus_multi_inst_acpi_ids[] = { > + { "BSG1160", (unsigned long)bsg1160_data }, > + { "BSG2150", (unsigned long)bsg2150_data }, > + { "INT3515", (unsigned long)int3515_data }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, bus_multi_inst_acpi_ids); > + > +static struct platform_driver bus_multi_inst_driver = { > + .driver = { > + .name = "Bus multi instantiate pseudo device driver", > + .acpi_match_table = bus_multi_inst_acpi_ids, > + }, > + .probe = bus_multi_inst_probe, > + .remove = bus_multi_inst_remove, > +}; > +module_platform_driver(bus_multi_inst_driver); > + > +MODULE_DESCRIPTION("Bus multi instantiate pseudo device driver"); > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c > deleted file mode 100644 > index 4956a1df5b90..000000000000 > --- a/drivers/platform/x86/i2c-multi-instantiate.c > +++ /dev/null > @@ -1,174 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0+ > -/* > - * I2C multi-instantiate driver, pseudo driver to instantiate multiple > - * i2c-clients from a single fwnode. > - * > - * Copyright 2018 Hans de Goede <hdegoede@redhat.com> > - */ > - > -#include <linux/acpi.h> > -#include <linux/bits.h> > -#include <linux/i2c.h> > -#include <linux/interrupt.h> > -#include <linux/kernel.h> > -#include <linux/module.h> > -#include <linux/platform_device.h> > -#include <linux/property.h> > -#include <linux/types.h> > - > -#define IRQ_RESOURCE_TYPE GENMASK(1, 0) > -#define IRQ_RESOURCE_NONE 0 > -#define IRQ_RESOURCE_GPIO 1 > -#define IRQ_RESOURCE_APIC 2 > - > -struct i2c_inst_data { > - const char *type; > - unsigned int flags; > - int irq_idx; > -}; > - > -struct i2c_multi_inst_data { > - int num_clients; > - struct i2c_client *clients[]; > -}; > - > -static int i2c_multi_inst_probe(struct platform_device *pdev) > -{ > - struct i2c_multi_inst_data *multi; > - const struct i2c_inst_data *inst_data; > - struct i2c_board_info board_info = {}; > - struct device *dev = &pdev->dev; > - struct acpi_device *adev; > - char name[32]; > - int i, ret; > - > - inst_data = device_get_match_data(dev); > - if (!inst_data) { > - dev_err(dev, "Error ACPI match data is missing\n"); > - return -ENODEV; > - } > - > - adev = ACPI_COMPANION(dev); > - > - /* Count number of clients to instantiate */ > - ret = i2c_acpi_client_count(adev); > - if (ret < 0) > - return ret; > - > - multi = devm_kmalloc(dev, struct_size(multi, clients, ret), GFP_KERNEL); > - if (!multi) > - return -ENOMEM; > - > - multi->num_clients = ret; > - > - for (i = 0; i < multi->num_clients && inst_data[i].type; i++) { > - memset(&board_info, 0, sizeof(board_info)); > - strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE); > - snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), > - inst_data[i].type, i); > - board_info.dev_name = name; > - switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) { > - case IRQ_RESOURCE_GPIO: > - ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); > - if (ret < 0) { > - dev_err(dev, "Error requesting irq at index %d: %d\n", > - inst_data[i].irq_idx, ret); > - goto error; > - } > - board_info.irq = ret; > - break; > - case IRQ_RESOURCE_APIC: > - ret = platform_get_irq(pdev, inst_data[i].irq_idx); > - if (ret < 0) { > - dev_dbg(dev, "Error requesting irq at index %d: %d\n", > - inst_data[i].irq_idx, ret); > - goto error; > - } > - board_info.irq = ret; > - break; > - default: > - board_info.irq = 0; > - break; > - } > - multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info); > - if (IS_ERR(multi->clients[i])) { > - ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]), > - "Error creating i2c-client, idx %d\n", i); > - goto error; > - } > - } > - if (i < multi->num_clients) { > - dev_err(dev, "Error finding driver, idx %d\n", i); > - ret = -ENODEV; > - goto error; > - } > - > - platform_set_drvdata(pdev, multi); > - return 0; > - > -error: > - while (--i >= 0) > - i2c_unregister_device(multi->clients[i]); > - > - return ret; > -} > - > -static int i2c_multi_inst_remove(struct platform_device *pdev) > -{ > - struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev); > - int i; > - > - for (i = 0; i < multi->num_clients; i++) > - i2c_unregister_device(multi->clients[i]); > - > - return 0; > -} > - > -static const struct i2c_inst_data bsg1160_data[] = { > - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 }, > - { "bmc150_magn" }, > - { "bmg160" }, > - {} > -}; > - > -static const struct i2c_inst_data bsg2150_data[] = { > - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 }, > - { "bmc150_magn" }, > - /* The resources describe a 3th client, but it is not really there. */ > - { "bsg2150_dummy_dev" }, > - {} > -}; > - > -static const struct i2c_inst_data int3515_data[] = { > - { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > - {} > -}; > - > -/* > - * Note new device-ids must also be added to i2c_multi_instantiate_ids in > - * drivers/acpi/scan.c: acpi_device_enumeration_by_parent(). > - */ > -static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { > - { "BSG1160", (unsigned long)bsg1160_data }, > - { "BSG2150", (unsigned long)bsg2150_data }, > - { "INT3515", (unsigned long)int3515_data }, > - { } > -}; > -MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); > - > -static struct platform_driver i2c_multi_inst_driver = { > - .driver = { > - .name = "I2C multi instantiate pseudo device driver", > - .acpi_match_table = i2c_multi_inst_acpi_ids, > - }, > - .probe = i2c_multi_inst_probe, > - .remove = i2c_multi_inst_remove, > -}; > -module_platform_driver(i2c_multi_inst_driver); > - > -MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver"); > -MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); > -MODULE_LICENSE("GPL"); >
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 */