Re: [alsa-devel] [PATCH 1/2] ASoC: Add support for Conexant CX2072X CODEC
On Mon, 06 May 2019 06:26:25 +0200, Mark Brown wrote:
On Fri, May 03, 2019 at 09:18:26AM +0200, Takashi Iwai wrote:
On Fri, 03 May 2019 08:47:29 +0200, Mark Brown wrote:
On Thu, May 02, 2019 at 09:04:06AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
This looks *very* much like board configuration rather than a patch - there's no kind of test bit and the comments talk specifically about
OK, will replace with the straight regmap_multi_reg_write().
That's probably not addressing the issue, a lot of that stuff just doesn't seem like it should be in some fixed configuration table at all.
So what's your alternative suggestion?
Make it either platform data or runtime controls depending on the specific thing being configured. Though...
Oh, joy. What's the story here? Do you have a datasheet for the part?
Not at all. I just refreshed the already submitted patches since I have a laptop with the codec. I tried to contact Conexant, but in vain, so I decided to submit the renewed one.
...this is not going to help. At the very least it needs to be abundantly clear to anyone reading the code tht any magic register write sequences like this are being done the way they are due to a badly documented part from an unsupportive vendor rather than because it's a good idea in case someone decides to use the driver as an example. Some comments or something. For safety the bits that look board specific like the pinmuxing should ideally be behind DMI checks, though if nobody else ever bought the CODEC that's probably not so important.
Fair enough. I'm going to put more comments on that part.
Since this is the only code for this codec on the net and all existing platforms (Intel devices, RPi extensions and else) seem working with it -- i.e. all these depend on the same init sequence more or less, so splitting to platform data won't help much in practice.
The jack detection in ASoC is anyway a bit funky, especially when involved with PM...
What do you mean here? I'm not aware of any issues and the systems I've worked with seemed robust...
There are tons of different ways of implementation for jack controls, with different API usages. IOW, no consistency.
The board registration interfaces do vary as the hardware isn't very standard so there's a bunch of things you can do for configuration (though there is a generic interface for the 90% case, the main limitation these days on using it is Intel's lack of any sensible firmware interface). I'm not aware of any issues with power management though?
Yeah, I understand the various use cases and coverage for a wide range of hardware implementations, but the lack of the documentation is... not a thing we can be proud of.
Regarding the PM, my statement wasn't straight, sorry. The fact is that ASoC manages so many different levels of (a sort of) "PM". We have the Linux device-object system and runtime PMs, and we have BIAS level control. The machine needed the jack handling via ACPI GPIO IRQ, that is outside the codec, which looks rather like a rare case, and I had hard time to look for the right choice of the API usage.
+EXPORT_SYMBOL_GPL(snd_soc_cx2072x_get_jack_state);
Why is this symbol exported?
It's called from the machine driver. snd_soc_jack_add_gpios() is called in the machine driver side, and it needs the jack_status_check callback that calls this function.
That code shouldn't be in the machine driver, the CODEC driver should request any interrupts it needs itself.
The similar things are done on many other Intel SST board drivers. The current patch just follows the pattern.
The only example I can find is the max98095 which it just plain buggy in this regard, that example shouldn't be followed - it looks like it's just plain an oversight in review. Frankly the export was done by me and looks like I was just rushing to fix up a build breakage.
This is rather a question how generic the codec driver should be written. I changed the code in v5 patchset to embed the jack_gpio stuff inside the codec driver side rather than the machine driver, so the whole exported functions can be reduced now. But, of course, it slightly gives more implicit assumption about the hardware implementations, too. Though, the existing code seems to have already fixed gpio / pin setups, so the other setups wouldn't have worked, in anyway.
No, that's not the question. The question is why there is any initialization at all?
Again, no idea. These are likely no default values of the hardware registers, and we need to set up some. I *guess* ditto for the initial register table in the above, too.
Or it's just a taste setting someone had for their particular system. With the earlier register default table an awful lot of it is fairly obvious board specific configuration, doing things like pinmuxing which will vary with the board.
It's unlikely there are *no* default values - you'd kind of have to try to have a specific register range like this with genuinely floating values. Given that the code for configuring the EQ was broken to IIRC never take effect I'd not be 100% surprisd if someone couldn't figure out why their settings weren't taking effect and just bodged something directly in the driver.
Actually I'm fine to drop the whole EQ stuff that brings lots of black magic. Certainly it'll benefit us for code simplification. Let's see.
thanks,
Takashi
On Mon, May 06, 2019 at 11:55:37AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
On Fri, May 03, 2019 at 09:18:26AM +0200, Takashi Iwai wrote:
comments or something. For safety the bits that look board specific like the pinmuxing should ideally be behind DMI checks, though if nobody else ever bought the CODEC that's probably not so important.
Since this is the only code for this codec on the net and all existing platforms (Intel devices, RPi extensions and else) seem working with it -- i.e. all these depend on the same init sequence more or less, so splitting to platform data won't help much in practice.
Yeah, like I say that's a bit more nice to have given that it looks like there's basically one system using the thing.
The board registration interfaces do vary as the hardware isn't very standard so there's a bunch of things you can do for configuration (though there is a generic interface for the 90% case, the main limitation these days on using it is Intel's lack of any sensible firmware interface). I'm not aware of any issues with power management though?
Yeah, I understand the various use cases and coverage for a wide range of hardware implementations, but the lack of the documentation is... not a thing we can be proud of.
The bits that vary are driver specific interfaces so any sort of general documentation is pretty hard. The core stuff all at least has kerneldoc which is about average here.
Regarding the PM, my statement wasn't straight, sorry. The fact is that ASoC manages so many different levels of (a sort of) "PM". We have the Linux device-object system and runtime PMs, and we have BIAS level control. The machine needed the jack handling via ACPI GPIO IRQ, that is outside the codec, which looks rather like a rare case, and I had hard time to look for the right choice of the API usage.
The reason you're not finding much handling for interrupt GPIOs is that if something is an interrupt it should be going through the kernel interrupt interfaces, even if the pin that is providing the interrupt has GPIO support the code should just ignore that and let the frameworks deal with things. The only exception that comes up is that there's a few devices that manually emulate level triggered interrupts with GPIOs and edge triggered interrupts for cases where controllers only provide level triggers, unfortunately nobody had the time/enthusiasm to push that emulation into the interrupt core but fortunately few hardware designers are implementing edge triggered interrupt controllers these days.
This is rather a question how generic the codec driver should be written. I changed the code in v5 patchset to embed the jack_gpio stuff inside the codec driver side rather than the machine driver, so the whole exported functions can be reduced now. But, of course, it slightly gives more implicit assumption about the hardware implementations, too. Though, the existing code seems to have already fixed gpio / pin setups, so the other setups wouldn't have worked, in anyway.
Like I say if the device is using the fact that the pin is a GPIO there's quite likely something wrong - that shouldn't be something that the user of an interrupt should need to see.
It's unlikely there are *no* default values - you'd kind of have to try to have a specific register range like this with genuinely floating values. Given that the code for configuring the EQ was broken to IIRC never take effect I'd not be 100% surprisd if someone couldn't figure out why their settings weren't taking effect and just bodged something directly in the driver.
Actually I'm fine to drop the whole EQ stuff that brings lots of black magic. Certainly it'll benefit us for code simplification. Let's see.
Probably worth checking to make sure the default EQ setup isn't too awful (though I guess the EQ is just turned off by default so it'll just be an uncorrected speaker/headphone which should be fine if not ideal).
On Mon, 06 May 2019 16:05:06 +0200, Mark Brown wrote:
This is rather a question how generic the codec driver should be written. I changed the code in v5 patchset to embed the jack_gpio stuff inside the codec driver side rather than the machine driver, so the whole exported functions can be reduced now. But, of course, it slightly gives more implicit assumption about the hardware implementations, too. Though, the existing code seems to have already fixed gpio / pin setups, so the other setups wouldn't have worked, in anyway.
Like I say if the device is using the fact that the pin is a GPIO there's quite likely something wrong - that shouldn't be something that the user of an interrupt should need to see.
Yeah, unfortunately we have no reference, so the only chance would be to test with another board that has a different setup.
It's unlikely there are *no* default values - you'd kind of have to try to have a specific register range like this with genuinely floating values. Given that the code for configuring the EQ was broken to IIRC never take effect I'd not be 100% surprisd if someone couldn't figure out why their settings weren't taking effect and just bodged something directly in the driver.
Actually I'm fine to drop the whole EQ stuff that brings lots of black magic. Certainly it'll benefit us for code simplification. Let's see.
Probably worth checking to make sure the default EQ setup isn't too awful (though I guess the EQ is just turned off by default so it'll just be an uncorrected speaker/headphone which should be fine if not ideal).
A good point. I checked the status now, and found that actually the EQ and others won't appear in the normal playback / capture via HiFi route, but only through DSP. Since the normal usages with UCM profile goes only via HiFi, EQ/DRC don't play any role. So it's OK to remove them, at least, for normal PC usages, as well as RPi HiFi extensions, as it seems.
thanks,
Takashi
On Mon, May 06, 2019 at 05:26:51PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
Like I say if the device is using the fact that the pin is a GPIO there's quite likely something wrong - that shouldn't be something that the user of an interrupt should need to see.
Yeah, unfortunately we have no reference, so the only chance would be to test with another board that has a different setup.
The GPIO/IRQ equivalence thing should just be a purely Linux internal thing - looking at the driver it appears that there's no need to look at the GPIO status, Linux can tell if something is plugged in purely by reading the chip registers so the jack status function could directly be the interrupt handler.
On Wed, 08 May 2019 10:10:06 +0200, Mark Brown wrote:
On Mon, May 06, 2019 at 05:26:51PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
Like I say if the device is using the fact that the pin is a GPIO there's quite likely something wrong - that shouldn't be something that the user of an interrupt should need to see.
Yeah, unfortunately we have no reference, so the only chance would be to test with another board that has a different setup.
The GPIO/IRQ equivalence thing should just be a purely Linux internal thing - looking at the driver it appears that there's no need to look at the GPIO status, Linux can tell if something is plugged in purely by reading the chip registers so the jack status function could directly be the interrupt handler.
But it can't see the button status or detect the headset type without reading the codec's magic registers... So some help from the codec side is required, and the irq handler isn't in the codec side because there is no i2c irq assigned but only ACPI gpio, as far as I checked.
thanks,
Takashi
On Wed, May 08, 2019 at 10:16:06AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
The GPIO/IRQ equivalence thing should just be a purely Linux internal thing - looking at the driver it appears that there's no need to look at the GPIO status, Linux can tell if something is plugged in purely by reading the chip registers so the jack status function could directly be the interrupt handler.
But it can't see the button status or detect the headset type without reading the codec's magic registers... So some help from the codec side is required, and the irq handler isn't in the codec side because
The whole thing should be in the CODEC side.
there is no i2c irq assigned but only ACPI gpio, as far as I checked.
You can map through an ACPI GPIO to an interrupt without worrying about the fact that it's mapped as a GPIO in ACPI IIRC - if you can't there's lots of other drivers are going to have issues.
On Wed, 08 May 2019 10:59:53 +0200, Mark Brown wrote:
On Wed, May 08, 2019 at 10:16:06AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
The GPIO/IRQ equivalence thing should just be a purely Linux internal thing - looking at the driver it appears that there's no need to look at the GPIO status, Linux can tell if something is plugged in purely by reading the chip registers so the jack status function could directly be the interrupt handler.
But it can't see the button status or detect the headset type without reading the codec's magic registers... So some help from the codec side is required, and the irq handler isn't in the codec side because
The whole thing should be in the CODEC side.
there is no i2c irq assigned but only ACPI gpio, as far as I checked.
You can map through an ACPI GPIO to an interrupt without worrying about the fact that it's mapped as a GPIO in ACPI IIRC - if you can't there's lots of other drivers are going to have issues.
But what gives a benefit? It needs more plumbing between codec and machine driver. The i2c probe doesn't give the irq, so you'd need and extra stuff to enable the irq in a different route if you'd need to implement the handler in the codec driver.
Actually, my latest patchset already eliminates the exported stuff by moving to set_jack callback like some other drivers do. If you have an idea for further simplification / fixes, let me know.
I haven't submitted because of the merge window. The patch is found at topic/soc-cx2072x-5.2 branch, the commit is https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=to...
thanks,
Takashi
On Wed, May 08, 2019 at 11:16:18AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
there is no i2c irq assigned but only ACPI gpio, as far as I checked.
You can map through an ACPI GPIO to an interrupt without worrying about the fact that it's mapped as a GPIO in ACPI IIRC - if you can't there's lots of other drivers are going to have issues.
But what gives a benefit? It needs more plumbing between codec and machine driver. The i2c probe doesn't give the irq, so you'd need and extra stuff to enable the irq in a different route if you'd need to implement the handler in the codec driver.
Handling the interrupt entirely within the CODEC driver *reduces* the coupling between the CODEC and machine drivers, making it more possible to use the standard set_jack() interface and generic machine drivers with the CODEC drivers.
Actually looking a little I think you may need some ACPI specific parsing code in the probe function to look up the GPIO but that should just be like all the other DMI quirking stuff, hidden in the CODEC driver. It's really sad how bad a job Intel have done of making firmware interfaces for their audio hardware. I had thought people had managed to hide all that stuff but I'm not seeing the code right now.
Actually, my latest patchset already eliminates the exported stuff by moving to set_jack callback like some other drivers do. If you have an idea for further simplification / fixes, let me know.
I haven't submitted because of the merge window. The patch is found at topic/soc-cx2072x-5.2 branch, the commit is https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=to...
I'm on a train with intermittent network connectivity right now (and getting near to my destination too)...
participants (2)
-
Mark Brown -
Takashi Iwai