Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:

Use sdw_master_device and driver instead of platform devices

To quote GregKH:

"Don't mess with a platform device unless you really have no other possible choice. And even then, don't do it and try to do something else. Platform devices are really abused, don't perpetuate it "

In addition, rather than a plain-vanilla init/exit, this patch provides 3 steps in the initialization (ACPI scan, probe, startup) which makes it easier to verify hardware support for SoundWire, allocate required resources as early as possible, and conversely help make the startup() callback lighter-weight with only hardware register setup.

...

+struct sdw_md_driver intel_sdw_driver = {

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

};

...

+extern struct sdw_md_driver intel_sdw_driver;

who uses this intel_sdw_driver? I would assumed someone would register this with the core...

+static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res) +{

• struct sdw_intel_link_res *link;

• struct sdw_intel_ctx *ctx;

• struct acpi_device *adev;

• struct sdw_master_device *md;

• u32 link_mask;

• int count;

• int i;

• if (!res)

• return NULL;
  

• if (acpi_bus_get_device(res->handle, &adev))

• return NULL;
  

• if (!res->count)

• return NULL;
  

• count = res->count; dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);

  ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL;

• ctx->count = count;
• ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);

• ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL); if (!ctx->links) goto link_err;

• ctx->count = count;

• ctx->mmio_base = res->mmio_base;

• ctx->link_mask = res->link_mask;

• ctx->handle = res->handle;

• link = ctx->links;

• link_mask = ctx->link_mask;

  /* Create SDW Master devices */

• for (i = 0; i < count; i++) {

• if (link_mask && !(link_mask & BIT(i))) {
  

• 	dev_dbg(&adev->dev,
  

• 		"Link %d masked, will not be enabled\n", i);
  

• 	link++;
  

• for (i = 0; i < count; i++, link++) {

• if (link_mask && !(link_mask & BIT(i)))
  continue;
  

• }
  
  

• md = sdw_md_add(&intel_sdw_driver,
  

• 		res->parent,
  

• 		acpi_fwnode_handle(adev),
  

• 		i);
  

• if (IS_ERR(md)) {
  

• 	dev_err(&adev->dev, "Could not create link %d\n", i);
  

• 	goto err;
  

• }
  

• link->md = md;
  

• link->mmio_base = res->mmio_base;
  

  link->registers = res->mmio_base + SDW_LINK_BASE

• 			+ (SDW_LINK_SIZE * i);
  

• 	+ (SDW_LINK_SIZE * i);
  

  link->shim = res->mmio_base + SDW_SHIM_BASE; link->alh = res->mmio_base + SDW_ALH_BASE;

• link->irq = res->irq;
  

  link->ops = res->ops; link->dev = res->dev;

• memset(&pdevinfo, 0, sizeof(pdevinfo));
  

• pdevinfo.parent = res->parent;
  

• pdevinfo.name = "int-sdw";
  

• pdevinfo.id = i;
  

• pdevinfo.fwnode = acpi_fwnode_handle(adev);
  

• pdev = platform_device_register_full(&pdevinfo);
  

• if (IS_ERR(pdev)) {
  

• 	dev_err(&adev->dev,
  

• 		"platform device creation failed: %ld\n",
  

• 		PTR_ERR(pdev));
  

• 	goto pdev_err;
  

• }
  

• link->pdev = pdev;
  

• link++;
  

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

That reminds me I am missing match code for master driver...

So we seem to be somewhere is middle wrt driver probing here! IIUC this is not a full master driver, thats okay, but then it is not completely transparent either...

I was somehow thinking that the driver will continue to be 'platform/acpi/of' driver and master device abstraction will be handled in the core (for example see how the busses like i2c handle this). The master device is created and used to represent but driver probing etc is not done

Thoughts..?