| Message ID | 20230512122838.243002-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add cs42l43 PC focused SoundWire CODEC | expand |
On 12/05/2023 18:18, Charles Keepax wrote: > On Fri, May 12, 2023 at 05:25:52PM +0200, Krzysztof Kozlowski wrote: >> On 12/05/2023 14:28, Charles Keepax wrote: >>> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface >>> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed >>> for portable applications. It provides a high dynamic range, stereo >>> DAC for headphone output, two integrated Class D amplifiers for >> >> ... >> >>> + >>> + interrupt-controller: true >>> + >>> + '#interrupt-cells': >>> + const: 2 >> >> Hm, are you sure? Who is the consumer/user of this interrupt controller? >> > > Anyone who wants the device has GPIOs that can signal IRQs. Some > of the other IRQs could be more generally useful, such as some of > the jack detection ones. OK, makes sense, but it is a bit odd then to have: codec { which is GPIO and interrupt controller, but not pin controller pinctrl { pin controller, which is not GPIO and not interrupt controller } } Maybe all the GPIO/pin/related interrupt properties should be moved to pinctrl node? Best regards, Krzysztof
On Fri, May 12, 2023 at 04:42:33PM +0000, Charles Keepax wrote: > I guess if Mark doesn't mind I think the only internal bit of the > device that uses the IRQs is the CODEC driver so I could move the > IRQ handling in there, it does seem a little odd to me, but I > guess I don't have any problems with it. The obvious (and previously standard) place for it would be the MFD.
On Mon, May 15, 2023 at 10:08:41AM +0900, Mark Brown wrote: > On Fri, May 12, 2023 at 04:42:33PM +0000, Charles Keepax wrote: > > > I guess if Mark doesn't mind I think the only internal bit of the > > device that uses the IRQs is the CODEC driver so I could move the > > IRQ handling in there, it does seem a little odd to me, but I > > guess I don't have any problems with it. > > The obvious (and previously standard) place for it would be the MFD. Alright I certainly have no objection to moving it there, although would be good to get Lee's thoughts, make sure he is happy with that too. Thanks, Charles
On Fri, 12 May 2023, Marc Zyngier wrote: > On Fri, 12 May 2023 16:39:33 +0100, > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > On Fri, May 12, 2023 at 04:10:05PM +0100, Marc Zyngier wrote: > > > On Fri, 12 May 2023 13:28:35 +0100, > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > > > > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > > > > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > > > > for portable applications. It provides a high dynamic range, stereo > > > > DAC for headphone output, two integrated Class D amplifiers for > > > > loudspeakers, and two ADCs for wired headset microphone input or > > > > stereo line input. PDM inputs are provided for digital microphones. > > > > > > > > The IRQ chip provides IRQ functionality both to other parts of the > > > > cs42l43 device and to external devices that wish to use its IRQs. > > > > > > Sorry, but this isn't much of an interrupt controller driver. A modern > > > interrupt controller driver is firmware-driven (DT or ACPI, pick your > > > poison), uses irq domains, and uses the irqchip API. > > > > > > > Apologies but I really need a little help clarifying the issues > > here. I am totally happy to fix things up but might need a couple > > pointers. > > > > 1) uses the irqchip API / uses irq domains > > > > The driver does use both the irqchip API and domains, what > > part of the IRQ API are we not using that we should be? > > > > The driver registers an irq domain using > > irq_domain_create_linear. It requests its parent IRQ using > > request_threaded_irq. It passes IRQs onto the devices requesting > > IRQs from it using handle_nested_irq and irq_find_mapping. > > > > Is the objection here that regmap is making these calls for us, > > rather than them being hard coded into this driver? > > That's one of the reasons. Look at the existing irqchip drivers: they > have nothing in common with yours. The regmap irqchip abstraction may > be convenient for what you are doing, but the result isn't really an > irqchip driver. It is something that is a small bit of a larger device > and not an interrupt controller driver on its own. The irqchip > subsystem is there for "first class" interrupt controllers. I'm not aware of another subsystem that deals with !IRQChip level IRQ controllers. Where do simple or "second class" interrupt controllers go?
On Fri, 12 May 2023 13:28:28 +0100, Charles Keepax wrote: > This patch chain adds support for the Cirrus Logic cs42l43 PC focused > SoundWire CODEC. Some supporting work is included in the chain, > including adding an ASoC control notification helper function and > adding support for IRQs generated by the in-band SoundWire alert > mechanism. > > The chain is currently based of v6.4-rc1 because I am not 100% sure > which tree we want to send everything through. The CODEC support > has a build dependency on both the SoundWire change and the ASoC > soc-component change. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [02/10] ASoC: soc-component: Add notify control helper function commit: ace9ed54bd874f2c63723b13b1747f6463e2587e [03/10] ASoC: ak4118: Update to use new component control notify helper commit: 476d942e50d4f22d8621a18612dd6cfbf0a5d1d9 [04/10] ASoC: wm_adsp: Update to use new component control notify helepr commit: 95d06196c83c9dc1b6fd6cda07a1bac54ca2d568 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
On Mon, 15 May 2023 12:25:54 +0100, Lee Jones <lee@kernel.org> wrote: > > On Fri, 12 May 2023, Marc Zyngier wrote: > > > On Fri, 12 May 2023 16:39:33 +0100, > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > > > On Fri, May 12, 2023 at 04:10:05PM +0100, Marc Zyngier wrote: > > > > On Fri, 12 May 2023 13:28:35 +0100, > > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > > > > > > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > > > > > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > > > > > for portable applications. It provides a high dynamic range, stereo > > > > > DAC for headphone output, two integrated Class D amplifiers for > > > > > loudspeakers, and two ADCs for wired headset microphone input or > > > > > stereo line input. PDM inputs are provided for digital microphones. > > > > > > > > > > The IRQ chip provides IRQ functionality both to other parts of the > > > > > cs42l43 device and to external devices that wish to use its IRQs. > > > > > > > > Sorry, but this isn't much of an interrupt controller driver. A modern > > > > interrupt controller driver is firmware-driven (DT or ACPI, pick your > > > > poison), uses irq domains, and uses the irqchip API. > > > > > > > > > > Apologies but I really need a little help clarifying the issues > > > here. I am totally happy to fix things up but might need a couple > > > pointers. > > > > > > 1) uses the irqchip API / uses irq domains > > > > > > The driver does use both the irqchip API and domains, what > > > part of the IRQ API are we not using that we should be? > > > > > > The driver registers an irq domain using > > > irq_domain_create_linear. It requests its parent IRQ using > > > request_threaded_irq. It passes IRQs onto the devices requesting > > > IRQs from it using handle_nested_irq and irq_find_mapping. > > > > > > Is the objection here that regmap is making these calls for us, > > > rather than them being hard coded into this driver? > > > > That's one of the reasons. Look at the existing irqchip drivers: they > > have nothing in common with yours. The regmap irqchip abstraction may > > be convenient for what you are doing, but the result isn't really an > > irqchip driver. It is something that is a small bit of a larger device > > and not an interrupt controller driver on its own. The irqchip > > subsystem is there for "first class" interrupt controllers. > > I'm not aware of another subsystem that deals with !IRQChip level IRQ > controllers. Where do simple or "second class" interrupt controllers > go? This isn't an interrupt controller. This is internal signalling, local to a single component that has been artificially broken into discrete bits, including an interrupt controller. The only *real* interrupts here are the GPIOs. I'm happy to see an interrupt controller for the GPIOs. But the rest is just internal muck that doesn't really belong here. Where should it go? Together with the rest of the stuff that manages the block as a whole. Which looks like the MFD subsystem to me. Thanks, M.
On Mon, 15 May 2023, Charles Keepax wrote: > On Mon, May 15, 2023 at 10:08:41AM +0900, Mark Brown wrote: > > On Fri, May 12, 2023 at 04:42:33PM +0000, Charles Keepax wrote: > > > > > I guess if Mark doesn't mind I think the only internal bit of the > > > device that uses the IRQs is the CODEC driver so I could move the > > > IRQ handling in there, it does seem a little odd to me, but I > > > guess I don't have any problems with it. > > > > The obvious (and previously standard) place for it would be the MFD. > > Alright I certainly have no objection to moving it there, although > would be good to get Lee's thoughts, make sure he is happy with > that too. Submit a patch and we'll take it from there.
On Tue, 16 May 2023, Marc Zyngier wrote: > On Mon, 15 May 2023 12:25:54 +0100, > Lee Jones <lee@kernel.org> wrote: > > > > On Fri, 12 May 2023, Marc Zyngier wrote: > > > > > On Fri, 12 May 2023 16:39:33 +0100, > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > > > > > On Fri, May 12, 2023 at 04:10:05PM +0100, Marc Zyngier wrote: > > > > > On Fri, 12 May 2023 13:28:35 +0100, > > > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > > > > > > > > > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface > > > > > > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed > > > > > > for portable applications. It provides a high dynamic range, stereo > > > > > > DAC for headphone output, two integrated Class D amplifiers for > > > > > > loudspeakers, and two ADCs for wired headset microphone input or > > > > > > stereo line input. PDM inputs are provided for digital microphones. > > > > > > > > > > > > The IRQ chip provides IRQ functionality both to other parts of the > > > > > > cs42l43 device and to external devices that wish to use its IRQs. > > > > > > > > > > Sorry, but this isn't much of an interrupt controller driver. A modern > > > > > interrupt controller driver is firmware-driven (DT or ACPI, pick your > > > > > poison), uses irq domains, and uses the irqchip API. > > > > > > > > > > > > > Apologies but I really need a little help clarifying the issues > > > > here. I am totally happy to fix things up but might need a couple > > > > pointers. > > > > > > > > 1) uses the irqchip API / uses irq domains > > > > > > > > The driver does use both the irqchip API and domains, what > > > > part of the IRQ API are we not using that we should be? > > > > > > > > The driver registers an irq domain using > > > > irq_domain_create_linear. It requests its parent IRQ using > > > > request_threaded_irq. It passes IRQs onto the devices requesting > > > > IRQs from it using handle_nested_irq and irq_find_mapping. > > > > > > > > Is the objection here that regmap is making these calls for us, > > > > rather than them being hard coded into this driver? > > > > > > That's one of the reasons. Look at the existing irqchip drivers: they > > > have nothing in common with yours. The regmap irqchip abstraction may > > > be convenient for what you are doing, but the result isn't really an > > > irqchip driver. It is something that is a small bit of a larger device > > > and not an interrupt controller driver on its own. The irqchip > > > subsystem is there for "first class" interrupt controllers. > > > > I'm not aware of another subsystem that deals with !IRQChip level IRQ > > controllers. Where do simple or "second class" interrupt controllers > > go? > > This isn't an interrupt controller. This is internal signalling, local > to a single component that has been artificially broken into discrete > bits, including an interrupt controller. The only *real* interrupts > here are the GPIOs. > > I'm happy to see an interrupt controller for the GPIOs. But the rest > is just internal muck that doesn't really belong here. Where should it You should have been a poet! =;-) > go? Together with the rest of the stuff that manages the block as a > whole. Which looks like the MFD subsystem to me. Very well. Let's see this "muck" in a patch please!
On Tue, 16 May 2023 11:09:36 +0100, Lee Jones <lee@kernel.org> wrote: > > On Tue, 16 May 2023, Marc Zyngier wrote: > > > I'm happy to see an interrupt controller for the GPIOs. But the rest > > is just internal muck that doesn't really belong here. Where should it > > You should have been a poet! =;-) Who says I'm not? M.
On Tue, May 16, 2023 at 11:09:36AM +0100, Lee Jones wrote: > On Tue, 16 May 2023, Marc Zyngier wrote: > > On Mon, 15 May 2023 12:25:54 +0100, > > Lee Jones <lee@kernel.org> wrote: > > > On Fri, 12 May 2023, Marc Zyngier wrote: > > > > On Fri, 12 May 2023 16:39:33 +0100, > > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > > I'm not aware of another subsystem that deals with !IRQChip level IRQ > > > controllers. Where do simple or "second class" interrupt controllers > > > go? > > > > This isn't an interrupt controller. This is internal signalling, local > > to a single component that has been artificially broken into discrete > > bits, including an interrupt controller. The only *real* interrupts > > here are the GPIOs. > > I would question this statement a little, they are fixed function IRQs sure but they are still real interrupts. These are lines which receive a signal and on an edge they set a stick status bit, which causes another signal to generate an edge, they have registers which let you mask events, if it walks like a duck and all. The only difference between this and a "real" interrupt is whether the chip designer or the board designer was the person who decided where the wire was connected. > > I'm happy to see an interrupt controller for the GPIOs. But the rest > > is just internal muck that doesn't really belong here. Where should it Internal-ish, granted many of them are primarily useful to the device itself. But it is very easy to construct situations where say knowing the speaker thermals are high, or that a jack has been inserted are useful outside of the CODEC driver itself. > > go? Together with the rest of the stuff that manages the block as a > > whole. Which looks like the MFD subsystem to me. > > Very well. Let's see this "muck" in a patch please! Groovy I will do a re-spin moving the IRQ stuff to the MFD and lets see where we get to. Thank you all for your help in reviewing this so far. Thanks, Charles