| Message ID | 20240814132939.308696-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/x86: serial-multi-instantiate: Don't require both I2C and SPI | expand |
Hi, On 8/14/24 3:29 PM, Richard Fitzgerald wrote: > Change the Kconfig dependency so that it doesn't require both I2C and SPI > subsystems to be built. Make a few small changes to the code so that the > code for a bus is only called if the bus is being built. > > When SPI support was added to serial-multi-instantiate it created a > dependency that both CONFIG_I2C and CONFIG_SPI must be enabled. > Typically they are, but there's no reason why this should be a > requirement. A specific kernel build could have only I2C devices > or only SPI devices. It should be possible to use serial-multi-instantiate > if only I2C or only SPI is enabled. > > The dependency formula used is: > > depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) > > The advantage of this approach is that if I2C=m or SPI=m then > SERIAL_MULTI_INSTANTIATE is limited to n/m. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > Changes from V1: > Use a different 'depends on' formula so that serial-multi-instantiate > must be built as a module if any dependencies are a module. > > drivers/platform/x86/Kconfig | 3 +- > .../platform/x86/serial-multi-instantiate.c | 32 ++++++++++++++----- > 2 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 665fa9524986..0dcf4d8eac56 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -999,7 +999,8 @@ config TOPSTAR_LAPTOP > > config SERIAL_MULTI_INSTANTIATE > tristate "Serial bus multi instantiate pseudo device driver" > - depends on I2C && SPI && ACPI > + depends on ACPI > + depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) > help > Some ACPI-based systems list multiple devices in a single ACPI > firmware-node. This driver will instantiate separate clients > diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c > index 3be016cfe601..7c04cc9e5891 100644 > --- a/drivers/platform/x86/serial-multi-instantiate.c > +++ b/drivers/platform/x86/serial-multi-instantiate.c > @@ -83,11 +83,15 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev, > > static void smi_devs_unregister(struct smi *smi) > { > +#if IS_REACHABLE(CONFIG_I2C) > while (smi->i2c_num--) > i2c_unregister_device(smi->i2c_devs[smi->i2c_num]); > +#endif > > - while (smi->spi_num--) > - spi_unregister_device(smi->spi_devs[smi->spi_num]); > + if (IS_REACHABLE(CONFIG_SPI)) { > + while (smi->spi_num--) > + spi_unregister_device(smi->spi_devs[smi->spi_num]); > + } > } > > /** > @@ -258,9 +262,15 @@ static int smi_probe(struct platform_device *pdev) > > switch (node->bus_type) { > case SMI_I2C: > - return smi_i2c_probe(pdev, smi, node->instances); > + if (IS_REACHABLE(CONFIG_I2C)) > + return smi_i2c_probe(pdev, smi, node->instances); > + > + return -ENODEV; > case SMI_SPI: > - return smi_spi_probe(pdev, smi, node->instances); > + if (IS_REACHABLE(CONFIG_SPI)) > + return smi_spi_probe(pdev, smi, node->instances); > + > + return -ENODEV; > case SMI_AUTO_DETECT: > /* > * For backwards-compatibility with the existing nodes I2C > @@ -270,10 +280,16 @@ static int smi_probe(struct platform_device *pdev) > * SpiSerialBus nodes that were previously ignored, and this > * preserves that behavior. > */ > - ret = smi_i2c_probe(pdev, smi, node->instances); > - if (ret != -ENOENT) > - return ret; > - return smi_spi_probe(pdev, smi, node->instances); > + if (IS_REACHABLE(CONFIG_I2C)) { > + ret = smi_i2c_probe(pdev, smi, node->instances); > + if (ret != -ENOENT) > + return ret; > + } > + > + if (IS_REACHABLE(CONFIG_SPI)) > + return smi_spi_probe(pdev, smi, node->instances); > + > + return -ENODEV; > default: > return -EINVAL; > }
Wed, Aug 14, 2024 at 02:29:39PM +0100, Richard Fitzgerald kirjoitti: > Change the Kconfig dependency so that it doesn't require both I2C and SPI > subsystems to be built. Make a few small changes to the code so that the > code for a bus is only called if the bus is being built. > > When SPI support was added to serial-multi-instantiate it created a > dependency that both CONFIG_I2C and CONFIG_SPI must be enabled. > Typically they are, but there's no reason why this should be a > requirement. A specific kernel build could have only I2C devices > or only SPI devices. It should be possible to use serial-multi-instantiate > if only I2C or only SPI is enabled. > > The dependency formula used is: > > depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) > > The advantage of this approach is that if I2C=m or SPI=m then > SERIAL_MULTI_INSTANTIATE is limited to n/m. ... > static void smi_devs_unregister(struct smi *smi) > { > +#if IS_REACHABLE(CONFIG_I2C) There is no explanation why ugly ifdeffery is used here, while normal conditionals elsewhere. > while (smi->i2c_num--) > i2c_unregister_device(smi->i2c_devs[smi->i2c_num]); > +#endif > > - while (smi->spi_num--) > - spi_unregister_device(smi->spi_devs[smi->spi_num]); > + if (IS_REACHABLE(CONFIG_SPI)) { > + while (smi->spi_num--) > + spi_unregister_device(smi->spi_devs[smi->spi_num]); > + } > } There are ways to solve this: 1) add a stub for I2C=n for i2c_unregister_device(); 2) resplit this driver to have several built modules: core, I2C parts, SPI parts.
Hi Andy, On 8/26/24 10:10 PM, Andy Shevchenko wrote: > Wed, Aug 14, 2024 at 02:29:39PM +0100, Richard Fitzgerald kirjoitti: >> Change the Kconfig dependency so that it doesn't require both I2C and SPI >> subsystems to be built. Make a few small changes to the code so that the >> code for a bus is only called if the bus is being built. >> >> When SPI support was added to serial-multi-instantiate it created a >> dependency that both CONFIG_I2C and CONFIG_SPI must be enabled. >> Typically they are, but there's no reason why this should be a >> requirement. A specific kernel build could have only I2C devices >> or only SPI devices. It should be possible to use serial-multi-instantiate >> if only I2C or only SPI is enabled. >> >> The dependency formula used is: >> >> depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) >> >> The advantage of this approach is that if I2C=m or SPI=m then >> SERIAL_MULTI_INSTANTIATE is limited to n/m. > > ... > >> static void smi_devs_unregister(struct smi *smi) >> { >> +#if IS_REACHABLE(CONFIG_I2C) > > There is no explanation why ugly ifdeffery is used here, while normal > conditionals elsewhere. Note that this has already been merged as is, as you've figured out yourself the reason to use #ifdef here is because there is no i2c_unregister_device() prototype declared when CONFIG_I2C=n > >> while (smi->i2c_num--) >> i2c_unregister_device(smi->i2c_devs[smi->i2c_num]); >> +#endif >> >> - while (smi->spi_num--) >> - spi_unregister_device(smi->spi_devs[smi->spi_num]); >> + if (IS_REACHABLE(CONFIG_SPI)) { >> + while (smi->spi_num--) >> + spi_unregister_device(smi->spi_devs[smi->spi_num]); >> + } >> } > > There are ways to solve this: > 1) add a stub for I2C=n for i2c_unregister_device(); Yes that would be an option to clean this up a bit as a follow-up patch series. Note no need for a stub, just move the declaration out of the #if IS_ENABLED(CONFIG_I2C) block, using if (IS_REACHABLE) only requires a prototype. > 2) resplit this driver to have several built modules: > core, I2C parts, SPI parts. > Regards, Hans
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 665fa9524986..0dcf4d8eac56 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -999,7 +999,8 @@ config TOPSTAR_LAPTOP config SERIAL_MULTI_INSTANTIATE tristate "Serial bus multi instantiate pseudo device driver" - depends on I2C && SPI && ACPI + depends on ACPI + depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) help Some ACPI-based systems list multiple devices in a single ACPI firmware-node. This driver will instantiate separate clients diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c index 3be016cfe601..7c04cc9e5891 100644 --- a/drivers/platform/x86/serial-multi-instantiate.c +++ b/drivers/platform/x86/serial-multi-instantiate.c @@ -83,11 +83,15 @@ static int smi_get_irq(struct platform_device *pdev, struct acpi_device *adev, static void smi_devs_unregister(struct smi *smi) { +#if IS_REACHABLE(CONFIG_I2C) while (smi->i2c_num--) i2c_unregister_device(smi->i2c_devs[smi->i2c_num]); +#endif - while (smi->spi_num--) - spi_unregister_device(smi->spi_devs[smi->spi_num]); + if (IS_REACHABLE(CONFIG_SPI)) { + while (smi->spi_num--) + spi_unregister_device(smi->spi_devs[smi->spi_num]); + } } /** @@ -258,9 +262,15 @@ static int smi_probe(struct platform_device *pdev) switch (node->bus_type) { case SMI_I2C: - return smi_i2c_probe(pdev, smi, node->instances); + if (IS_REACHABLE(CONFIG_I2C)) + return smi_i2c_probe(pdev, smi, node->instances); + + return -ENODEV; case SMI_SPI: - return smi_spi_probe(pdev, smi, node->instances); + if (IS_REACHABLE(CONFIG_SPI)) + return smi_spi_probe(pdev, smi, node->instances); + + return -ENODEV; case SMI_AUTO_DETECT: /* * For backwards-compatibility with the existing nodes I2C @@ -270,10 +280,16 @@ static int smi_probe(struct platform_device *pdev) * SpiSerialBus nodes that were previously ignored, and this * preserves that behavior. */ - ret = smi_i2c_probe(pdev, smi, node->instances); - if (ret != -ENOENT) - return ret; - return smi_spi_probe(pdev, smi, node->instances); + if (IS_REACHABLE(CONFIG_I2C)) { + ret = smi_i2c_probe(pdev, smi, node->instances); + if (ret != -ENOENT) + return ret; + } + + if (IS_REACHABLE(CONFIG_SPI)) + return smi_spi_probe(pdev, smi, node->instances); + + return -ENODEV; default: return -EINVAL; }
Change the Kconfig dependency so that it doesn't require both I2C and SPI subsystems to be built. Make a few small changes to the code so that the code for a bus is only called if the bus is being built. When SPI support was added to serial-multi-instantiate it created a dependency that both CONFIG_I2C and CONFIG_SPI must be enabled. Typically they are, but there's no reason why this should be a requirement. A specific kernel build could have only I2C devices or only SPI devices. It should be possible to use serial-multi-instantiate if only I2C or only SPI is enabled. The dependency formula used is: depends on (I2C && !SPI) || (!I2C && SPI) || (I2C && SPI) The advantage of this approach is that if I2C=m or SPI=m then SERIAL_MULTI_INSTANTIATE is limited to n/m. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- Changes from V1: Use a different 'depends on' formula so that serial-multi-instantiate must be built as a module if any dependencies are a module. drivers/platform/x86/Kconfig | 3 +- .../platform/x86/serial-multi-instantiate.c | 32 ++++++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-)