ASoC: cs35l56: Switch to using the IRQ from the SoundWire core
Subsystems (4)
Richard Fitzgerald
maintainercover ASoC: cs35l56: Switch to using the IRQ from the SoundWire core
At the time the cs35l56 driver was written the only way to get interrupts from the SoundWire core was to implement a custom handler inside the interrupt_callback() function.
The SoundWire core now provides a virtual IRQ for notifying ImpDef interrupts, and switching to this simplifies the code and also makes it more similar to the normal interrupt handling of I2C/SPI (though some SoundWire specials are still needed).
Patches #1 and #2 do some preparatory code shuffling so that there is less clutter in patch #4.
Patch #3 changes the SoundWire core code to create the virtual ImpDef IRQ before calling the codec drive probe() so that the IRQ can be requested in probe().
Richard Fitzgerald (4):
ASoC: cs35l56: Request IRQ in cs35l56_common_probe()
ASoC: cs35l56: Move cs35l56_irq_request() after cs35l56_irq()
soundwire: bus_type: Create IRQ mapping before calling driver probe()
ASoC: cs35l56: Use IRQ provided by the SoundWire core
drivers/soundwire/bus_type.c | 6 +-- include/sound/cs35l56.h | 1 - sound/soc/codecs/Kconfig | 1 + sound/soc/codecs/cs35l56-i2c.c | 10 +---- sound/soc/codecs/cs35l56-sdw.c | 69 ++++++------------------------- sound/soc/codecs/cs35l56-shared.c | 41 +++++++++--------- sound/soc/codecs/cs35l56-spi.c | 10 +---- sound/soc/codecs/cs35l56.c | 57 +++++++++++++------------ sound/soc/codecs/cs35l56.h | 11 ++--- 9 files changed, 73 insertions(+), 133 deletions(-)
Signature · 1 line
2.47.3
Richard Fitzgerald
maintainerPATCH 1/4 ASoC: cs35l56: Request IRQ in cs35l56_common_probe()
Call cs35l56_irq_request() in cs35l56_common_probe() instead of calling it afterwards in the probe() for each bus type.
Calling cs35l56_irq_request() in each bus probe() is a legacy of dealing with the oddities of the SoundWire framework. It's no longer serving any useful purpose to do it outside of the main cs35l56_common_probe().
--- sound/soc/codecs/cs35l56-i2c.c | 10 +--------- sound/soc/codecs/cs35l56-sdw.c | 6 +----- sound/soc/codecs/cs35l56-spi.c | 10 +--------- sound/soc/codecs/cs35l56.c | 12 ++++++++++-- sound/soc/codecs/cs35l56.h | 2 +- 5 files changed, 14 insertions(+), 26 deletions(-)
sound/soc/codecs/cs35l56-i2c.c +1 -9
@@ -51,15 +51,7 @@ static int cs35l56_i2c_probe(struct i2c_client *client)return dev_err_probe (cs35l56->base.dev, ret,"Failed to allocate register map\n" ); } - ret =cs35l56_common_probe (cs35l56); -if (ret !=0 ) -return ret; - - ret =cs35l56_irq_request (&cs35l56->base, client->irq); -if (ret <0 ) -cs35l56_remove (cs35l56); - -return ret; +return cs35l56_common_probe (cs35l56, client->irq); }static void cs35l56_i2c_remove (struct i2c_client *client)
sound/soc/codecs/cs35l56-sdw.c +1 -5
@@ -484,11 +484,7 @@ static int cs35l56_sdw_probe(struct sdw_slave *peripheral, const struct sdw_devi/* Start in cache-only until device is enumerated */ regcache_cache_only (cs35l56->base.regmap, true); - ret =cs35l56_common_probe (cs35l56); -if (ret !=0 ) -return ret; - -return 0 ; +return cs35l56_common_probe (cs35l56, -EINVAL ); }static void cs35l56_sdw_remove (struct sdw_slave *peripheral)
sound/soc/codecs/cs35l56-spi.c +1 -9
@@ -40,15 +40,7 @@ static int cs35l56_spi_probe(struct spi_device *spi)if (ret)return ret; - ret =cs35l56_common_probe (cs35l56); -if (ret !=0 ) -return ret; - - ret =cs35l56_irq_request (&cs35l56->base, spi->irq); -if (ret <0 ) -cs35l56_remove (cs35l56); - -return ret; +return cs35l56_common_probe (cs35l56, spi->irq); }static void cs35l56_spi_remove (struct spi_device *spi)
sound/soc/codecs/cs35l56.c +10 -2
@@ -1942,7 +1942,7 @@ static int cs35l56_try_get_broken_sdca_spkid_gpio(struct cs35l56_private *cs35l5return ret; } -int cs35l56_common_probe (struct cs35l56_private *cs35l56) +int cs35l56_common_probe (struct cs35l56_private *cs35l56,int irq) {int ret;
@@ -2019,16 +2019,24 @@ int cs35l56_common_probe(struct cs35l56_private *cs35l56) goto err_remove_wm_adsp; } + ret =cs35l56_irq_request (&cs35l56->base, irq); +if (ret) + goto err_remove_wm_adsp; + ret =snd_soc_register_component (cs35l56->base.dev, &soc_component_dev_cs35l56, cs35l56_dai,ARRAY_SIZE (cs35l56_dai));if (ret <0 ) {dev_err_probe (cs35l56->base.dev, ret,"Register codec failed\n" ); - goto err_remove_wm_adsp; + goto err_free_irq; }return 0 ; +err_free_irq: +if (cs35l56->base.irq) +devm_free_irq (cs35l56->base.dev, cs35l56->base.irq, &cs35l56->base); + err_remove_wm_adsp:wm_adsp2_remove (&cs35l56->dsp);
sound/soc/codecs/cs35l56.h +1 -1
@@ -78,7 +78,7 @@ int cs35l56_system_resume_early(struct device *dev);int cs35l56_system_resume (struct device *dev);irqreturn_t cs35l56_irq (int irq,void *data);int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq); -int cs35l56_common_probe (struct cs35l56_private *cs35l56); +int cs35l56_common_probe (struct cs35l56_private *cs35l56,int irq);int cs35l56_init (struct cs35l56_private *cs35l56);void cs35l56_remove (struct cs35l56_private *cs35l56);
Signature · 1 line
2.47.3
Richard Fitzgerald
maintainerPATCH 2/4 ASoC: cs35l56: Move cs35l56_irq_request() after cs35l56_irq()
In reply to Richard Fitzgeraldcs35l56_irq_request() references cs35l56_irq() but was above it in the source (although they are in the other order in the header file). Switch to convertional C ordering.
This is preparation for a future patch that will stop exporting cs35l56_irq() and make it static.
--- sound/soc/codecs/cs35l56-shared.c | 38 +++++++++++++++---------------- 1 file changed, 19 insertions(+), 19 deletions(-)
sound/soc/codecs/cs35l56-shared.c +19 -19
@@ -616,25 +616,6 @@ void cs35l56_system_reset(struct cs35l56_base *cs35l56_base, bool is_soundwire) }EXPORT_SYMBOL_NS_GPL (cs35l56_system_reset,"SND_SOC_CS35L56_SHARED" ); -int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq) -{ -int ret; - -if (irq <1 ) -return 0 ; - - ret =devm_request_threaded_irq (cs35l56_base->dev, irq,NULL , cs35l56_irq, -IRQF_ONESHOT |IRQF_SHARED |IRQF_TRIGGER_LOW , -"cs35l56" , cs35l56_base); -if (!ret) - cs35l56_base->irq = irq; -else -dev_err (cs35l56_base->dev,"Failed to get IRQ: %d\n" , ret); - -return ret; -} -EXPORT_SYMBOL_NS_GPL (cs35l56_irq_request,"SND_SOC_CS35L56_SHARED" ); -irqreturn_t cs35l56_irq (int irq,void *data) {struct cs35l56_base *cs35l56_base = data;
@@ -694,6 +675,25 @@ irqreturn_t cs35l56_irq(int irq, void *data) }EXPORT_SYMBOL_NS_GPL (cs35l56_irq,"SND_SOC_CS35L56_SHARED" ); +int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq) +{ +int ret; + +if (irq <1 ) +return 0 ; + + ret =devm_request_threaded_irq (cs35l56_base->dev, irq,NULL , cs35l56_irq, +IRQF_ONESHOT |IRQF_SHARED |IRQF_TRIGGER_LOW , +"cs35l56" , cs35l56_base); +if (!ret) + cs35l56_base->irq = irq; +else +dev_err (cs35l56_base->dev,"Failed to get IRQ: %d\n" , ret); + +return ret; +} +EXPORT_SYMBOL_NS_GPL (cs35l56_irq_request,"SND_SOC_CS35L56_SHARED" ); +int cs35l56_is_fw_reload_needed (struct cs35l56_base *cs35l56_base) {unsigned int val;
Signature · 1 line
2.47.3
Richard Fitzgerald
maintainerPATCH 3/4 soundwire: bus_type: Create IRQ mapping before calling driver probe()
In reply to Richard FitzgeraldCall sdw_irq_create_mapping() before calling the peripheral driver probe() so that it is possible to request the IRQ during probe().
Previously creation of the mapping was conditional on the use_domain_irq flag in the driver properties. But these are filled in after probe(), which meant it wasn't possible to request the IRQ during probe(). This was ok for MFD drivers where only children requested the IRQ. But for normal drivers it led to the non-standard behavior of having to defer requesting the IRQ until after probe().
--- drivers/soundwire/bus_type.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/soundwire/bus_type.c +3 -3
@@ -105,6 +105,9 @@ static int sdw_bus_probe(struct device *dev) } slave->index = ret; +/* Create IRQ mapping now so the driver can get it in probe() */ +sdw_irq_create_mapping (slave); + ret = drv->probe (slave, id);if (ret) {ida_free (&slave->bus->slave_ida, slave->index);
@@ -117,9 +120,6 @@ static int sdw_bus_probe(struct device *dev)if (drv->ops && drv->ops->read_prop) drv->ops->read_prop (slave); -if (slave->prop.use_domain_irq) -sdw_irq_create_mapping (slave); -/* init the dynamic sysfs attributes we need */ ret =sdw_slave_sysfs_dpn_init (slave);if (ret <0 )
Signature · 1 line
2.47.3
Vinod Koul
maintainerRe: [PATCH 3/4] soundwire: bus_type: Create IRQ mapping before calling driver probe()
Quoted 9 lines · Richard Fitzgerald
Call
sdw_irq_create_mapping()before calling the peripheral driverprobe()so that it is possible to request the IRQ duringprobe().Previously creation of the mapping was conditional on the
use_domain_irqflag in the driver properties. But these are filled in afterprobe(), which meant it wasn't possible to request the IRQ duringprobe(). This was ok for MFD drivers where only children requested the IRQ. But for normal drivers it led to the non-standard behavior of having to defer requesting the IRQ until afterprobe().
Shouldnt that still be the case (conditionally creating mapping). Does every need this mapping?
Quoted 32 lines
Signed-off-by Richard Fitzgerald <[email protected]>--- drivers/soundwire/bus_type.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/soundwire/bus_type.c +3
-3@@ -105,6 +105,9 @@ static int sdw_bus_probe(struct device *dev) } slave->index = ret; +/* Create IRQ mapping now so the driver can get it in probe() */ +sdw_irq_create_mapping (slave); + ret = drv->probe (slave, id);if (ret) {ida_free (&slave->bus->slave_ida, slave->index);@@ -117,9 +120,6 @@ static int sdw_bus_probe(struct device *dev)if (drv->ops && drv->ops->read_prop) drv->ops->read_prop (slave); -if (slave->prop.use_domain_irq) -sdw_irq_create_mapping (slave); -/* init the dynamic sysfs attributes we need */ ret =sdw_slave_sysfs_dpn_init (slave);if (ret <0 )Signature · 1 line
2.47.3
Signature · 1 line
~Vinod
Richard Fitzgerald
maintainerRe: [PATCH 3/4] soundwire: bus_type: Create IRQ mapping before calling driver probe()
Quoted 11 lines · Vinod Koul
Quoted 9 lines · Richard Fitzgerald
Call
sdw_irq_create_mapping()before calling the peripheral driverprobe()so that it is possible to request the IRQ duringprobe().Previously creation of the mapping was conditional on the
use_domain_irqflag in the driver properties. But these are filled in afterprobe(), which meant it wasn't possible to request the IRQ duringprobe(). This was ok for MFD drivers where only children requested the IRQ. But for normal drivers it led to the non-standard behavior of having to defer requesting the IRQ until afterprobe().
Quoted 2 lines
Shouldnt that still be the case (conditionally creating mapping). Does every need this mapping?
Other interrupt providers create all their IRQs even if nothing uses them, so why worry about one IRQ mapping per SoundWire peripheral?
Currently it's just another way that the SoundWire subsystem doesn't follow normal Linux conventions and requires the codec drivers to work around the strangeness.
We could unmap it after ops->read_prop() if we see use_domain_irq is false.
If we want to skip ever creating it, we'd need to move use_domain_irq to somewhere that is valid _before_ calling the codec driver probe().
Or call ops->read_prop() before calling probe(). There's no explanation why read_prop() was placed after probe() so I'm reluctant to move it. But I assume it's expected that probe() will always be the first function called in a driver.
Re: [PATCH 3/4] soundwire: bus_type: Create IRQ mapping before calling driver probe()
Quoted 15 lines · Richard Fitzgerald
Quoted 11 lines · Vinod Koul
Quoted 9 lines · Richard Fitzgerald
Call
sdw_irq_create_mapping()before calling the peripheral driverprobe()so that it is possible to request the IRQ duringprobe().Previously creation of the mapping was conditional on the
use_domain_irqflag in the driver properties. But these are filled in afterprobe(), which meant it wasn't possible to request the IRQ duringprobe(). This was ok for MFD drivers where only children requested the IRQ. But for normal drivers it led to the non-standard behavior of having to defer requesting the IRQ until afterprobe().Quoted 2 lines
Shouldnt that still be the case (conditionally creating mapping). Does every need this mapping?
Quoted 6 lines
Other interrupt providers create all their IRQs even if nothing uses them, so why worry about one IRQ mapping per SoundWire peripheral?
Currently it's just another way that the SoundWire subsystem doesn't follow normal Linux conventions and requires the codec drivers to work around the strangeness.
Hmm okay lets go with this, so
Quoted 11 lines
We could unmap it after
ops->read_prop()if we seeuse_domain_irqis false.If we want to skip ever creating it, we'd need to move
use_domain_irqto somewhere that is valid _before_ calling the codec driverprobe().Or call
ops->read_prop()before callingprobe(). There's no explanation whyread_prop()was placed afterprobe()so I'm reluctant to move it. But I assume it's expected thatprobe()will always be the first function called in a driver.
The intent was that drivers can override the properties, so after the probe is recommended.
Signature · 1 line
~Vinod
Richard Fitzgerald
maintainerPATCH 4/4 ASoC: cs35l56: Use IRQ provided by the SoundWire core
In reply to Richard FitzgeraldReplace the custom SoundWire IRQ handling with the generic nested IRQ provided by the SoundWire core. This removes the local IRQ work function and the convoluted IRQ masking and pm_runtime management around it.
We still need the local functions to mask/disable and unmask/enable the SoundWire interrupts because the devices handled by the cs35l56 driver don't have the generic mask bit for the ImpDef1 interrupt so masking and unmasking has to use a custom mask bit.
cs35l56_sdw_remove() doesn't need to call cs35l56_disable_sdw_interrupts() now that there isn't a local work function to be flushed. It only masks the custom interrupt mask bit and the rest of the handler cleanup will be done the normal way by devm_free_irq() in cs35l56_remove().
Similar applies to cs35l56_sdw_system_suspend() - it is enough to write the custom mask bits.
cs35l56_irq() doesn't need to be exported because cs35l56_sdw.c isn't calling it.
--- include/sound/cs35l56.h | 1 - sound/soc/codecs/Kconfig | 1 + sound/soc/codecs/cs35l56-sdw.c | 65 +++++++------------------------ sound/soc/codecs/cs35l56-shared.c | 3 +- sound/soc/codecs/cs35l56.c | 45 ++++++++++----------- sound/soc/codecs/cs35l56.h | 9 +---- 6 files changed, 38 insertions(+), 86 deletions(-)
include/sound/cs35l56.h +0 -1
@@ -417,7 +417,6 @@ void cs35l56_wait_control_port_ready(void);void cs35l56_wait_min_reset_pulse (void );void cs35l56_system_reset (struct cs35l56_base *cs35l56_base,bool is_soundwire);int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq); -irqreturn_t cs35l56_irq (int irq,void *data);int cs35l56_is_fw_reload_needed (struct cs35l56_base *cs35l56_base);int cs35l56_runtime_suspend_common (struct cs35l56_base *cs35l56_base);int cs35l56_runtime_resume_common (struct cs35l56_base *cs35l56_base,bool is_soundwire);
sound/soc/codecs/Kconfig +1 -0
@@ -896,6 +896,7 @@ config SND_SOC_CS35L56_SDW tristate "Cirrus Logic CS35L56 CODEC (SDW)" depends on SOUNDWIRE select REGMAP_SOUNDWIRE + select IRQ_DOMAIN select SND_SOC_CS35L56 select SND_SOC_CS35L56_SHARED help
sound/soc/codecs/cs35l56-sdw.c +13 -52
129 lines not on this page · raw message
sound/soc/codecs/cs35l56-shared.c +1 -2
@@ -616,7 +616,7 @@ void cs35l56_system_reset(struct cs35l56_base *cs35l56_base, bool is_soundwire) }EXPORT_SYMBOL_NS_GPL (cs35l56_system_reset,"SND_SOC_CS35L56_SHARED" ); -irqreturn_t cs35l56_irq (int irq,void *data) +static irqreturn_t cs35l56_irq (int irq,void *data) {struct cs35l56_base *cs35l56_base = data;unsigned int status1 =0 , status8 =0 , status20 =0 ;
@@ -673,7 +673,6 @@ irqreturn_t cs35l56_irq(int irq, void *data)return IRQ_HANDLED ; } -EXPORT_SYMBOL_NS_GPL (cs35l56_irq,"SND_SOC_CS35L56_SHARED" );int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq) {
sound/soc/codecs/cs35l56.c +21 -24
80 lines not on this page · raw message
sound/soc/codecs/cs35l56.h +2 -7
@@ -39,8 +39,6 @@ struct cs35l56_private {struct sdw_slave *sdw_peripheral;struct regmap *sdw_bus_regmap;const char *fallback_fw_suffix; -struct work_struct sdw_irq_work; -bool sdw_irq_no_unmask;bool soft_resetting;bool sdw_attached;struct completion init_completion;
@@ -65,10 +63,8 @@ static inline struct cs35l56_private *cs35l56_private_from_base(struct cs35l56_bextern const struct dev_pm_ops cs35l56_pm_ops_i2c_spi; -void cs35l56_mask_soundwire_interrupts (struct sdw_slave *peripheral); -void cs35l56_unmask_soundwire_interrupts (struct sdw_slave *peripheral); -void cs35l56_disable_sdw_interrupts (struct cs35l56_private *cs35l56); -void cs35l56_enable_sdw_interrupts (struct cs35l56_private *cs35l56); +void cs35l56_mask_soundwire_interrupts (struct cs35l56_private *cs35l56); +void cs35l56_unmask_soundwire_interrupts (struct cs35l56_private *cs35l56);int cs35l56_system_suspend (struct device *dev);int cs35l56_system_suspend_late (struct device *dev);
@@ -76,7 +72,6 @@ int cs35l56_system_suspend_no_irq(struct device *dev);int cs35l56_system_resume_no_irq (struct device *dev);int cs35l56_system_resume_early (struct device *dev);int cs35l56_system_resume (struct device *dev); -irqreturn_t cs35l56_irq (int irq,void *data);int cs35l56_irq_request (struct cs35l56_base *cs35l56_base,int irq);int cs35l56_common_probe (struct cs35l56_private *cs35l56,int irq);int cs35l56_init (struct cs35l56_private *cs35l56);
Signature · 1 line
2.47.3
Richard Fitzgerald
maintainerQuoted 3 lines · Richard Fitzgerald
At the time the cs35l56 driver was written the only way to get interrupts from the SoundWire core was to implement a custom handler inside the
interrupt_callback()function.
Mark, don't take this series. I need to send a bugfix for something else and I want to do that first.
Mark Brown
maintainerQuoted 6 lines · Richard Fitzgerald
ASoC: cs35l56: Switch to using the IRQ from the SoundWire core
At the time the cs35l56 driver was written the only way to get interrupts from the SoundWire core was to implement a custom handler inside the
interrupt_callback()function.
Quoted 6 lines
The SoundWire core now provides a virtual IRQ for notifying ImpDef interrupts, and switching to this simplifies the code and also makes it more similar to the normal interrupt handling of I2C/SPI (though some SoundWire specials are still needed).
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.3
Thanks!
[1/4] ASoC: cs35l56: Request IRQ in cs35l56_common_probe()
https://git.kernel.org/broonie/sound/c/888162dabf64 [2/4] ASoC: cs35l56: Move cs35l56_irq_request() after cs35l56_irq() https://git.kernel.org/broonie/sound/c/a075fef187a6 [3/4] soundwire: bus_type: Create IRQ mapping before calling driver probe() https://git.kernel.org/broonie/sound/c/ee1811eacdbb [4/4] ASoC: cs35l56: Use IRQ provided by the SoundWire core https://git.kernel.org/broonie/sound/c/243ca1fb5383
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks,
Mark