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

[Vinod Koul]

On 23-03-21, 08:43, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@xxxxxxxxxxxxxxx>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> Signed-off-by: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> ---
>  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...

-- 
~Vinod