| Message ID | 20190916212342.12578-9-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: add Master device support, GreyBus style | expand |
On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: > +/** > + * sdw_intel_probe() - SoundWire Intel probe routine > + * @parent_handle: ACPI parent handle > + * @res: resource data > + * > + * This creates SoundWire Master and Slave devices below the controller. > + * All the information necessary is stored in the context, and the res > + * argument pointer can be freed after this step. > + */ > +struct sdw_intel_ctx > +*sdw_intel_probe(struct sdw_intel_res *res) > +{ > + return sdw_intel_probe_controller(res); > +} > +EXPORT_SYMBOL(sdw_intel_probe); > + > +/** > + * sdw_intel_startup() - SoundWire Intel startup > + * @ctx: SoundWire context allocated in the probe > + * > + */ > +int sdw_intel_startup(struct sdw_intel_ctx *ctx) > +{ > + return sdw_intel_startup_controller(ctx); > +} > +EXPORT_SYMBOL(sdw_intel_startup); Why are you exporting these functions if no one calls them? thanks, greg k-h
On 9/17/19 12:55 AM, Greg KH wrote: > On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: >> +/** >> + * sdw_intel_probe() - SoundWire Intel probe routine >> + * @parent_handle: ACPI parent handle >> + * @res: resource data >> + * >> + * This creates SoundWire Master and Slave devices below the controller. >> + * All the information necessary is stored in the context, and the res >> + * argument pointer can be freed after this step. >> + */ >> +struct sdw_intel_ctx >> +*sdw_intel_probe(struct sdw_intel_res *res) >> +{ >> + return sdw_intel_probe_controller(res); >> +} >> +EXPORT_SYMBOL(sdw_intel_probe); >> + >> +/** >> + * sdw_intel_startup() - SoundWire Intel startup >> + * @ctx: SoundWire context allocated in the probe >> + * >> + */ >> +int sdw_intel_startup(struct sdw_intel_ctx *ctx) >> +{ >> + return sdw_intel_startup_controller(ctx); >> +} >> +EXPORT_SYMBOL(sdw_intel_startup); > > Why are you exporting these functions if no one calls them? They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: add SoundWire configuration interface' +int hda_sdw_startup(struct snd_sof_dev *sdev) +{ + struct sof_intel_hda_dev *hdev; + int ret; + + hdev = sdev->pdata->hw_pdata; + + ret = sdw_intel_startup(hdev->sdw); + if (ret < 0) + return ret; + hda_sdw_int_enable(sdev, true); + + return ret; +} These 4 functions sdw_intel_acpi_scan, sdw_intel_probe, sdw_intel_startup and sdw_intel_exit are the interface between the ASoC world and the Soundwire/Intel module. I split the patches in two series to make the review and integration easier on maintainers. The first one is strictly contained within the driver/soundwire directory while will impact the soundwire and ASoC trees.
On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: > On 9/17/19 12:55 AM, Greg KH wrote: > > On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: > > > +/** > > > + * sdw_intel_probe() - SoundWire Intel probe routine > > > + * @parent_handle: ACPI parent handle > > > + * @res: resource data > > > + * > > > + * This creates SoundWire Master and Slave devices below the controller. > > > + * All the information necessary is stored in the context, and the res > > > + * argument pointer can be freed after this step. > > > + */ > > > +struct sdw_intel_ctx > > > +*sdw_intel_probe(struct sdw_intel_res *res) > > > +{ > > > + return sdw_intel_probe_controller(res); > > > +} > > > +EXPORT_SYMBOL(sdw_intel_probe); > > > + > > > +/** > > > + * sdw_intel_startup() - SoundWire Intel startup > > > + * @ctx: SoundWire context allocated in the probe > > > + * > > > + */ > > > +int sdw_intel_startup(struct sdw_intel_ctx *ctx) > > > +{ > > > + return sdw_intel_startup_controller(ctx); > > > +} > > > +EXPORT_SYMBOL(sdw_intel_startup); > > > > Why are you exporting these functions if no one calls them? > > They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: > add SoundWire configuration interface' That wasn't obvious :) Also, why not EXPORT_SYMBOL_GPL()? :) thanks, greg k-h
On 9/18/19 7:06 AM, Greg KH wrote: > On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: >> On 9/17/19 12:55 AM, Greg KH wrote: >>> On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: >>>> +/** >>>> + * sdw_intel_probe() - SoundWire Intel probe routine >>>> + * @parent_handle: ACPI parent handle >>>> + * @res: resource data >>>> + * >>>> + * This creates SoundWire Master and Slave devices below the controller. >>>> + * All the information necessary is stored in the context, and the res >>>> + * argument pointer can be freed after this step. >>>> + */ >>>> +struct sdw_intel_ctx >>>> +*sdw_intel_probe(struct sdw_intel_res *res) >>>> +{ >>>> + return sdw_intel_probe_controller(res); >>>> +} >>>> +EXPORT_SYMBOL(sdw_intel_probe); >>>> + >>>> +/** >>>> + * sdw_intel_startup() - SoundWire Intel startup >>>> + * @ctx: SoundWire context allocated in the probe >>>> + * >>>> + */ >>>> +int sdw_intel_startup(struct sdw_intel_ctx *ctx) >>>> +{ >>>> + return sdw_intel_startup_controller(ctx); >>>> +} >>>> +EXPORT_SYMBOL(sdw_intel_startup); >>> >>> Why are you exporting these functions if no one calls them? >> >> They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: >> add SoundWire configuration interface' > > That wasn't obvious :) > > Also, why not EXPORT_SYMBOL_GPL()? :) Since the beginning of this SoundWire work, the intent what that the code could be reused in non-GPL open-source circles, hence the dual license and EXPORT_SYMBOL. That said, there are cases where the code only makes sense for Linux, or relies on symbols that are exported with EXPORT_SYMBOL_GPL, in those cases we rely on GPLv2 and EXPORT_SYMBOL_GPL. For this series I added a disclaimer in the cover letter that those parts need to be reviewed further to make sure there are no conflicts with GPL. This is an RFC-level contribution to check if my understanding of the bus/device/driver model is aligned with recommendations. I've already made local improvements by fixing bisect issues, removing warnings, improved some sequences, and that GPL question will be revisited before I send a formal patch.
On Wed, Sep 18, 2019 at 08:48:33AM -0500, Pierre-Louis Bossart wrote: > On 9/18/19 7:06 AM, Greg KH wrote: > > On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: > > > On 9/17/19 12:55 AM, Greg KH wrote: > > > > On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: > > > > > +/** > > > > > + * sdw_intel_probe() - SoundWire Intel probe routine > > > > > + * @parent_handle: ACPI parent handle > > > > > + * @res: resource data > > > > > + * > > > > > + * This creates SoundWire Master and Slave devices below the controller. > > > > > + * All the information necessary is stored in the context, and the res > > > > > + * argument pointer can be freed after this step. > > > > > + */ > > > > > +struct sdw_intel_ctx > > > > > +*sdw_intel_probe(struct sdw_intel_res *res) > > > > > +{ > > > > > + return sdw_intel_probe_controller(res); > > > > > +} > > > > > +EXPORT_SYMBOL(sdw_intel_probe); > > > > > + > > > > > +/** > > > > > + * sdw_intel_startup() - SoundWire Intel startup > > > > > + * @ctx: SoundWire context allocated in the probe > > > > > + * > > > > > + */ > > > > > +int sdw_intel_startup(struct sdw_intel_ctx *ctx) > > > > > +{ > > > > > + return sdw_intel_startup_controller(ctx); > > > > > +} > > > > > +EXPORT_SYMBOL(sdw_intel_startup); > > > > > > > > Why are you exporting these functions if no one calls them? > > > > > > They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: > > > add SoundWire configuration interface' > > > > That wasn't obvious :) > > > > Also, why not EXPORT_SYMBOL_GPL()? :) > > Since the beginning of this SoundWire work, the intent what that the code > could be reused in non-GPL open-source circles, hence the dual license and > EXPORT_SYMBOL. Hah, you _have_ talked to your lawyers about this, right? You have a chance to do something like this for header files, for .c files, good luck. That's going to be a hard road to go down. Many have tried in the past, all but 1 have failed. > That said, there are cases where the code only makes sense for Linux, or > relies on symbols that are exported with EXPORT_SYMBOL_GPL, in those cases > we rely on GPLv2 and EXPORT_SYMBOL_GPL. For this series I added a disclaimer > in the cover letter that those parts need to be reviewed further to make > sure there are no conflicts with GPL. Please do that with your lawyers, do not require developers to do legal work for you, that's just mean :( thanks, greg k-h
On Wed, Sep 18, 2019 at 03:53:02PM +0200, Greg KH wrote: > On Wed, Sep 18, 2019 at 08:48:33AM -0500, Pierre-Louis Bossart wrote: > > On 9/18/19 7:06 AM, Greg KH wrote: > > > On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: > > > > On 9/17/19 12:55 AM, Greg KH wrote: > > > > > On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: > > > > > > +/** > > > > > > + * sdw_intel_probe() - SoundWire Intel probe routine > > > > > > + * @parent_handle: ACPI parent handle > > > > > > + * @res: resource data > > > > > > + * > > > > > > + * This creates SoundWire Master and Slave devices below the controller. > > > > > > + * All the information necessary is stored in the context, and the res > > > > > > + * argument pointer can be freed after this step. > > > > > > + */ > > > > > > +struct sdw_intel_ctx > > > > > > +*sdw_intel_probe(struct sdw_intel_res *res) > > > > > > +{ > > > > > > + return sdw_intel_probe_controller(res); > > > > > > +} > > > > > > +EXPORT_SYMBOL(sdw_intel_probe); > > > > > > + > > > > > > +/** > > > > > > + * sdw_intel_startup() - SoundWire Intel startup > > > > > > + * @ctx: SoundWire context allocated in the probe > > > > > > + * > > > > > > + */ > > > > > > +int sdw_intel_startup(struct sdw_intel_ctx *ctx) > > > > > > +{ > > > > > > + return sdw_intel_startup_controller(ctx); > > > > > > +} > > > > > > +EXPORT_SYMBOL(sdw_intel_startup); > > > > > > > > > > Why are you exporting these functions if no one calls them? > > > > > > > > They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: > > > > add SoundWire configuration interface' > > > > > > That wasn't obvious :) > > > > > > Also, why not EXPORT_SYMBOL_GPL()? :) > > > > Since the beginning of this SoundWire work, the intent what that the code > > could be reused in non-GPL open-source circles, hence the dual license and > > EXPORT_SYMBOL. > > Hah, you _have_ talked to your lawyers about this, right? > > You have a chance to do something like this for header files, for .c > files, good luck. That's going to be a hard road to go down. Many have > tried in the past, all but 1 have failed. Also note, the last I checked, the _default_ license for Linux kernel code from Intel was GPLv2. If you got an exception for this, please work with your legal council on how to do this "properly" as that was part of getting that exception, right? If you didn't get the exception, um, you have some people to go talk to, and how come I am the one asking you about this? :( thanks, greg k-h
On 9/18/19 8:54 AM, Greg KH wrote: > On Wed, Sep 18, 2019 at 03:53:02PM +0200, Greg KH wrote: >> On Wed, Sep 18, 2019 at 08:48:33AM -0500, Pierre-Louis Bossart wrote: >>> On 9/18/19 7:06 AM, Greg KH wrote: >>>> On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: >>>>> On 9/17/19 12:55 AM, Greg KH wrote: >>>>>> On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: >>>>>>> +/** >>>>>>> + * sdw_intel_probe() - SoundWire Intel probe routine >>>>>>> + * @parent_handle: ACPI parent handle >>>>>>> + * @res: resource data >>>>>>> + * >>>>>>> + * This creates SoundWire Master and Slave devices below the controller. >>>>>>> + * All the information necessary is stored in the context, and the res >>>>>>> + * argument pointer can be freed after this step. >>>>>>> + */ >>>>>>> +struct sdw_intel_ctx >>>>>>> +*sdw_intel_probe(struct sdw_intel_res *res) >>>>>>> +{ >>>>>>> + return sdw_intel_probe_controller(res); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(sdw_intel_probe); >>>>>>> + >>>>>>> +/** >>>>>>> + * sdw_intel_startup() - SoundWire Intel startup >>>>>>> + * @ctx: SoundWire context allocated in the probe >>>>>>> + * >>>>>>> + */ >>>>>>> +int sdw_intel_startup(struct sdw_intel_ctx *ctx) >>>>>>> +{ >>>>>>> + return sdw_intel_startup_controller(ctx); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(sdw_intel_startup); >>>>>> >>>>>> Why are you exporting these functions if no one calls them? >>>>> >>>>> They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: >>>>> add SoundWire configuration interface' >>>> >>>> That wasn't obvious :) >>>> >>>> Also, why not EXPORT_SYMBOL_GPL()? :) >>> >>> Since the beginning of this SoundWire work, the intent what that the code >>> could be reused in non-GPL open-source circles, hence the dual license and >>> EXPORT_SYMBOL. >> >> Hah, you _have_ talked to your lawyers about this, right? >> >> You have a chance to do something like this for header files, for .c >> files, good luck. That's going to be a hard road to go down. Many have >> tried in the past, all but 1 have failed. > > Also note, the last I checked, the _default_ license for Linux kernel > code from Intel was GPLv2. If you got an exception for this, please > work with your legal council on how to do this "properly" as that was > part of getting that exception, right? > > If you didn't get the exception, um, you have some people to go talk to, > and how come I am the one asking you about this? :( All the legal due-diligence was done when SoundWire was initially contributed in 2018. You asked that question at the time and I will point you to the email exchange Alan Cox and you had on this topic [1]. [1] https://patchwork.kernel.org/patch/10015813/
On Wed, Sep 18, 2019 at 10:14:51AM -0500, Pierre-Louis Bossart wrote: > On 9/18/19 8:54 AM, Greg KH wrote: > > On Wed, Sep 18, 2019 at 03:53:02PM +0200, Greg KH wrote: > > > On Wed, Sep 18, 2019 at 08:48:33AM -0500, Pierre-Louis Bossart wrote: > > > > On 9/18/19 7:06 AM, Greg KH wrote: > > > > > On Tue, Sep 17, 2019 at 09:29:52AM -0500, Pierre-Louis Bossart wrote: > > > > > > On 9/17/19 12:55 AM, Greg KH wrote: > > > > > > > On Mon, Sep 16, 2019 at 04:23:41PM -0500, Pierre-Louis Bossart wrote: > > > > > > > > +/** > > > > > > > > + * sdw_intel_probe() - SoundWire Intel probe routine > > > > > > > > + * @parent_handle: ACPI parent handle > > > > > > > > + * @res: resource data > > > > > > > > + * > > > > > > > > + * This creates SoundWire Master and Slave devices below the controller. > > > > > > > > + * All the information necessary is stored in the context, and the res > > > > > > > > + * argument pointer can be freed after this step. > > > > > > > > + */ > > > > > > > > +struct sdw_intel_ctx > > > > > > > > +*sdw_intel_probe(struct sdw_intel_res *res) > > > > > > > > +{ > > > > > > > > + return sdw_intel_probe_controller(res); > > > > > > > > +} > > > > > > > > +EXPORT_SYMBOL(sdw_intel_probe); > > > > > > > > + > > > > > > > > +/** > > > > > > > > + * sdw_intel_startup() - SoundWire Intel startup > > > > > > > > + * @ctx: SoundWire context allocated in the probe > > > > > > > > + * > > > > > > > > + */ > > > > > > > > +int sdw_intel_startup(struct sdw_intel_ctx *ctx) > > > > > > > > +{ > > > > > > > > + return sdw_intel_startup_controller(ctx); > > > > > > > > +} > > > > > > > > +EXPORT_SYMBOL(sdw_intel_startup); > > > > > > > > > > > > > > Why are you exporting these functions if no one calls them? > > > > > > > > > > > > They are used in the next series, see '[RFC PATCH 04/12] ASoC: SOF: Intel: > > > > > > add SoundWire configuration interface' > > > > > > > > > > That wasn't obvious :) > > > > > > > > > > Also, why not EXPORT_SYMBOL_GPL()? :) > > > > > > > > Since the beginning of this SoundWire work, the intent what that the code > > > > could be reused in non-GPL open-source circles, hence the dual license and > > > > EXPORT_SYMBOL. > > > > > > Hah, you _have_ talked to your lawyers about this, right? > > > > > > You have a chance to do something like this for header files, for .c > > > files, good luck. That's going to be a hard road to go down. Many have > > > tried in the past, all but 1 have failed. > > > > Also note, the last I checked, the _default_ license for Linux kernel > > code from Intel was GPLv2. If you got an exception for this, please > > work with your legal council on how to do this "properly" as that was > > part of getting that exception, right? > > > > If you didn't get the exception, um, you have some people to go talk to, > > and how come I am the one asking you about this? :( > > All the legal due-diligence was done when SoundWire was initially > contributed in 2018. You asked that question at the time and I will point > you to the email exchange Alan Cox and you had on this topic [1]. > > [1] https://patchwork.kernel.org/patch/10015813/ Yes, that is fine, what I am saying here is that you are now asking the community to do this for you. You said in this thread: > For this series I added a disclaimer in the cover letter that those > parts need to be reviewed further to make sure there are no conflicts > with GPL. Why are you sending code out that you think might have conflicts before your lawyers have reviewed it? That's just screaming for problems in the future (hint, you distributed something in the previous emails...) Again, go and get this sorted out before dumping that kind of work on the community as this is something that you are having to deal with (i.e. it is self-inflicted). Don't make others do this for you here in public. Otherwise I will probably just purposefully tell you the wrong thing, and then watch what kind of fun your lawyers will have :) thanks, greg k-h
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 8a1f6c627788..267e0fad7494 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -92,8 +92,6 @@ #define SDW_ALH_STRMZCFG_DMAT GENMASK(7, 0) #define SDW_ALH_STRMZCFG_CHN GENMASK(19, 16) -#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) - enum intel_pdi_type { INTEL_PDI_IN = 0, INTEL_PDI_OUT = 1, @@ -909,24 +907,23 @@ static int intel_init(struct sdw_intel *sdw) /* * probe and init */ -static int intel_probe(struct platform_device *pdev) +static int intel_master_probe(struct sdw_master_device *md, void *link_ctx) { - struct sdw_cdns_stream_config config; struct sdw_intel *sdw; int ret; - sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL); + sdw = devm_kzalloc(&md->dev, sizeof(*sdw), GFP_KERNEL); if (!sdw) return -ENOMEM; - sdw->instance = pdev->id; - sdw->link_res = dev_get_platdata(&pdev->dev); - sdw->cdns.dev = &pdev->dev; + sdw->instance = md->link_id; + sdw->link_res = link_ctx; + sdw->cdns.dev = &md->dev; sdw->cdns.registers = sdw->link_res->registers; - sdw->cdns.instance = sdw->instance; + sdw->cdns.instance = md->link_id; sdw->cdns.msg_count = 0; - sdw->cdns.bus.dev = &pdev->dev; - sdw->cdns.bus.link_id = pdev->id; + sdw->cdns.bus.dev = &md->dev; + sdw->cdns.bus.link_id = md->link_id; sdw_cdns_probe(&sdw->cdns); @@ -934,16 +931,50 @@ static int intel_probe(struct platform_device *pdev) sdw_intel_ops.read_prop = intel_prop_read; sdw->cdns.bus.ops = &sdw_intel_ops; - platform_set_drvdata(pdev, sdw); + md->pdata = sdw; + + /* set driver data, accessed by snd_soc_dai_set_drvdata() */ + dev_set_drvdata(&md->dev, &sdw->cdns); ret = sdw_add_bus_master(&sdw->cdns.bus); if (ret) { - dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret); + dev_err(&md->dev, "sdw_add_bus_master fail: %d\n", ret); return ret; } if (sdw->cdns.bus.prop.hw_disabled) { - dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n", + dev_info(&md->dev, "SoundWire master %d is disabled, ignoring\n", + sdw->cdns.bus.link_id); + return 0; + } + + /* Acquire IRQ */ + ret = request_threaded_irq(sdw->link_res->irq, + sdw_cdns_irq, sdw_cdns_thread, + IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); + if (ret < 0) { + dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", + sdw->link_res->irq); + goto err_init; + } + + return 0; + +err_init: + sdw_delete_bus_master(&sdw->cdns.bus); + return ret; +} + +static int intel_master_startup(struct sdw_master_device *md) +{ + struct sdw_cdns_stream_config config; + struct sdw_intel *sdw; + int ret; + + sdw = md->pdata; + + if (sdw->cdns.bus.prop.hw_disabled) { + dev_info(&md->dev, "SoundWire master %d is disabled, ignoring\n", sdw->cdns.bus.link_id); return 0; } @@ -961,16 +992,6 @@ static int intel_probe(struct platform_device *pdev) intel_pdi_ch_update(sdw); - /* Acquire IRQ */ - ret = request_threaded_irq(sdw->link_res->irq, - sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); - if (ret < 0) { - dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", - sdw->link_res->irq); - goto err_init; - } - ret = sdw_cdns_enable_interrupt(&sdw->cdns, true); if (ret < 0) { dev_err(sdw->cdns.dev, "cannot enable interrupts\n"); @@ -997,18 +1018,17 @@ static int intel_probe(struct platform_device *pdev) err_interrupt: sdw_cdns_enable_interrupt(&sdw->cdns, false); -err_dai: - free_irq(sdw->link_res->irq, sdw); err_init: + free_irq(sdw->link_res->irq, sdw); sdw_delete_bus_master(&sdw->cdns.bus); return ret; } -static int intel_remove(struct platform_device *pdev) +static int intel_master_remove(struct sdw_master_device *md) { struct sdw_intel *sdw; - sdw = platform_get_drvdata(pdev); + sdw = md->pdata; if (!sdw->cdns.bus.prop.hw_disabled) { intel_debugfs_exit(sdw); @@ -1021,16 +1041,12 @@ static int intel_remove(struct platform_device *pdev) return 0; } -static struct platform_driver sdw_intel_drv = { - .probe = intel_probe, - .remove = intel_remove, - .driver = { - .name = "int-sdw", - - }, +struct sdw_md_driver intel_sdw_driver = { + .probe = intel_master_probe, + .startup = intel_master_startup, + .remove = intel_master_remove, }; - -module_platform_driver(sdw_intel_drv); +EXPORT_SYMBOL(intel_sdw_driver); MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS("platform:int-sdw"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index d923b6262330..25cc51e15ef5 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -4,24 +4,8 @@ #ifndef __SDW_INTEL_LOCAL_H #define __SDW_INTEL_LOCAL_H -/** - * struct sdw_intel_link_res - Soundwire link resources - * @registers: Link IO registers base - * @shim: Audio shim pointer - * @alh: ALH (Audio Link Hub) pointer - * @irq: Interrupt line - * @ops: Shim callback ops - * @arg: Shim callback ops argument - * - * This is set as pdata for each link instance. - */ -struct sdw_intel_link_res { - void __iomem *registers; - void __iomem *shim; - void __iomem *alh; - int irq; - const struct sdw_intel_ops *ops; - void *arg; -}; +#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) + +extern struct sdw_md_driver intel_sdw_driver; #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index d488c44fcbae..47124fc13a4a 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -11,7 +11,7 @@ #include <linux/export.h> #include <linux/iomap.h> #include <linux/module.h> -#include <linux/platform_device.h> +#include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_intel.h> #include "intel.h" @@ -27,28 +27,45 @@ static int link_mask; module_param_named(sdw_link_mask, link_mask, int, 0444); MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)"); -struct sdw_link_data { - struct sdw_intel_link_res res; - struct platform_device *pdev; -}; +static bool is_link_enabled(struct fwnode_handle *fw_node, int i) +{ + struct fwnode_handle *link; + char name[32]; + u32 quirk_mask = 0; -struct sdw_intel_ctx { - int count; - struct sdw_link_data *links; -}; + /* Find master handle */ + snprintf(name, sizeof(name), + "mipi-sdw-link-%d-subproperties", i); + + link = fwnode_get_named_child_node(fw_node, name); + if (!link) + return false; -static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) + fwnode_property_read_u32(link, + "intel-quirk-mask", + &quirk_mask); + + if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE) + return false; + + return true; +} + +static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) { - struct sdw_link_data *link = ctx->links; + struct sdw_intel_link_res *link = ctx->links; + struct sdw_master_device *md; int i; if (!link) return 0; - for (i = 0; i < ctx->count; i++) { - if (link->pdev) - platform_device_unregister(link->pdev); - link++; + for (i = 0; i < ctx->count; i++, link++) { + md = link->md; + if (md) { + md->driver->remove(md); + put_device(&md->dev); + } } kfree(ctx->links); @@ -57,115 +74,187 @@ static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) return 0; } -static struct sdw_intel_ctx -*sdw_intel_add_controller(struct sdw_intel_res *res) +static int +sdw_intel_scan_controller(struct sdw_intel_acpi_info *info) { - struct platform_device_info pdevinfo; - struct platform_device *pdev; - struct sdw_link_data *link; - struct sdw_intel_ctx *ctx; struct acpi_device *adev; int ret, i; u8 count; - u32 caps; - if (acpi_bus_get_device(res->handle, &adev)) - return NULL; + if (acpi_bus_get_device(info->handle, &adev)) + return -EINVAL; /* Found controller, find links supported */ count = 0; ret = fwnode_property_read_u8_array(acpi_fwnode_handle(adev), "mipi-sdw-master-count", &count, 1); - /* Don't fail on error, continue and use hw value */ + /* + * In theory we could check the number of links supported in + * hardware, but in that step we cannot assume SoundWire IP is + * powered. + * + * In addition, if the BIOS doesn't even provide this + * 'master-count' property then all the inits based on link + * masks will fail as well. + * + * We will check the hardware capabilities in the startup() step + */ + if (ret) { dev_err(&adev->dev, "Failed to read mipi-sdw-master-count: %d\n", ret); - count = SDW_MAX_LINKS; + return -EINVAL; } - /* Check SNDWLCAP.LCOUNT */ - caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); - caps &= GENMASK(2, 0); - - /* Check HW supported vs property value and use min of two */ - count = min_t(u8, caps, count); - /* Check count is within bounds */ if (count > SDW_MAX_LINKS) { dev_err(&adev->dev, "Link count %d exceeds max %d\n", count, SDW_MAX_LINKS); - return NULL; + return -EINVAL; } else if (!count) { dev_warn(&adev->dev, "No SoundWire links detected\n"); - return NULL; + return -EINVAL; } + dev_dbg(&adev->dev, "Detected %d SDW Link devices\n", count); + info->count = count; + + 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); + continue; + } + + if (!is_link_enabled(acpi_fwnode_handle(adev), i)) + continue; + + info->link_mask |= BIT(i); + } + + return 0; +} + +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 (acpi_bus_get_device(res->handle, &adev)) + return NULL; + + if (!res || !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; - } - link->res.irq = res->irq; - link->res.registers = res->mmio_base + SDW_LINK_BASE - + (SDW_LINK_SIZE * i); - link->res.shim = res->mmio_base + SDW_SHIM_BASE; - link->res.alh = res->mmio_base + SDW_ALH_BASE; - - link->res.ops = res->ops; - link->res.arg = res->arg; - - memset(&pdevinfo, 0, sizeof(pdevinfo)); - - pdevinfo.parent = res->parent; - pdevinfo.name = "int-sdw"; - pdevinfo.id = i; - pdevinfo.fwnode = acpi_fwnode_handle(adev); - pdevinfo.data = &link->res; - pdevinfo.size_data = sizeof(link->res); - - 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; - } + md = sdw_md_add(&intel_sdw_driver, + &adev->dev, + acpi_fwnode_handle(adev), + i); - link->pdev = pdev; - link++; + 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); + 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->arg = res->arg; + + /* let the SoundWire master driver to its probe */ + md->driver->probe(md, link); } return ctx; -pdev_err: - sdw_intel_cleanup_pdev(ctx); +err: + sdw_intel_cleanup(ctx); link_err: kfree(ctx); return NULL; } +static int +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) +{ + struct acpi_device *adev; + struct sdw_intel_link_res *link; + struct sdw_master_device *md; + u32 caps; + int i; + + if (acpi_bus_get_device(ctx->handle, &adev)) + return -EINVAL; + + /* Check SNDWLCAP.LCOUNT */ + caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); + caps &= GENMASK(2, 0); + + /* Check HW supported vs property value */ + if (caps < ctx->count) { + dev_err(&adev->dev, + "BIOS master count is larger than hardware capabilities\n"); + return -EINVAL; + } + + if (!ctx->links) + return -EINVAL; + + link = ctx->links; + link_mask = ctx->link_mask; + + /* Create SDW Master devices */ + for (i = 0; i < ctx->count; i++, link++) { + if (link_mask && !(link_mask & BIT(i))) + continue; + + md = link->md; + + md->driver->startup(md); + } + + return 0; +} + static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, void *cdata, void **return_value) { - struct sdw_intel_res *res = cdata; + struct sdw_intel_acpi_info *info = cdata; struct acpi_device *adev; acpi_status status; u64 adr; @@ -179,7 +268,7 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, return AE_NOT_FOUND; } - res->handle = handle; + info->handle = handle; /* * On some Intel platforms, multiple children of the HDAS @@ -196,40 +285,68 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, } /** - * sdw_intel_init() - SoundWire Intel init routine + * sdw_intel_acpi_scan() - SoundWire Intel init routine * @parent_handle: ACPI parent handle - * @res: resource data + * @info: description of what firmware/DSDT tables expose * - * This scans the namespace and creates SoundWire link controller devices - * based on the info queried. + * This scans the namespace and queries firmware to figure out which + * links to enable. A follow-up use of sdw_intel_probe() and + * sdw_intel_startup() is required for creation of devices and bus + * startup */ -void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res) +int sdw_intel_acpi_scan(acpi_handle *parent_handle, + struct sdw_intel_acpi_info *info) { acpi_status status; status = acpi_walk_namespace(ACPI_TYPE_DEVICE, parent_handle, 1, sdw_intel_acpi_cb, - NULL, res, NULL); + NULL, info, NULL); if (ACPI_FAILURE(status)) - return NULL; + return -ENODEV; - return sdw_intel_add_controller(res); + return sdw_intel_scan_controller(info); } -EXPORT_SYMBOL(sdw_intel_init); +EXPORT_SYMBOL(sdw_intel_acpi_scan); +/** + * sdw_intel_probe() - SoundWire Intel probe routine + * @parent_handle: ACPI parent handle + * @res: resource data + * + * This creates SoundWire Master and Slave devices below the controller. + * All the information necessary is stored in the context, and the res + * argument pointer can be freed after this step. + */ +struct sdw_intel_ctx +*sdw_intel_probe(struct sdw_intel_res *res) +{ + return sdw_intel_probe_controller(res); +} +EXPORT_SYMBOL(sdw_intel_probe); + +/** + * sdw_intel_startup() - SoundWire Intel startup + * @ctx: SoundWire context allocated in the probe + * + */ +int sdw_intel_startup(struct sdw_intel_ctx *ctx) +{ + return sdw_intel_startup_controller(ctx); +} +EXPORT_SYMBOL(sdw_intel_startup); /** * sdw_intel_exit() - SoundWire Intel exit - * @arg: callback context + * @ctx: SoundWire context allocated in the probe * * Delete the controller instances created and cleanup */ -void sdw_intel_exit(void *arg) +void sdw_intel_exit(struct sdw_intel_ctx *ctx) { - struct sdw_intel_ctx *ctx = arg; - - sdw_intel_cleanup_pdev(ctx); + sdw_intel_cleanup(ctx); kfree(ctx); + ctx = NULL; } EXPORT_SYMBOL(sdw_intel_exit); diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index c9427cb6020b..0ce3e4023074 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -16,24 +16,104 @@ struct sdw_intel_ops { }; /** - * struct sdw_intel_res - Soundwire Intel resource structure + * struct sdw_intel_acpi_info - Soundwire Intel information found in ACPI tables + * @handle: ACPI controller handle + * @count: link count found with "sdw-master-count" property + * @link_mask: bit-wise mask listing links enabled by BIOS menu + * + * this structure could be expanded to e.g. provide all the _ADR + * information in case the link_mask is not sufficient to identify + * platform capabilities. + */ +struct sdw_intel_acpi_info { + acpi_handle handle; + int count; + u32 link_mask; +}; + +/** + * struct sdw_intel_res - Soundwire Intel global resource structure, + * typically populated by the DSP driver + * + * @count: link count (may be filtered by DSP driver) * @mmio_base: mmio base of SoundWire registers * @irq: interrupt number * @handle: ACPI parent handle * @parent: parent device * @ops: callback ops * @arg: callback arg + * @link_mask: bit-wise mask listing links selected by the DSP driver */ struct sdw_intel_res { + int count; void __iomem *mmio_base; int irq; acpi_handle handle; struct device *parent; const struct sdw_intel_ops *ops; void *arg; + u32 link_mask; }; -void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res); -void sdw_intel_exit(void *arg); +/** + * struct sdw_intel_link_res - Soundwire Intel link resource structure, + * typically populated by the controller driver. + * @md: Master device + * @mmio_base: mmio base of SoundWire registers + * @registers: Link IO registers base + * @shim: Audio shim pointer + * @alh: ALH (Audio Link Hub) pointer + * @irq: Interrupt line + * @ops: Shim callback ops + * @arg: Shim callback ops argument + */ +struct sdw_intel_link_res { + struct sdw_master_device *md; + void __iomem *mmio_base; /* not strictly needed, useful for debug */ + void __iomem *registers; + void __iomem *shim; + void __iomem *alh; + int irq; + const struct sdw_intel_ops *ops; + void *arg; +}; + +/** + * struct sdw_intel_ctx - context allocated by the controller + * driver probe + * @count: link count + * @mmio_base: mmio base of SoundWire registers, only used to check + * hardware capabilities after all power dependencies are settled. + * @arg: Shim callback ops argument + */ +struct sdw_intel_ctx { + int count; + void __iomem *mmio_base; + u32 link_mask; + acpi_handle handle; + struct sdw_intel_link_res *links; +}; + +/* + * On Intel platforms, the SoundWire IP has dependencies on power + * rails shared with the DSP, and the initialization steps are split + * in three. First an ACPI scan to check what the firmware describes + * in DSDT tables, then an allocation step (with no hardware + * configuration but with all the relevant devices created) and last + * the actual hardware configuration. The final stage is a global + * interrupt enable which is controlled by the DSP driver. Splitting + * these phases helps simplify the boot flow and make early decisions + * on e.g. which machine driver to select (I2S mode, HDaudio or + * SoundWire). + */ +int sdw_intel_acpi_scan(acpi_handle *parent_handle, + struct sdw_intel_acpi_info *info); + +struct sdw_intel_ctx * +sdw_intel_probe(struct sdw_intel_res *res); + +int sdw_intel_startup(struct sdw_intel_ctx *ctx); + +void sdw_intel_exit(struct sdw_intel_ctx *ctx); #endif
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 make it easier to verify support and allocate required resources as early as possible, and conversely help make the startup lighter-weight with only hardware register setup. The data structures are also consolidated in a single file and comments added to help follow what is used for what. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/intel.c | 90 +++++---- drivers/soundwire/intel.h | 22 +-- drivers/soundwire/intel_init.c | 293 +++++++++++++++++++--------- include/linux/soundwire/sdw_intel.h | 86 +++++++- 4 files changed, 344 insertions(+), 147 deletions(-)