Re: [PATCH] soundwire: intel: move to auxiliary bus

On 23-03-21, 08:43, Bard Liao wrote:

From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Now that the auxiliary_bus exists, there's no reason to use platform devices as children of a PCI device any longer.

This patch refactors the code by extending a basic auxiliary device with Intel link-specific structures that need to be passed between controller and link levels. This refactoring is much cleaner with no need for cross-pointers between device and link structures.

Note that the auxiliary bus API has separate init and add steps, which requires more attention in the error unwinding paths. The main loop needs to deal with kfree() and auxiliary_device_uninit() for the current iteration before jumping to the common label which releases everything allocated in prior iterations.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

---

drivers/soundwire/Kconfig | 1 + drivers/soundwire/intel.c | 52 ++++---- drivers/soundwire/intel.h | 14 +- drivers/soundwire/intel_init.c | 190 +++++++++++++++++++--------- include/linux/soundwire/sdw_intel.h | 6 +- 5 files changed, 175 insertions(+), 88 deletions(-)

diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 016e74230bb7..2b7795233282 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL tristate "Intel SoundWire Master driver" select SOUNDWIRE_CADENCE select SOUNDWIRE_GENERIC_ALLOCATION

• select AUXILIARY_BUS depends on ACPI && SND_SOC help SoundWire Intel Master driver.

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index d2254ee2fee2..039a101982c9 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -11,7 +11,7 @@ #include <linux/module.h> #include <linux/interrupt.h> #include <linux/io.h> -#include <linux/platform_device.h> +#include <linux/auxiliary_bus.h> #include <sound/pcm_params.h> #include <linux/pm_runtime.h> #include <sound/soc.h> @@ -1331,9 +1331,10 @@ static int intel_init(struct sdw_intel *sdw) /*

• probe and init

*/ -static int intel_master_probe(struct platform_device *pdev) +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id) {

• struct device *dev = &pdev->dev;

• struct device *dev = &auxdev->dev;
• struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);

Do we need another abstractions for resources here, why not aux dev creation set the resources required and we skip this step...

struct sdw_intel *sdw; struct sdw_cdns *cdns; struct sdw_bus *bus; @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev) cdns = &sdw->cdns; bus = &cdns->bus;

• sdw->instance = pdev->id;
• sdw->link_res = dev_get_platdata(dev);

• sdw->instance = auxdev->id;

so auxdev has id and still we pass id as argument :( Not sure if folks can fix this now

• sdw->link_res = &ldev->link_res; cdns->dev = dev; cdns->registers = sdw->link_res->registers; cdns->instance = sdw->instance; cdns->msg_count = 0;

• bus->link_id = pdev->id;

• bus->link_id = auxdev->id;

  sdw_cdns_probe(cdns);

@@ -1386,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev) return 0; }

-int intel_master_startup(struct platform_device *pdev) +int intel_link_startup(struct auxiliary_device *auxdev) { struct sdw_cdns_stream_config config;

• struct device *dev = &pdev->dev;

• struct device *dev = &auxdev->dev; struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus;

@@ -1526,9 +1527,9 @@ int intel_master_startup(struct platform_device *pdev) return ret; }

-static int intel_master_remove(struct platform_device *pdev) +static void intel_link_remove(struct auxiliary_device *auxdev) {

• struct device *dev = &pdev->dev;

• struct device *dev = &auxdev->dev; struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus;

@@ -1544,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev) snd_soc_unregister_component(dev); } sdw_bus_master_delete(bus);

• return 0;

}

-int intel_master_process_wakeen_event(struct platform_device *pdev) +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev) {

• struct device *dev = &pdev->dev;

• struct device *dev = &auxdev->dev; struct sdw_intel *sdw; struct sdw_bus *bus; void __iomem *shim; u16 wake_sts;

• sdw = platform_get_drvdata(pdev);

• sdw = dev_get_drvdata(dev);

No auxdev_get_drvdata() ?

bus = &sdw->cdns.bus;

if (bus->prop.hw_disabled) { @@ -1978,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = { SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL) };

-static struct platform_driver sdw_intel_drv = {

• .probe = intel_master_probe,
• .remove = intel_master_remove,

+static const struct auxiliary_device_id intel_link_id_table[] = {

• { .name = "soundwire_intel.link" },

Curious why name with .link..?

• {},

+}; +MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table);

+static struct auxiliary_driver sdw_intel_drv = {

• .probe = intel_link_probe,
• .remove = intel_link_remove, .driver = {

• .name = "intel-sdw",
  

• /* auxiliary_driver_register() sets .name to be the modname */
  

  .pm = &intel_pm,

• }

• },
• .id_table = intel_link_id_table

};

-module_platform_driver(sdw_intel_drv); +module_auxiliary_driver(sdw_intel_drv);

MODULE_LICENSE("Dual BSD/GPL"); -MODULE_ALIAS("platform:intel-sdw"); -MODULE_DESCRIPTION("Intel Soundwire Master Driver"); +MODULE_DESCRIPTION("Intel Soundwire Link Driver"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 06bac8ba14e9..0b47b148da3f 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -7,7 +7,6 @@ /**

• struct sdw_intel_link_res - Soundwire Intel link resource structure,
• typically populated by the controller driver.

• • @pdev: platform_device
  • @mmio_base: mmio base of SoundWire registers
  • @registers: Link IO registers base
  • @shim: Audio shim pointer

@@ -23,7 +22,6 @@

• @list: used to walk-through all masters exposed by the same controller

*/ struct sdw_intel_link_res {

• struct platform_device *pdev; void __iomem *mmio_base; /* not strictly needed, useful for debug */ void __iomem *registers; void __iomem *shim;

@@ -48,7 +46,15 @@ struct sdw_intel { #endif };

-int intel_master_startup(struct platform_device *pdev); -int intel_master_process_wakeen_event(struct platform_device *pdev); +int intel_link_startup(struct auxiliary_device *auxdev); +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev);

+struct sdw_intel_link_dev {

• struct auxiliary_device auxdev;
• struct sdw_intel_link_res link_res;

+};

I was hoping that with auxdev we can get rid of this init layer, can that still be done?

The auxdev is created by Intel controller, so it sets up resources. I am not really happy that we need to continue having this layer.. can we add something is auxdev core to handle these situations.

What I would like to see the end result is that sdw driver for Intel controller here is a simple auxdev device and no additional custom setup layer required... which implies that this handling should be moved into auxdev or Intel code setting up auxdev...