| Message ID | 20221109165331.29332-10-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the Cirrus Logic CS48L32 audio codecs | expand |
On Wed, 09 Nov 2022 16:53:28 +0000, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable > interrupt controller with a variety of interrupt sources, including > GPIOs that can be used as interrupt inputs. > > This driver provides the handling for the interrupt controller. As the > codec is accessed via regmap, the generic regmap_irq functionality > is used to do most of the work. > I cannot spot a shred of interrupt controller code in there. This belongs IMO to the MFD code. It is also a direct copy of the existing irq-madera.c code, duplicated for no obvious reason. M.
On 10/11/2022 08:02, Marc Zyngier wrote: > On Wed, 09 Nov 2022 16:53:28 +0000, > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >> >> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable >> interrupt controller with a variety of interrupt sources, including >> GPIOs that can be used as interrupt inputs. >> >> This driver provides the handling for the interrupt controller. As the >> codec is accessed via regmap, the generic regmap_irq functionality >> is used to do most of the work. >> > > I cannot spot a shred of interrupt controller code in there. This It is providing support for handling an interrupt controller so that other drivers can bind to those interrupts. It's just that regmap provides a lot of generic implementation for SPI-connected interrupt controllers so we don't need to open-code all that in the irqchip driver. > belongs IMO to the MFD code. We did once put interrupt support in MFD for an older product line but the MFD maintainer doesn't like the MFD being a dumping-ground for random other functionality that have their own subsystems. > It is also a direct copy of the existing > irq-madera.c code, duplicated for no obvious reason. It's not a duplicate. The register map of this device is different (different addressing, 32-bit registers not 16-bit) > > M. >
On Thu, 10 Nov 2022 11:22:26 +0000, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > On 10/11/2022 08:02, Marc Zyngier wrote: > > On Wed, 09 Nov 2022 16:53:28 +0000, > > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >> > >> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable > >> interrupt controller with a variety of interrupt sources, including > >> GPIOs that can be used as interrupt inputs. > >> > >> This driver provides the handling for the interrupt controller. As the > >> codec is accessed via regmap, the generic regmap_irq functionality > >> is used to do most of the work. > >> > > > > I cannot spot a shred of interrupt controller code in there. This > > It is providing support for handling an interrupt controller so that > other drivers can bind to those interrupts. It's just that regmap > provides a lot of generic implementation for SPI-connected interrupt > controllers so we don't need to open-code all that in the > irqchip driver. And thus none of that code needs to live in drivers/irqchip. > > > belongs IMO to the MFD code. > > We did once put interrupt support in MFD for an older product line but > the MFD maintainer doesn't like the MFD being a dumping-ground for > random other functionality that have their own subsystems. I don't like this stuff either. All this code is a glorified set of interrupt handlers and #defines that only hide the lack of a proper DT binding to express the interrupt routing (it feels like looking at board files from 10 years ago). None of that belongs in the irqchip code. > > > It is also a direct copy of the existing > > irq-madera.c code, duplicated for no obvious reason. > > It's not a duplicate. The register map of this device is different > (different addressing, 32-bit registers not 16-bit) And? How hard is it to implement an indirection containing the register map and the relevant callbacks? /roll-eyes M.
On 10/11/2022 12:01, Marc Zyngier wrote: > On Thu, 10 Nov 2022 11:22:26 +0000, > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >> >> On 10/11/2022 08:02, Marc Zyngier wrote: >>> On Wed, 09 Nov 2022 16:53:28 +0000, >>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >>>> >>>> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable >>>> interrupt controller with a variety of interrupt sources, including >>>> GPIOs that can be used as interrupt inputs. >>>> >>>> This driver provides the handling for the interrupt controller. As the >>>> codec is accessed via regmap, the generic regmap_irq functionality >>>> is used to do most of the work. >>>> >>> >>> I cannot spot a shred of interrupt controller code in there. This >> >> It is providing support for handling an interrupt controller so that >> other drivers can bind to those interrupts. It's just that regmap >> provides a lot of generic implementation for SPI-connected interrupt >> controllers so we don't need to open-code all that in the >> irqchip driver. > > And thus none of that code needs to live in drivers/irqchip. > >> >>> belongs IMO to the MFD code. >> >> We did once put interrupt support in MFD for an older product line but >> the MFD maintainer doesn't like the MFD being a dumping-ground for >> random other functionality that have their own subsystems. > > I don't like this stuff either. All this code is a glorified set of > interrupt handlers and #defines that only hide the lack of a proper DT > binding to express the interrupt routing (it feels like looking at > board files from 10 years ago). > I didn't understand this. The whole purpose of this is to instantiate Linux interrupts for the PIC interrupt sources so that other drivers that want to use the interrupts from the CS48L32 PIC can use standard kernel APIs or DT to bind against them. The four handlers registered within the driver are done here simply because they don't belong to any particular child driver. Since they are a fixed feature of the chip that we know we want to handle we may as well just register them. If we put them in the MFD with DT definitions it would make a circular dependency between MFD and its child, which is not a great situation. If it's these handlers that are bothering you, we could move them to the audio driver. > None of that belongs in the irqchip code. > I don't really understand here what the criteria is that makes this not a irqchip driver but it was ok for madera. We have a PIC and we need to handle that and export those interrupts so other drivers can bind against them. Is the problem that the PIC is on a SPI bus and irqchip is only for memory-mapped PICs? Or is it that we have re-used existing library code instead of open-coding it, so you aren't seeing the actual handling code? As Lee has already objected in the past to having the interrupt controller implementation in MFD I don't want to move it there without Lee's agreement that it's ok to put the PIC IRQ implementation in MFD for CS48L32. >> >>> It is also a direct copy of the existing >>> irq-madera.c code, duplicated for no obvious reason. >> >> It's not a duplicate. The register map of this device is different >> (different addressing, 32-bit registers not 16-bit) > > And? How hard is it to implement an indirection containing the > register map and the relevant callbacks? /roll-eyes > > M. >
On Thu, Nov 10, 2022 at 11:22:26AM +0000, Richard Fitzgerald wrote: > On 10/11/2022 08:02, Marc Zyngier wrote: > > belongs IMO to the MFD code. > We did once put interrupt support in MFD for an older product line but > the MFD maintainer doesn't like the MFD being a dumping-ground for > random other functionality that have their own subsystems. There's bits of this like logging the top level error interrupts that seem like they clearly fit in the driver for the top level chip (SYSCLK possibly in the audio driver, dunno if it gets used by other functions), they're users of the interrupt controller rather than part of the interrupt controller. > > It is also a direct copy of the existing > > irq-madera.c code, duplicated for no obvious reason. > It's not a duplicate. The register map of this device is different > (different addressing, 32-bit registers not 16-bit) Isn't that just a data difference which could be parameterised?
On 10/11/2022 12:01, Marc Zyngier wrote: > On Thu, 10 Nov 2022 11:22:26 +0000, > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >> >> On 10/11/2022 08:02, Marc Zyngier wrote: >>> On Wed, 09 Nov 2022 16:53:28 +0000, >>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >>>> >>>> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable >>>> interrupt controller with a variety of interrupt sources, including >>>> GPIOs that can be used as interrupt inputs. >>>> >>>> This driver provides the handling for the interrupt controller. As the >>>> codec is accessed via regmap, the generic regmap_irq functionality >>>> is used to do most of the work. >>>> >>> >>> I cannot spot a shred of interrupt controller code in there. This >> >> It is providing support for handling an interrupt controller so that >> other drivers can bind to those interrupts. It's just that regmap >> provides a lot of generic implementation for SPI-connected interrupt >> controllers so we don't need to open-code all that in the >> irqchip driver. > > And thus none of that code needs to live in drivers/irqchip. > >> >>> belongs IMO to the MFD code. >> >> We did once put interrupt support in MFD for an older product line but >> the MFD maintainer doesn't like the MFD being a dumping-ground for >> random other functionality that have their own subsystems. > > I don't like this stuff either. All this code is a glorified set of > interrupt handlers and #defines that only hide the lack of a proper DT > binding to express the interrupt routing (it feels like looking at > board files from 10 years ago). > > None of that belongs in the irqchip code. > >> >>> It is also a direct copy of the existing >>> irq-madera.c code, duplicated for no obvious reason. >> >> It's not a duplicate. The register map of this device is different >> (different addressing, 32-bit registers not 16-bit) > > And? How hard is it to implement an indirection containing the > register map and the relevant callbacks? /roll-eyes > I note your accusation that we were too lazy (or too stupid?) to think of this. > M. >
On Thu, 10 Nov 2022 13:00:50 +0000, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > On 10/11/2022 12:01, Marc Zyngier wrote: > > On Thu, 10 Nov 2022 11:22:26 +0000, > > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >> > >> On 10/11/2022 08:02, Marc Zyngier wrote: > >>> On Wed, 09 Nov 2022 16:53:28 +0000, > >>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >>>> > >>>> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable > >>>> interrupt controller with a variety of interrupt sources, including > >>>> GPIOs that can be used as interrupt inputs. > >>>> > >>>> This driver provides the handling for the interrupt controller. As the > >>>> codec is accessed via regmap, the generic regmap_irq functionality > >>>> is used to do most of the work. > >>>> > >>> > >>> I cannot spot a shred of interrupt controller code in there. This > >> > >> It is providing support for handling an interrupt controller so that > >> other drivers can bind to those interrupts. It's just that regmap > >> provides a lot of generic implementation for SPI-connected interrupt > >> controllers so we don't need to open-code all that in the > >> irqchip driver. > > > > And thus none of that code needs to live in drivers/irqchip. > > > >> > >>> belongs IMO to the MFD code. > >> > >> We did once put interrupt support in MFD for an older product line but > >> the MFD maintainer doesn't like the MFD being a dumping-ground for > >> random other functionality that have their own subsystems. > > > > I don't like this stuff either. All this code is a glorified set of > > interrupt handlers and #defines that only hide the lack of a proper DT > > binding to express the interrupt routing (it feels like looking at > > board files from 10 years ago). > > > > I didn't understand this. The whole purpose of this is to instantiate > Linux interrupts for the PIC interrupt sources so that other drivers > that want to use the interrupts from the CS48L32 PIC can use standard > kernel APIs or DT to bind against them. There is zero standard APIs in this patch. Does cs48l32_request_irq() look standard to you? This whole thing makes a mockery of the interrupt model and of firmware-based interrupt description which we spent years to build. > > The four handlers registered within the driver are done here simply > because they don't belong to any particular child driver. Since they > are a fixed feature of the chip that we know we want to handle we may as > well just register them. Again, they have no purpose in an interrupt controller driver. > If we put them in the MFD with DT definitions it would make a > circular dependency between MFD and its child, which is not a great > situation. If it's these handlers that are bothering you, we could move > them to the audio driver. And what's left? Nothing. > > > None of that belongs in the irqchip code. > > > > I don't really understand here what the criteria is that makes this not > a irqchip driver but it was ok for madera. We have a PIC and we need to > handle that and export those interrupts so other drivers can bind > against them. Is the problem that the PIC is on a SPI bus and irqchip is > only for memory-mapped PICs? Or is it that we have re-used existing > library code instead of open-coding it, so you aren't seeing the actual > handling code? An irqchip driver uses the irq_chip structure, uses irq domains to abstract the device-specific interrupt numbering from clients, and doesn't force the use of an esoteric API on these clients. What I see here is the exact opposite. Was it OK for madera? No. A moment of weakness, I presume. Do I want to repeat the same mistake? Neither. > As Lee has already objected in the past to having the interrupt > controller implementation in MFD I don't want to move it there without > Lee's agreement that it's ok to put the PIC IRQ implementation in MFD > for CS48L32. If you were implementing an actual interrupt controller driver, I'd take it without any question. The fact that this code mandates the use of its own homegrown API rules it out. Thanks, M.
On Thu, 10 Nov 2022 13:14:30 +0000, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > I note your accusation that we were too lazy (or too stupid?) > to think of this. Take it the way you want. But I criticise the code, not the author. And I'm merely pointed out that there was significant room for improvement. M.
On 10/11/2022 15:13, Marc Zyngier wrote: > On Thu, 10 Nov 2022 13:00:50 +0000, > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >> >> On 10/11/2022 12:01, Marc Zyngier wrote: >>> On Thu, 10 Nov 2022 11:22:26 +0000, >>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >>>> >>>> On 10/11/2022 08:02, Marc Zyngier wrote: >>>>> On Wed, 09 Nov 2022 16:53:28 +0000, >>>>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: >>>>>> >>>>>> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable >>>>>> interrupt controller with a variety of interrupt sources, including >>>>>> GPIOs that can be used as interrupt inputs. >>>>>> >>>>>> This driver provides the handling for the interrupt controller. As the >>>>>> codec is accessed via regmap, the generic regmap_irq functionality >>>>>> is used to do most of the work. >>>>>> >>>>> >>>>> I cannot spot a shred of interrupt controller code in there. This >>>> >>>> It is providing support for handling an interrupt controller so that >>>> other drivers can bind to those interrupts. It's just that regmap >>>> provides a lot of generic implementation for SPI-connected interrupt >>>> controllers so we don't need to open-code all that in the >>>> irqchip driver. >>> >>> And thus none of that code needs to live in drivers/irqchip. >>> >>>> >>>>> belongs IMO to the MFD code. >>>> >>>> We did once put interrupt support in MFD for an older product line but >>>> the MFD maintainer doesn't like the MFD being a dumping-ground for >>>> random other functionality that have their own subsystems. >>> >>> I don't like this stuff either. All this code is a glorified set of >>> interrupt handlers and #defines that only hide the lack of a proper DT >>> binding to express the interrupt routing (it feels like looking at >>> board files from 10 years ago). >>> >> >> I didn't understand this. The whole purpose of this is to instantiate >> Linux interrupts for the PIC interrupt sources so that other drivers >> that want to use the interrupts from the CS48L32 PIC can use standard >> kernel APIs or DT to bind against them. > > There is zero standard APIs in this patch. Does cs48l32_request_irq() > look standard to you? This whole thing makes a mockery of the > interrupt model and of firmware-based interrupt description which we > spent years to build. > >> >> The four handlers registered within the driver are done here simply >> because they don't belong to any particular child driver. Since they >> are a fixed feature of the chip that we know we want to handle we may as >> well just register them. > > Again, they have no purpose in an interrupt controller driver. > >> If we put them in the MFD with DT definitions it would make a >> circular dependency between MFD and its child, which is not a great >> situation. If it's these handlers that are bothering you, we could move >> them to the audio driver. > > And what's left? Nothing. Ah, I see. You've missed that the bulk of the implementation re-uses existing library code from regmap. It does say this in the commit message. "the generic regmap_irq functionality is used to do most of the work." and I've also said this in previous replies. This is no way driver that does nothing. There's over 1000 lines of code handling the PIC and dispatching its interrupts to other drivers that want to bind to them. It's just that it makes no sense to duplicate 1300 lines of interrupt handling code from elsewhere when we can re-use that by calling regmap_add_irq_chip(). That gives us all the interrupt- controller-handling code in drivers/base/regmap/regmap-irq.c Perhaps you could re-review this taking into account that regmap_add_irq_chip() is significant.
On Thu, Nov 10, 2022 at 04:31:06PM +0000, Richard Fitzgerald wrote: > On 10/11/2022 15:13, Marc Zyngier wrote: > > > If we put them in the MFD with DT definitions it would make a > > > circular dependency between MFD and its child, which is not a great > > > situation. If it's these handlers that are bothering you, we could move > > > them to the audio driver. > > And what's left? Nothing. > Ah, I see. You've missed that the bulk of the implementation re-uses > existing library code from regmap. It does say this in the commit > message. > "the generic regmap_irq functionality is used to do most of the work." > and I've also said this in previous replies. The thread prompted me to have a look at regmap-irq earlier today and see what it's still doing that peers into the regmap core internals and it seems it's just getting the register stride which has had an external API added already and getting the device for the regmap. It should be straightforward to repaint it and move it into the irqchip subsystem which would be a much more sensible home for a library for implementing irqchips in this day and age. I started looking at the code changes for that a bit. > This is no way driver that does nothing. There's over 1000 lines of code > handling the PIC and dispatching its interrupts to other drivers that > want to bind to them. It's just that it makes no sense to duplicate 1300 > lines of interrupt handling code from elsewhere when we can re-use that > by calling regmap_add_irq_chip(). That gives us all the interrupt- > controller-handling code in drivers/base/regmap/regmap-irq.c TBF that's 1000 lines of overly generic code, a bunch of it is conditional stuff and it's unlikely that any individual driver would want all of it. Equally it does mean that all the users are just providing data rather than writing any code which generally makes things easier to maintain and was the main goal.
On Thu, 10 Nov 2022 16:31:06 +0000, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > On 10/11/2022 15:13, Marc Zyngier wrote: > > On Thu, 10 Nov 2022 13:00:50 +0000, > > Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >> > >> On 10/11/2022 12:01, Marc Zyngier wrote: > >>> On Thu, 10 Nov 2022 11:22:26 +0000, > >>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >>>> > >>>> On 10/11/2022 08:02, Marc Zyngier wrote: > >>>>> On Wed, 09 Nov 2022 16:53:28 +0000, > >>>>> Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > >>>>>> > >>>>>> The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable > >>>>>> interrupt controller with a variety of interrupt sources, including > >>>>>> GPIOs that can be used as interrupt inputs. > >>>>>> > >>>>>> This driver provides the handling for the interrupt controller. As the > >>>>>> codec is accessed via regmap, the generic regmap_irq functionality > >>>>>> is used to do most of the work. > >>>>>> > >>>>> > >>>>> I cannot spot a shred of interrupt controller code in there. This > >>>> > >>>> It is providing support for handling an interrupt controller so that > >>>> other drivers can bind to those interrupts. It's just that regmap > >>>> provides a lot of generic implementation for SPI-connected interrupt > >>>> controllers so we don't need to open-code all that in the > >>>> irqchip driver. > >>> > >>> And thus none of that code needs to live in drivers/irqchip. > >>> > >>>> > >>>>> belongs IMO to the MFD code. > >>>> > >>>> We did once put interrupt support in MFD for an older product line but > >>>> the MFD maintainer doesn't like the MFD being a dumping-ground for > >>>> random other functionality that have their own subsystems. > >>> > >>> I don't like this stuff either. All this code is a glorified set of > >>> interrupt handlers and #defines that only hide the lack of a proper DT > >>> binding to express the interrupt routing (it feels like looking at > >>> board files from 10 years ago). > >>> > >> > >> I didn't understand this. The whole purpose of this is to instantiate > >> Linux interrupts for the PIC interrupt sources so that other drivers > >> that want to use the interrupts from the CS48L32 PIC can use standard > >> kernel APIs or DT to bind against them. > > > > There is zero standard APIs in this patch. Does cs48l32_request_irq() > > look standard to you? This whole thing makes a mockery of the > > interrupt model and of firmware-based interrupt description which we > > spent years to build. > > > >> > >> The four handlers registered within the driver are done here simply > >> because they don't belong to any particular child driver. Since they > >> are a fixed feature of the chip that we know we want to handle we may as > >> well just register them. > > > > Again, they have no purpose in an interrupt controller driver. > > > >> If we put them in the MFD with DT definitions it would make a > >> circular dependency between MFD and its child, which is not a great > >> situation. If it's these handlers that are bothering you, we could move > >> them to the audio driver. > > > > And what's left? Nothing. > > Ah, I see. You've missed that the bulk of the implementation re-uses > existing library code from regmap. It does say this in the commit > message. > > "the generic regmap_irq functionality is used to do most of the work." > > and I've also said this in previous replies. > > This is no way driver that does nothing. There's over 1000 lines of code > handling the PIC and dispatching its interrupts to other drivers that > want to bind to them. It's just that it makes no sense to duplicate 1300 > lines of interrupt handling code from elsewhere when we can re-use that > by calling regmap_add_irq_chip(). That gives us all the interrupt- > controller-handling code in drivers/base/regmap/regmap-irq.c > > Perhaps you could re-review this taking into account that > regmap_add_irq_chip() is significant. Read again what I have written. Having to expose a device-specific API for endpoint drivers to obtain their interrupts, and requiring them to know about some magic values that describe the interrupts source are not a acceptable constructs. We have firmware descriptions to expose interrupt linkages, and your HW is not special enough to deserve its own top level API. Yes, we accepted such drivers in the past, but it has to stop. Either you describe the internal structure of your device in DT or ACPI, and make all client drivers use the standard API, or you make this a codec library, purely specific to your device and only used by it. But the current shape is not something I'm prepared to accept. M.
On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote: > Read again what I have written. Having to expose a device-specific API > for endpoint drivers to obtain their interrupts, and requiring them to > know about some magic values that describe the interrupts source are > not a acceptable constructs. > We have firmware descriptions to expose interrupt linkages, and your > HW is not special enough to deserve its own top level API. Yes, we > accepted such drivers in the past, but it has to stop. > Either you describe the internal structure of your device in DT or > ACPI, and make all client drivers use the standard API, or you make > this a codec library, purely specific to your device and only used by > it. But the current shape is not something I'm prepared to accept. ACPI gets to be a lot of fun here, it's just not idiomatic to describe the internals of these devices in firmware there and a lot of the systems shipping this stuff are targeted at other OSs and system integrators are therefore not in the least worried about Linux preferences. You'd need to look at having the MFD add additional description via swnode or something to try to get things going. MFD does have support for that, though it's currently mainly used with devices that only have ACPI use (axp20x looks like the only potentially DT user, from the git history the swnode bits are apparently for use on ACPI systems). That might get fragile in the DT case since you could have multiple sources for description of the same thing unless you do something like suppress the swnode stuff on DT systems. Given that swnode is basically DT written out in C code I'm not actually convinced it's that much of a win, unless someone writes some tooling to generate swnode data from DT files you're not getting the benefit of any of the schema validation work that's being done. We'd also need to do some work for regulators to make sure that if we are parsing DT properties on ACPI systems we don't do so from _DSD since ACPI has strong ideas about how power works and we don't want to end up with systems with firmware providing mixed ACPI/DT models without a clear understanding of what we're geting into. I do also have other concerns in the purely DT case, especially with chip functions like the CODEC where there's a very poor mapping between physical IPs and how Linux is tending to describe things internally at the minute. In particular these devices often have a clock tree portions of which can be visible and useful off chip but which tends to get lumped in with the audio IPs in our current code. Ideally we'd describe that as a clock subdevice (or subdevices if that fits the hardware) using the clock bindings but then that has a bunch of knock on effects the way the code currently is which probably it's probably disproportionate to force an individual driver author to work through. OTOH the DT bindings should be OS neutral ABI so...
On Thu, 10 Nov 2022 20:36:16 +0000, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote: > > > Read again what I have written. Having to expose a device-specific API > > for endpoint drivers to obtain their interrupts, and requiring them to > > know about some magic values that describe the interrupts source are > > not a acceptable constructs. > > > We have firmware descriptions to expose interrupt linkages, and your > > HW is not special enough to deserve its own top level API. Yes, we > > accepted such drivers in the past, but it has to stop. > > > Either you describe the internal structure of your device in DT or > > ACPI, and make all client drivers use the standard API, or you make > > this a codec library, purely specific to your device and only used by > > it. But the current shape is not something I'm prepared to accept. > > ACPI gets to be a lot of fun here, it's just not idiomatic to describe > the internals of these devices in firmware there and a lot of the > systems shipping this stuff are targeted at other OSs and system > integrators are therefore not in the least worried about Linux > preferences. Let me reassure the vendors that I do not care about them either. By this standard, we'd all run Windows on x86. > You'd need to look at having the MFD add additional > description via swnode or something to try to get things going. MFD > does have support for that, though it's currently mainly used with > devices that only have ACPI use (axp20x looks like the only potentially > DT user, from the git history the swnode bits are apparently for use on > ACPI systems). That might get fragile in the DT case since you could > have multiple sources for description of the same thing unless you do > something like suppress the swnode stuff on DT systems. > > Given that swnode is basically DT written out in C code I'm not actually > convinced it's that much of a win, unless someone writes some tooling to > generate swnode data from DT files you're not getting the benefit of any > of the schema validation work that's being done. We'd also need to do > some work for regulators to make sure that if we are parsing DT > properties on ACPI systems we don't do so from _DSD since ACPI has > strong ideas about how power works and we don't want to end up with > systems with firmware providing mixed ACPI/DT models without a clear > understanding of what we're geting into. > > I do also have other concerns in the purely DT case, especially with > chip functions like the CODEC where there's a very poor mapping between > physical IPs and how Linux is tending to describe things internally at > the minute. In particular these devices often have a clock tree > portions of which can be visible and useful off chip but which tends to > get lumped in with the audio IPs in our current code. Ideally we'd > describe that as a clock subdevice (or subdevices if that fits the > hardware) using the clock bindings but then that has a bunch of knock on > effects the way the code currently is which probably it's probably > disproportionate to force an individual driver author to work through. > OTOH the DT bindings should be OS neutral ABI so... I don't think this is a reason to continue on the current path that pretends to have something generic, but instead is literally a board file fragment with baked-in magic numbers. An irqchip is supposed to offer services to arbitrary clients (endpoint drivers) that are oblivious of the irqchip itself, of the hwirq mapping, and use the standard APIs to obtain a virtual interrupt number. None of that here. This is a monolithic driver, only split across multiple subsystem to satisfy a "not in my backyard" requirement. If the vendors/authors want to keep the shape of the code as is, they can do it outside of the irqchip code and have some library code with an internal API. At least they will stop pretending that this is a general purpose driver. And the existing madera code can also go in the process. M.
On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote: > On Thu, 10 Nov 2022 20:36:16 +0000, > Mark Brown <broonie@kernel.org> wrote: > > On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote: Apologies this ended up getting quite long. Cirrus has no trouble changing how the IRQ driver works I just think we are struggling a little to understand exactly what parts of the code need reworking in what way, we appreciate your patience in helping us through. > > > If you were implementing an actual interrupt controller driver, I'd > > > take it without any question. The fact that this code mandates > > > the use of its own homegrown API rules it out. I think this is part of the crossed wires here, the code does not mandate the use of its own home grown API, although it does provide one. For example our CODECs often provide GPIO/IRQ services for other devices such as say speaker amps attached along side them. Here is a DT example from one of my dev systems with GPIO1 on cs47l35 (a madera CODEC) handling the IRQ for cs35l35 (a speaker amp): cs35l35_left: cs35l35@40 { compatible = "cirrus,cs35l35"; reg = <0x40>; #sound-dai-cells = <1>; reset-gpios = <&axi_gpio0 0 0>; interrupt-parent = <&cs47l35>; interrupts = <57 0>; }; No special code is required in the cs35l35 driver (it is fully upstreamed sound/soc/codecs/cs35l35.c). So if we are missing some actual interrupt controller API we need to be supporting that we are not please point us at it and we will happily add support? So I think your objections are mostly regarding the cs48l32_request_irq function (and friends) that are being used by the other parts of the MFD. I don't think it would be super hard to remove these functions and move the IRQ into DT if that is the preferred way. > > ACPI gets to be a lot of fun here, it's just not idiomatic to describe > > the internals of these devices in firmware there and a lot of the > > systems shipping this stuff are targeted at other OSs and system > > integrators are therefore not in the least worried about Linux > > preferences. I would echo Mark's statement that going the way of moving this into DT/ACPI will actually likely necessitate the addition of a lot of "board file" stuff in the future. If the part gets used in any ACPI systems (granted support is not in yet but this is not a super unlikely addition in the future for cs48l32) we will need to support the laptops containing the part in Linux and the vendors are extremely unlikely to put internal CODEC IRQs into the ACPI tables. But that aside I guess my main question about this approach would be what the DT binding would look like for the CODEC. Currently our devices use a single DT node for the device. Again pulling a Madera example from my dev setup, this is what the DT binding for one of our CODECs currently looks vaguely like: cs47l35: cs47l35@1 { compatible = "cirrus,cs47l35"; reg = <0x1>; spi-max-frequency = <11000000>; interrupt-controller; #interrupt-cells = <2>; interrupt-parent = <&gpio0>; interrupts = <56 8>; gpio-controller; #gpio-cells = <2>; #sound-dai-cells = <1>; AVDD-supply = <&lochnagar_vdd1v8>; DBVDD1-supply = <&lochnagar_vdd1v8>; DBVDD2-supply = <&lochnagar_vdd1v8>; CPVDD1-supply = <&lochnagar_vdd1v8>; CPVDD2-supply = <&lochnagar_vddcore>; DCVDD-supply = <&lochnagar_vddcore>; SPKVDD-supply = <&wallvdd>; reset-gpios = <&lochnagar_pin 0 0>; clocks = <&lochnagar_clk LOCHNAGAR_CDC_MCLK1>, <&lochnagar_clk LOCHNAGAR_CDC_MCLK2>; clock-names = "mclk1", "mclk2"; pinctrl-names = "default"; pinctrl-0 = <&cs47l35_defaults>; }; The interrupt-parent points at who our IRQ is connected to, and we are an interrupt-controller so people can use our IRQs. I think it is not currently supported to have more than a single interrupt-parent for a device so with the current binding is it actually possible for the device to refer to its own IRQs in DT? An alternative approach would be to actually represent the MFD in device tree, I think this would allow things to work and look something like (totally not tested just for discussion): cs47l35: cs47l35@1 { compatible = "cirrus,cs47l35"; reg = <0x1>; spi-max-frequency = <11000000>; irq: madera-irq { compatible = "cirrus,madera-irq"; interrupt-controller; #interrupt-cells = <2>; interrupt-parent = <&gpio0>; interrupts = <56 8>; }; gpio: madera-gpio { compatible = "cirrus,madera-gpio"; gpio-controller; #gpio-cells = <2>; }; sound: madera-sound { compatible = "cirrus,cs47l35-sound"; interrupt-parent = <&madera-irq>; interrupts = <55 0>, <56 0>; #sound-dai-cells = <1>; }; }; Historically I believe we have been discouraged (by upstream, not from our customers) from explicitly representing the parts of the MFD in device tree separately, as it was viewed that this is just an external SPI CODEC and one node mapped much more logically to the hardware, which is what DT should be describing. However, are you saying this would be a preferrable approach from your side? Or am I missing some alternative solution? Again thank you kindly for you time looking at this. Thanks, Charles
On Fri, Nov 11, 2022 at 11:16:11AM +0000, Charles Keepax wrote: > On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote: > > > > ACPI gets to be a lot of fun here, it's just not idiomatic to describe > > > the internals of these devices in firmware there and a lot of the > > > systems shipping this stuff are targeted at other OSs and system > > > integrators are therefore not in the least worried about Linux > > > preferences. > I would echo Mark's statement that going the way of moving this > into DT/ACPI will actually likely necessitate the addition of a > lot of "board file" stuff in the future. If the part gets used in > any ACPI systems (granted support is not in yet but this is not a > super unlikely addition in the future for cs48l32) we will need to > support the laptops containing the part in Linux and the vendors are > extremely unlikely to put internal CODEC IRQs into the ACPI tables. It's a bit of a stronger issue than that in that it's not how ACPI is usually expected to work (it draws more from the PCI model where you just get a top level ID from the device and have to figure the rest out yourself). > An alternative approach would be to actually represent the MFD in > device tree, I think this would allow things to work and look > something like (totally not tested just for discussion): That's what Marc's pushing for - there is an idea to do that which works well enough for cases (like this irqchip for the most part, modulo how to handle the top level interrupts for the chip) where the way Linux wants to model the device maps clearly onto the hardware but like I was mentioning with the audio/clocking split it gets tricky where things are more up in the air and potentially changable since it's much harder to define a suitable ABI.
On Fri, Nov 11, 2022 at 11:16:11AM +0000, Charles Keepax wrote: > On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote: > > On Thu, 10 Nov 2022 20:36:16 +0000, > > Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote: > The interrupt-parent points at who our IRQ is connected to, and > we are an interrupt-controller so people can use our IRQs. I > think it is not currently supported to have more than a single > interrupt-parent for a device so with the current binding is it > actually possible for the device to refer to its own IRQs in DT? > I see there is actually interrupts-extended which would let us refer to ourselves although its a little unclear to be if that would actually work but might be worth a look. Thanks, Charles
On Fri, Nov 11, 2022 at 11:49:25AM +0000, Mark Brown wrote: > On Fri, Nov 11, 2022 at 11:16:11AM +0000, Charles Keepax wrote: > > On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote: > > > > > > ACPI gets to be a lot of fun here, it's just not idiomatic to describe > > > > the internals of these devices in firmware there and a lot of the > > > > systems shipping this stuff are targeted at other OSs and system > > > > integrators are therefore not in the least worried about Linux > > > > preferences. > > > I would echo Mark's statement that going the way of moving this > > into DT/ACPI will actually likely necessitate the addition of a > > lot of "board file" stuff in the future. If the part gets used in > > any ACPI systems (granted support is not in yet but this is not a > > super unlikely addition in the future for cs48l32) we will need to > > support the laptops containing the part in Linux and the vendors are > > extremely unlikely to put internal CODEC IRQs into the ACPI tables. > > It's a bit of a stronger issue than that in that it's not how ACPI is > usually expected to work (it draws more from the PCI model where you > just get a top level ID from the device and have to figure the rest out > yourself). > Hmm... yes ok true ACPI isn't going to put the elements of the MFD in either so we would then need something to bind all those in as well. Thanks, Charles
On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote: > > > Either you describe the internal structure of your device in DT or > > > ACPI, and make all client drivers use the standard API, or you make > > > this a codec library, purely specific to your device and only used by > > > it. But the current shape is not something I'm prepared to accept. > > ACPI gets to be a lot of fun here, it's just not idiomatic to describe > > the internals of these devices in firmware there and a lot of the > > systems shipping this stuff are targeted at other OSs and system > > integrators are therefore not in the least worried about Linux > > preferences. > Let me reassure the vendors that I do not care about them either. By > this standard, we'd all run Windows on x86. It turns out a bunch of these systems are intended to be used with Linux, and even where the vendor does care about Linux we also have to consider what's tasteful for ACPI. > > You'd need to look at having the MFD add additional > > description via swnode or something to try to get things going. MFD ... > > Given that swnode is basically DT written out in C code I'm not actually > > convinced it's that much of a win, unless someone writes some tooling to > > generate swnode data from DT files you're not getting the benefit of any ... > > I do also have other concerns in the purely DT case, especially with > > chip functions like the CODEC where there's a very poor mapping between > > physical IPs and how Linux is tending to describe things internally at > > the minute. In particular these devices often have a clock tree > I don't think this is a reason to continue on the current path that > pretends to have something generic, but instead is literally a board > file fragment with baked-in magic numbers. > An irqchip is supposed to offer services to arbitrary clients > (endpoint drivers) that are oblivious of the irqchip itself, of the > hwirq mapping, and use the standard APIs to obtain a virtual interrupt > number. None of that here. This is a monolithic driver, only split > across multiple subsystem to satisfy a "not in my backyard" > requirement. > If the vendors/authors want to keep the shape of the code as is, they > can do it outside of the irqchip code and have some library code with > an internal API. At least they will stop pretending that this is a > general purpose driver. And the existing madera code can also go in > the process. Yeah, I'm definitely not in the least bit convinced that the irqchip code is a good home for this sort of glue (especially the interrupt consumers) for the reasons you mention - my concern was more that the firmware interface also has issues, and that putting things into firmware is also putting them into ABI which is much harder to do a good job with later.
diff --git a/MAINTAINERS b/MAINTAINERS index cd1773d39dd8..f52e9a6e290c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5015,12 +5015,14 @@ F: Documentation/devicetree/bindings/pinctrl/cirrus,cs48l32.yaml F: Documentation/devicetree/bindings/pinctrl/cirrus,madera.yaml F: Documentation/devicetree/bindings/sound/cirrus,madera.yaml F: drivers/gpio/gpio-madera* +F: drivers/irqchip/irq-cirrus-cs48l32* F: drivers/irqchip/irq-madera* F: drivers/mfd/cs47l* F: drivers/mfd/cs48l* F: drivers/mfd/madera* F: drivers/pinctrl/cirrus/* F: include/dt-bindings/sound/madera* +F: include/linux/irqchip/irq-cirrus-cs48l32* F: include/linux/irqchip/irq-madera* F: include/linux/mfd/cs48l32/* F: include/linux/mfd/madera/* diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 7ef9f5e696d3..d4521158849c 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -136,6 +136,9 @@ config BRCMSTB_L2_IRQ select GENERIC_IRQ_CHIP select IRQ_DOMAIN +config CIRRUS_CS48L32_IRQ + tristate + config DAVINCI_AINTC bool select GENERIC_IRQ_CHIP diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 87b49a10962c..049796365232 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -121,3 +121,4 @@ obj-$(CONFIG_IRQ_IDT3243X) += irq-idt3243x.o obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o obj-$(CONFIG_MCHP_EIC) += irq-mchp-eic.o obj-$(CONFIG_SUNPLUS_SP7021_INTC) += irq-sp7021-intc.o +obj-$(CONFIG_CIRRUS_CS48L32_IRQ) += irq-cirrus-cs48l32.o diff --git a/drivers/irqchip/irq-cirrus-cs48l32.c b/drivers/irqchip/irq-cirrus-cs48l32.c new file mode 100644 index 000000000000..3ca9f34a6289 --- /dev/null +++ b/drivers/irqchip/irq-cirrus-cs48l32.c @@ -0,0 +1,281 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Interrupt support for Cirrus Logic CS48L32 audio codec +// +// Copyright (C) 2020, 2022 Cirrus Logic, Inc. and +// Cirrus Logic International Semiconductor Ltd. + +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqchip/irq-cirrus-cs48l32.h> +#include <linux/irqdomain.h> +#include <linux/mfd/cs48l32/core.h> +#include <linux/mfd/cs48l32/registers.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#include "irq-cirrus-cs48l32.h" + +#define CS48L32_IRQ(_irq, _reg) \ + [CS48L32_IRQ_ ## _irq] = { \ + .reg_offset = (_reg) - CS48L32_IRQ1_EINT_1, \ + .mask = CS48L32_ ## _irq ## _EINT1_MASK \ + } + +static const struct regmap_irq cs48l32_irqs[] = { + CS48L32_IRQ(DSP1_IRQ0, CS48L32_IRQ1_EINT_9), + CS48L32_IRQ(DSP1_IRQ1, CS48L32_IRQ1_EINT_9), + CS48L32_IRQ(DSP1_IRQ2, CS48L32_IRQ1_EINT_9), + CS48L32_IRQ(DSP1_IRQ3, CS48L32_IRQ1_EINT_9), + + CS48L32_IRQ(US1_ACT_DET_RISE, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(US1_ACT_DET_FALL, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(US2_ACT_DET_RISE, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(US2_ACT_DET_FALL, CS48L32_IRQ1_EINT_5), + + CS48L32_IRQ(INPUTS_SIG_DET_RISE, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(INPUTS_SIG_DET_FALL, CS48L32_IRQ1_EINT_5), + + CS48L32_IRQ(GPIO1_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO1_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO2_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO2_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO3_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO3_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO4_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO4_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO5_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO5_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO6_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO6_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO7_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO7_FALL, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO8_RISE, CS48L32_IRQ1_EINT_11), + CS48L32_IRQ(GPIO8_FALL, CS48L32_IRQ1_EINT_11), + + CS48L32_IRQ(DRC1_SIG_DET_RISE, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(DRC1_SIG_DET_FALL, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(DRC2_SIG_DET_RISE, CS48L32_IRQ1_EINT_5), + CS48L32_IRQ(DRC2_SIG_DET_FALL, CS48L32_IRQ1_EINT_5), + + CS48L32_IRQ(FLL1_LOCK_RISE, CS48L32_IRQ1_EINT_6), + CS48L32_IRQ(FLL1_LOCK_FALL, CS48L32_IRQ1_EINT_6), + CS48L32_IRQ(FLL1_REF_LOST, CS48L32_IRQ1_EINT_6), + + CS48L32_IRQ(SYSCLK_FAIL, CS48L32_IRQ1_EINT_1), + CS48L32_IRQ(CTRLIF_ERR, CS48L32_IRQ1_EINT_1), + CS48L32_IRQ(SYSCLK_ERR, CS48L32_IRQ1_EINT_1), + CS48L32_IRQ(DSPCLK_ERR, CS48L32_IRQ1_EINT_1), + + CS48L32_IRQ(DSP1_NMI_ERR, CS48L32_IRQ1_EINT_7), + CS48L32_IRQ(DSP1_WDT_EXPIRE, CS48L32_IRQ1_EINT_7), + CS48L32_IRQ(DSP1_MPU_ERR, CS48L32_IRQ1_EINT_7), + + CS48L32_IRQ(BOOT_DONE, CS48L32_IRQ1_EINT_2), +}; + +static const struct regmap_irq_chip cs48l32_irqchip = { + .name = "CS48L32 IRQ", + .status_base = CS48L32_IRQ1_EINT_1, + .mask_base = CS48L32_IRQ1_MASK_1, + .ack_base = CS48L32_IRQ1_EINT_1, + .runtime_pm = true, + .num_regs = 11, + .irqs = cs48l32_irqs, + .num_irqs = ARRAY_SIZE(cs48l32_irqs), +}; + +static int __maybe_unused cs48l32_suspend(struct device *dev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(dev->parent); + + dev_dbg(mfd->irq_dev, "Suspend, disabling IRQ\n"); + + /* + * A runtime resume would be needed to access the chip interrupt + * controller but runtime pm doesn't function during suspend. + * Temporarily disable interrupts until we reach suspend_noirq state. + */ + disable_irq(mfd->irq); + + return 0; +} + +static int __maybe_unused cs48l32_suspend_noirq(struct device *dev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(dev->parent); + + dev_dbg(mfd->irq_dev, "No IRQ suspend, reenabling IRQ\n"); + + /* Re-enable interrupts to service wakeup interrupts from the chip */ + enable_irq(mfd->irq); + + return 0; +} + +static int __maybe_unused cs48l32_resume_noirq(struct device *dev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(dev->parent); + + dev_dbg(mfd->irq_dev, "No IRQ resume, disabling IRQ\n"); + + /* + * We can't handle interrupts until runtime pm is available again. + * Disable them temporarily. + */ + disable_irq(mfd->irq); + + return 0; +} + +static int __maybe_unused cs48l32_resume(struct device *dev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(dev->parent); + + dev_dbg(mfd->irq_dev, "Resume, enabling IRQ\n"); + + /* Interrupts can now be handled */ + enable_irq(mfd->irq); + + return 0; +} + +static const struct dev_pm_ops cs48l32_irq_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(cs48l32_suspend, cs48l32_resume) + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cs48l32_suspend_noirq, cs48l32_resume_noirq) +}; + +static irqreturn_t cs48l32_sysclk_fail(int irq, void *data) +{ + struct cs48l32_mfd *mfd = data; + + dev_warn(mfd->dev, "SYSCLK fail\n"); + + return IRQ_HANDLED; +} + +static irqreturn_t cs48l32_sysclk_error(int irq, void *data) +{ + struct cs48l32_mfd *mfd = data; + + dev_warn(mfd->dev, "SYSCLK error\n"); + + return IRQ_HANDLED; +} + +static irqreturn_t cs48l32_ctrlif_error(int irq, void *data) +{ + struct cs48l32_mfd *mfd = data; + + dev_warn(mfd->dev, "CTRLIF error\n"); + + return IRQ_HANDLED; +} + +static irqreturn_t cs48l32_boot_done(int irq, void *data) +{ + struct cs48l32_mfd *mfd = data; + + dev_dbg(mfd->dev, "BOOT_DONE\n"); + + return IRQ_HANDLED; +} + +static int cs48l32_irq_probe(struct platform_device *pdev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(pdev->dev.parent); + struct irq_data *irq_data; + unsigned int irq_flags; + int ret; + + irq_data = irq_get_irq_data(mfd->irq); + if (!irq_data) + return dev_err_probe(&pdev->dev, -EINVAL, "Invalid IRQ: %d\n", mfd->irq); + + irq_flags = irqd_get_trigger_type(irq_data); + + /* Codec defaults to trigger low, use this if no flags given */ + if (irq_flags == IRQ_TYPE_NONE) + irq_flags = IRQF_TRIGGER_LOW; + + if (irq_flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) + return dev_err_probe(&pdev->dev, -EINVAL, "Host interrupt not level-triggered\n"); + + /* + * The silicon always starts at active-low, check if we need to + * switch to active-high. + */ + if (irq_flags & IRQF_TRIGGER_HIGH) + ret = regmap_clear_bits(mfd->regmap, CS48L32_IRQ1_CTRL_AOD, + CS48L32_IRQ_POL_MASK); + else + ret = regmap_set_bits(mfd->regmap, CS48L32_IRQ1_CTRL_AOD, + CS48L32_IRQ_POL_MASK); + if (ret) + return dev_err_probe(&pdev->dev, ret, "Failed to set IRQ polarity\n"); + + /* + * NOTE: regmap registers this against the OF node of the parent of + * the regmap - that is, against the mfd driver + */ + ret = regmap_add_irq_chip(mfd->regmap, mfd->irq, IRQF_ONESHOT, 0, + &cs48l32_irqchip, &mfd->irq_data); + if (ret) + return dev_err_probe(&pdev->dev, ret, "add_irq_chip failed\n"); + + /* Save dev in parent MFD struct so it is accessible to siblings */ + mfd->irq_dev = &pdev->dev; + + /* + * Log global chip error conditions that aren't specific to + * any particular sibling driver. + */ + cs48l32_request_irq(mfd, CS48L32_IRQ_SYSCLK_FAIL, "SYSCLK fail", + cs48l32_sysclk_fail, mfd); + cs48l32_request_irq(mfd, CS48L32_IRQ_SYSCLK_ERR, "SYSCLK error", + cs48l32_sysclk_error, mfd); + cs48l32_request_irq(mfd, CS48L32_IRQ_CTRLIF_ERR, "CTRLIF error", + cs48l32_ctrlif_error, mfd); + cs48l32_request_irq(mfd, CS48L32_IRQ_BOOT_DONE, "BOOT_DONE", + cs48l32_boot_done, mfd); + + return 0; +} + +static int cs48l32_irq_remove(struct platform_device *pdev) +{ + struct cs48l32_mfd *mfd = dev_get_drvdata(pdev->dev.parent); + + /* + * The IRQ is disabled by the parent MFD driver before + * it starts cleaning up all child drivers + */ + cs48l32_free_irq(mfd, CS48L32_IRQ_BOOT_DONE, mfd); + cs48l32_free_irq(mfd, CS48L32_IRQ_CTRLIF_ERR, mfd); + cs48l32_free_irq(mfd, CS48L32_IRQ_SYSCLK_ERR, mfd); + cs48l32_free_irq(mfd, CS48L32_IRQ_SYSCLK_FAIL, mfd); + + mfd->irq_dev = NULL; + regmap_del_irq_chip(mfd->irq, mfd->irq_data); + + return 0; +} + +static struct platform_driver cs48l32_irq_driver = { + .probe = &cs48l32_irq_probe, + .remove = &cs48l32_irq_remove, + .driver = { + .name = "cs48l32-irq", + .pm = &cs48l32_irq_pm_ops, + } +}; + +module_platform_driver(cs48l32_irq_driver); + +MODULE_DESCRIPTION("CS48L32 IRQ driver"); +MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/irqchip/irq-cirrus-cs48l32.h b/drivers/irqchip/irq-cirrus-cs48l32.h new file mode 100644 index 000000000000..6dae6ddf724d --- /dev/null +++ b/drivers/irqchip/irq-cirrus-cs48l32.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Interrupt support for Cirrus Logic CS48L32 audio codec + * + * Copyright (C) 2020, 2022 Cirrus Logic, Inc. and + * Cirrus Logic International Semiconductor Ltd. + */ + +#ifndef IRQ_CIRRUS_CS48L32_H +#define IRQ_CIRRUS_CS48L32_H + +/* (0x18010) IRQ1_EINT_1 */ +#define CS48L32_DSPCLK_ERR_EINT1_MASK 0x00001000 +#define CS48L32_SYSCLK_ERR_EINT1_MASK 0x00000400 +#define CS48L32_CTRLIF_ERR_EINT1_MASK 0x00000200 +#define CS48L32_SYSCLK_FAIL_EINT1_MASK 0x00000100 + +/* (0x18014) IRQ1_EINT_2 */ +#define CS48L32_BOOT_DONE_EINT1_MASK 0x00000008 + +/* (0x18020) IRQ1_EINT_5 */ +#define CS48L32_US2_ACT_DET_FALL_EINT1_MASK 0x02000000 +#define CS48L32_US2_ACT_DET_RISE_EINT1_MASK 0x01000000 +#define CS48L32_US1_ACT_DET_FALL_EINT1_MASK 0x00800000 +#define CS48L32_US1_ACT_DET_RISE_EINT1_MASK 0x00400000 +#define CS48L32_INPUTS_SIG_DET_FALL_EINT1_MASK 0x00200000 +#define CS48L32_INPUTS_SIG_DET_RISE_EINT1_MASK 0x00100000 +#define CS48L32_DRC2_SIG_DET_FALL_EINT1_MASK 0x00080000 +#define CS48L32_DRC2_SIG_DET_RISE_EINT1_MASK 0x00040000 +#define CS48L32_DRC1_SIG_DET_FALL_EINT1_MASK 0x00020000 +#define CS48L32_DRC1_SIG_DET_RISE_EINT1_MASK 0x00010000 + +/* (0x18024) IRQ1_EINT_6 */ +#define CS48L32_FLL1_REF_LOST_EINT1_MASK 0x00000100 +#define CS48L32_FLL1_LOCK_FALL_EINT1_MASK 0x00000002 +#define CS48L32_FLL1_LOCK_RISE_EINT1_MASK 0x00000001 + +/* (0x18028) IRQ1_EINT_7 */ +#define CS48L32_DSP1_MPU_ERR_EINT1_MASK 0x00200000 +#define CS48L32_DSP1_WDT_EXPIRE_EINT1_MASK 0x00100000 +#define CS48L32_DSP1_IHB_ERR_EINT1_MASK 0x00080000 +#define CS48L32_DSP1_AHB_SYS_ERR_EINT1_MASK 0x00040000 +#define CS48L32_DSP1_AHB_PACK_ERR_EINT1_MASK 0x00020000 +#define CS48L32_DSP1_NMI_ERR_EINT1_MASK 0x00010000 + +/* (0x18030) IRQ1_EINT_9 */ +#define CS48L32_MCU_HWERR_IRQ_OUT_EINT1_MASK 0x80000000 +#define CS48L32_DSP1_IRQ3_EINT1_MASK 0x00000008 +#define CS48L32_DSP1_IRQ2_EINT1_MASK 0x00000004 +#define CS48L32_DSP1_IRQ1_EINT1_MASK 0x00000002 +#define CS48L32_DSP1_IRQ0_EINT1_MASK 0x00000001 + +/* (0x18034) IRQ1_EINT_10 */ +#define CS48L32_CLOCK_DETECT_EINT1_MASK 0x01000000 + +/* (0x18038) IRQ1_EINT_11 */ +#define CS48L32_GPIO8_FALL_EINT1_MASK 0x80000000 +#define CS48L32_GPIO8_RISE_EINT1_MASK 0x40000000 +#define CS48L32_GPIO7_FALL_EINT1_MASK 0x20000000 +#define CS48L32_GPIO7_RISE_EINT1_MASK 0x10000000 +#define CS48L32_GPIO6_FALL_EINT1_MASK 0x08000000 +#define CS48L32_GPIO6_RISE_EINT1_MASK 0x04000000 +#define CS48L32_GPIO5_FALL_EINT1_MASK 0x02000000 +#define CS48L32_GPIO5_RISE_EINT1_MASK 0x01000000 +#define CS48L32_GPIO4_FALL_EINT1_MASK 0x00800000 +#define CS48L32_GPIO4_RISE_EINT1_MASK 0x00400000 +#define CS48L32_GPIO3_FALL_EINT1_MASK 0x00200000 +#define CS48L32_GPIO3_RISE_EINT1_MASK 0x00100000 +#define CS48L32_GPIO2_FALL_EINT1_MASK 0x00080000 +#define CS48L32_GPIO2_RISE_EINT1_MASK 0x00040000 +#define CS48L32_GPIO1_FALL_EINT1_MASK 0x00020000 +#define CS48L32_GPIO1_RISE_EINT1_MASK 0x00010000 + +#endif diff --git a/include/linux/irqchip/irq-cirrus-cs48l32.h b/include/linux/irqchip/irq-cirrus-cs48l32.h new file mode 100644 index 000000000000..c94d31cef96f --- /dev/null +++ b/include/linux/irqchip/irq-cirrus-cs48l32.h @@ -0,0 +1,101 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Interrupt support for Cirrus Logic CS48L32 audio codec + * + * Copyright (C) 2017-2020, 2022 Cirrus Logic, Inc. and + * Cirrus Logic International Semiconductor Ltd. + */ + +#ifndef IRQCHIP_CIRRUS_CS48L32_H +#define IRQCHIP_CIRRUS_CS48L32_H + +#include <linux/interrupt.h> +#include <linux/mfd/cs48l32/core.h> + +/* Main interrupts, organized by priority order - highest first */ +#define CS48L32_IRQ_DSP1_IRQ0 0 +#define CS48L32_IRQ_DSP1_IRQ1 1 +#define CS48L32_IRQ_DSP1_IRQ2 2 +#define CS48L32_IRQ_DSP1_IRQ3 3 +#define CS48L32_IRQ_US1_ACT_DET_RISE 4 +#define CS48L32_IRQ_US1_ACT_DET_FALL 5 +#define CS48L32_IRQ_US2_ACT_DET_RISE 6 +#define CS48L32_IRQ_US2_ACT_DET_FALL 7 +#define CS48L32_IRQ_INPUTS_SIG_DET_RISE 8 +#define CS48L32_IRQ_INPUTS_SIG_DET_FALL 9 +#define CS48L32_IRQ_GPIO1_RISE 10 +#define CS48L32_IRQ_GPIO1_FALL 11 +#define CS48L32_IRQ_GPIO2_RISE 12 +#define CS48L32_IRQ_GPIO2_FALL 13 +#define CS48L32_IRQ_GPIO3_RISE 14 +#define CS48L32_IRQ_GPIO3_FALL 15 +#define CS48L32_IRQ_GPIO4_RISE 16 +#define CS48L32_IRQ_GPIO4_FALL 17 +#define CS48L32_IRQ_GPIO5_RISE 18 +#define CS48L32_IRQ_GPIO5_FALL 19 +#define CS48L32_IRQ_GPIO6_RISE 20 +#define CS48L32_IRQ_GPIO6_FALL 21 +#define CS48L32_IRQ_GPIO7_RISE 22 +#define CS48L32_IRQ_GPIO7_FALL 23 +#define CS48L32_IRQ_GPIO8_RISE 24 +#define CS48L32_IRQ_GPIO8_FALL 25 +#define CS48L32_IRQ_DRC1_SIG_DET_RISE 26 +#define CS48L32_IRQ_DRC1_SIG_DET_FALL 27 +#define CS48L32_IRQ_DRC2_SIG_DET_RISE 28 +#define CS48L32_IRQ_DRC2_SIG_DET_FALL 29 +#define CS48L32_IRQ_FLL1_LOCK_RISE 30 +#define CS48L32_IRQ_FLL1_LOCK_FALL 31 +#define CS48L32_IRQ_FLL1_REF_LOST 32 +#define CS48L32_IRQ_SYSCLK_FAIL 33 +#define CS48L32_IRQ_CTRLIF_ERR 34 +#define CS48L32_IRQ_SYSCLK_ERR 35 +#define CS48L32_IRQ_DSPCLK_ERR 36 +#define CS48L32_IRQ_DSP1_NMI_ERR 37 +#define CS48L32_IRQ_DSP1_WDT_EXPIRE 38 +#define CS48L32_IRQ_DSP1_MPU_ERR 39 +#define CS48L32_IRQ_BOOT_DONE 40 + +struct cs48l32_mfd; + +/* + * These wrapper functions are for use by other child drivers of the + * same parent MFD. + */ +static inline int cs48l32_get_irq_mapping(struct cs48l32_mfd *cs48l32, int irq) +{ + if (!cs48l32->irq_dev) + return -ENODEV; + + return regmap_irq_get_virq(cs48l32->irq_data, irq); +} + +static inline int cs48l32_request_irq(struct cs48l32_mfd *cs48l32, int irq, + const char *name, + irq_handler_t handler, void *data) +{ + irq = cs48l32_get_irq_mapping(cs48l32, irq); + if (irq < 0) + return irq; + + return request_threaded_irq(irq, NULL, handler, IRQF_ONESHOT, name, data); +} + +static inline void cs48l32_free_irq(struct cs48l32_mfd *cs48l32, int irq, void *data) +{ + irq = cs48l32_get_irq_mapping(cs48l32, irq); + if (irq < 0) + return; + + free_irq(irq, data); +} + +static inline int cs48l32_set_irq_wake(struct cs48l32_mfd *cs48l32, int irq, int on) +{ + irq = cs48l32_get_irq_mapping(cs48l32, irq); + if (irq < 0) + return irq; + + return irq_set_irq_wake(irq, on); +} + +#endif
The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable interrupt controller with a variety of interrupt sources, including GPIOs that can be used as interrupt inputs. This driver provides the handling for the interrupt controller. As the codec is accessed via regmap, the generic regmap_irq functionality is used to do most of the work. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- MAINTAINERS | 2 + drivers/irqchip/Kconfig | 3 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-cirrus-cs48l32.c | 281 +++++++++++++++++++++ drivers/irqchip/irq-cirrus-cs48l32.h | 74 ++++++ include/linux/irqchip/irq-cirrus-cs48l32.h | 101 ++++++++ 6 files changed, 462 insertions(+) create mode 100644 drivers/irqchip/irq-cirrus-cs48l32.c create mode 100644 drivers/irqchip/irq-cirrus-cs48l32.h create mode 100644 include/linux/irqchip/irq-cirrus-cs48l32.h