| Message ID | 20230512122838.243002-2-ckeepax@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add cs42l43 PC focused SoundWire CODEC | expand |
> @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) > struct device *dev = &slave->dev; > struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > > + if (slave->prop.use_domain_irq && slave->irq) > + handle_nested_irq(slave->irq); > + I am a bit lost here, I can understand that alerts would be handled by a dedicated handler, but here the code continues and will call the existing interrupt_callback. Is this intentional? I wonder if there's a risk with two entities dealing with the same event and programming the same registers. Shouldn't there be some sort of 'either or' rule? > if (drv->ops && drv->ops->interrupt_callback) { > slave_intr.sdca_cascade = sdca_cascade; > slave_intr.control_port = clear; > diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c > index 1f43ee848eac8..fafbc284e82da 100644 > --- a/drivers/soundwire/bus_type.c > +++ b/drivers/soundwire/bus_type.c > @@ -122,6 +122,12 @@ static int sdw_drv_probe(struct device *dev) > if (drv->ops && drv->ops->read_prop) > drv->ops->read_prop(slave); > > + if (slave->prop.use_domain_irq) { > + slave->irq = irq_create_mapping(slave->bus->domain, slave->dev_num); > + if (!slave->irq) > + dev_warn(dev, "Failed to map IRQ\n"); > + } > + > /* init the sysfs as we have properties now */ > ret = sdw_slave_sysfs_init(slave); > if (ret < 0) > @@ -166,7 +172,13 @@ static int sdw_drv_remove(struct device *dev) > int ret = 0; > > mutex_lock(&slave->sdw_dev_lock); > + > slave->probed = false; > + > + if (slave->prop.use_domain_irq) > + irq_dispose_mapping(irq_find_mapping(slave->bus->domain, > + slave->dev_num)); > + > mutex_unlock(&slave->sdw_dev_lock); > > if (drv->remove) > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index ef645de13ae93..c3ab5e5f9cfa4 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -5,6 +5,8 @@ > #define __SOUNDWIRE_H > > #include <linux/bug.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > #include <linux/mod_devicetable.h> > #include <linux/bitfield.h> > > @@ -369,6 +371,7 @@ struct sdw_dpn_prop { > * @clock_reg_supported: the Peripheral implements the clock base and scale > * registers introduced with the SoundWire 1.2 specification. SDCA devices > * do not need to set this boolean property as the registers are required. > + * @use_domain_irq: call actual IRQ handler on slave, as well as callback what callback, the interrupt_callback? That would mean the interrupt is handled twice? I am probably missing something here? > */ > struct sdw_slave_prop { > u32 mipi_revision; > @@ -393,6 +396,7 @@ struct sdw_slave_prop { > u8 scp_int1_mask; > u32 quirks; > bool clock_reg_supported; > + bool use_domain_irq; > };
On Fri, May 12, 2023 at 08:45:51AM -0500, Pierre-Louis Bossart wrote: > > @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) > > struct device *dev = &slave->dev; > > struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > > > > + if (slave->prop.use_domain_irq && slave->irq) > > + handle_nested_irq(slave->irq); > > + > > I am a bit lost here, I can understand that alerts would be handled by a > dedicated handler, but here the code continues and will call the > existing interrupt_callback. > > Is this intentional? I wonder if there's a risk with two entities > dealing with the same event and programming the same registers. > Shouldn't there be some sort of 'either or' rule? > I guess there is a risk of them "handling" the IRQ twice, although it is hard to see why you would write the driver that way. Also since they are sequencial the second would I guess just see that no IRQs are pending. The intention for calling both is that it facilitates using the same IRQ handler for I2C and SoundWire. At least on the Cirrus devices there are a bunch of chip specific registers that need treated exactly the same on I2C and SoundWire, but then a couple of extra registers that need handled in the SoundWire case. This way the handling of those can be kept completely in the SoundWire part of the code and not ifdef-ed into the main IRQ path. Thanks, Charles
On 5/12/23 11:02, Charles Keepax wrote: > On Fri, May 12, 2023 at 08:45:51AM -0500, Pierre-Louis Bossart wrote: >>> @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) >>> struct device *dev = &slave->dev; >>> struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); >>> >>> + if (slave->prop.use_domain_irq && slave->irq) >>> + handle_nested_irq(slave->irq); >>> + >> >> I am a bit lost here, I can understand that alerts would be handled by a >> dedicated handler, but here the code continues and will call the >> existing interrupt_callback. >> >> Is this intentional? I wonder if there's a risk with two entities >> dealing with the same event and programming the same registers. >> Shouldn't there be some sort of 'either or' rule? >> > > I guess there is a risk of them "handling" the IRQ twice, > although it is hard to see why you would write the driver that > way. Also since they are sequencial the second would I guess > just see that no IRQs are pending. > > The intention for calling both is that it facilitates using > the same IRQ handler for I2C and SoundWire. At least on the > Cirrus devices there are a bunch of chip specific registers > that need treated exactly the same on I2C and SoundWire, but > then a couple of extra registers that need handled in the > SoundWire case. This way the handling of those can be kept > completely in the SoundWire part of the code and not ifdef-ed > into the main IRQ path. Sounds good to me, but it's worth adding a comment and improving the commit message with design intent/rules since it's a common part in drivers/soundwire/
On Fri, May 12, 2023 at 11:34:44AM -0500, Pierre-Louis Bossart wrote: > > > On 5/12/23 11:02, Charles Keepax wrote: > > On Fri, May 12, 2023 at 08:45:51AM -0500, Pierre-Louis Bossart wrote: > >>> @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) > >>> struct device *dev = &slave->dev; > >>> struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > >>> > >>> + if (slave->prop.use_domain_irq && slave->irq) > >>> + handle_nested_irq(slave->irq); > >>> + > >> > >> I am a bit lost here, I can understand that alerts would be handled by a > >> dedicated handler, but here the code continues and will call the > >> existing interrupt_callback. > >> > >> Is this intentional? I wonder if there's a risk with two entities > >> dealing with the same event and programming the same registers. > >> Shouldn't there be some sort of 'either or' rule? > >> > > > > I guess there is a risk of them "handling" the IRQ twice, > > although it is hard to see why you would write the driver that > > way. Also since they are sequencial the second would I guess > > just see that no IRQs are pending. > > > > The intention for calling both is that it facilitates using > > the same IRQ handler for I2C and SoundWire. At least on the > > Cirrus devices there are a bunch of chip specific registers > > that need treated exactly the same on I2C and SoundWire, but > > then a couple of extra registers that need handled in the > > SoundWire case. This way the handling of those can be kept > > completely in the SoundWire part of the code and not ifdef-ed > > into the main IRQ path. > > Sounds good to me, but it's worth adding a comment and improving the > commit message with design intent/rules since it's a common part in > drivers/soundwire/ Yeah no issues with updating the commit message to explain that in more detail. Thanks, Charles
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 1ea6a64f8c4a5..30cd03757aafe 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -3,6 +3,7 @@ #include <linux/acpi.h> #include <linux/delay.h> +#include <linux/irq.h> #include <linux/mod_devicetable.h> #include <linux/pm_runtime.h> #include <linux/soundwire/sdw_registers.h> @@ -25,6 +26,23 @@ static int sdw_get_id(struct sdw_bus *bus) return 0; } +static int sdw_irq_map(struct irq_domain *h, unsigned int virq, + irq_hw_number_t hw) +{ + struct sdw_bus *bus = h->host_data; + + irq_set_chip_data(virq, bus); + irq_set_chip(virq, &bus->irq_chip); + irq_set_nested_thread(virq, 1); + irq_set_noprobe(virq); + + return 0; +} + +static const struct irq_domain_ops sdw_domain_ops = { + .map = sdw_irq_map, +}; + /** * sdw_bus_master_add() - add a bus Master instance * @bus: bus instance @@ -142,6 +160,13 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, bus->params.curr_bank = SDW_BANK0; bus->params.next_bank = SDW_BANK1; + bus->irq_chip.name = dev_name(bus->dev); + bus->domain = irq_domain_add_linear(NULL, SDW_MAX_DEVICES, &sdw_domain_ops, bus); + if (!bus->domain) { + dev_err(bus->dev, "Failed to add IRQ domain\n"); + return -EINVAL; + } + return 0; } EXPORT_SYMBOL(sdw_bus_master_add); @@ -178,6 +203,9 @@ static int sdw_delete_slave(struct device *dev, void *data) void sdw_bus_master_delete(struct sdw_bus *bus) { device_for_each_child(bus->dev, NULL, sdw_delete_slave); + + irq_domain_remove(bus->domain); + sdw_master_device_del(bus); sdw_bus_debugfs_exit(bus); @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); + if (slave->prop.use_domain_irq && slave->irq) + handle_nested_irq(slave->irq); + if (drv->ops && drv->ops->interrupt_callback) { slave_intr.sdca_cascade = sdca_cascade; slave_intr.control_port = clear; diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 1f43ee848eac8..fafbc284e82da 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -122,6 +122,12 @@ static int sdw_drv_probe(struct device *dev) if (drv->ops && drv->ops->read_prop) drv->ops->read_prop(slave); + if (slave->prop.use_domain_irq) { + slave->irq = irq_create_mapping(slave->bus->domain, slave->dev_num); + if (!slave->irq) + dev_warn(dev, "Failed to map IRQ\n"); + } + /* init the sysfs as we have properties now */ ret = sdw_slave_sysfs_init(slave); if (ret < 0) @@ -166,7 +172,13 @@ static int sdw_drv_remove(struct device *dev) int ret = 0; mutex_lock(&slave->sdw_dev_lock); + slave->probed = false; + + if (slave->prop.use_domain_irq) + irq_dispose_mapping(irq_find_mapping(slave->bus->domain, + slave->dev_num)); + mutex_unlock(&slave->sdw_dev_lock); if (drv->remove) diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ef645de13ae93..c3ab5e5f9cfa4 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -5,6 +5,8 @@ #define __SOUNDWIRE_H #include <linux/bug.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> #include <linux/mod_devicetable.h> #include <linux/bitfield.h> @@ -369,6 +371,7 @@ struct sdw_dpn_prop { * @clock_reg_supported: the Peripheral implements the clock base and scale * registers introduced with the SoundWire 1.2 specification. SDCA devices * do not need to set this boolean property as the registers are required. + * @use_domain_irq: call actual IRQ handler on slave, as well as callback */ struct sdw_slave_prop { u32 mipi_revision; @@ -393,6 +396,7 @@ struct sdw_slave_prop { u8 scp_int1_mask; u32 quirks; bool clock_reg_supported; + bool use_domain_irq; }; #define SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY BIT(0) @@ -640,6 +644,7 @@ struct sdw_slave_ops { * struct sdw_slave - SoundWire Slave * @id: MIPI device ID * @dev: Linux device + * @irq: IRQ number * @status: Status reported by the Slave * @bus: Bus handle * @prop: Slave properties @@ -669,6 +674,7 @@ struct sdw_slave_ops { struct sdw_slave { struct sdw_slave_id id; struct device dev; + int irq; enum sdw_slave_status status; struct sdw_bus *bus; struct sdw_slave_prop prop; @@ -883,6 +889,7 @@ struct sdw_master_ops { * is used to compute and program bus bandwidth, clock, frame shape, * transport and port parameters * @debugfs: Bus debugfs + * @domain: IRQ domain * @defer_msg: Defer message * @clk_stop_timeout: Clock stop timeout computed * @bank_switch_timeout: Bank switch timeout computed @@ -916,6 +923,8 @@ struct sdw_bus { #ifdef CONFIG_DEBUG_FS struct dentry *debugfs; #endif + struct irq_chip irq_chip; + struct irq_domain *domain; struct sdw_defer defer_msg; unsigned int clk_stop_timeout; u32 bank_switch_timeout;