On Tue, Feb 1, 2022 at 10:40 AM Mark Brown broonie@kernel.org wrote:
On Mon, Jan 31, 2022 at 10:39:00AM -0800, Curtis Malainey wrote:
I feel instead of reverting this complete patch we can quickly submit a new patch set with "gpio_spk_en = NONE" for maxim codec case. As codec driver is anyhow controlling that gpio we don't need to do it from machine driver. We've already done that here https://patchwork.kernel.org/project/alsa-devel/patch/20220131203225.1418648...
I'm pretty sure the proper solution is to shove this logic into the alc1019 driver like it is in the max98357 driver. The header is already there for gpio which makes me think it was planned but forgotten. Otherwise everyone else who uses this codec and associated pin will have to duplicate this logic in their machine driver.
In general if there's something like a speaker amplifier that's just controlled via GPIO I'd expect that to be something that's set up by the machine driver. If the CODEC is a GPIO provider then the pattern should be that the CODEC registers with gpiolib and then the machine driver uses that GPIO, that way the GPIO can get used for any other purpose and if a system picks another GPIO for whatever reason then that'll just work.
Just to confirm, by provider, you mean it has on codec gpio it is exposing to the kernel correct? Interestingly, this appears to be counter to the max98357a.c driver. It has the SDMODE pin which can put it into a low power state. The codec driver appears to consume this pin from FW lookup and toggle it on trigger. I am just curious why we would not want the codec to handle its own pins? That way the control logic for the pin is collected into a non-chipset dependent location that is close to the internal state of the driver.
This gets more annoying for ACPI systems due to their lack of standards based enumeration of course, the endemic reliance on board specific quirks causes breakage all over - it looks like this is just an example of some ACPI systems using firmware description and some not. Are the systems that need the hard coding here shipping, for example if the Windows drivers rely on such hard coding rather than enumeration from ACPI? If we need the quirking then the fix isn't just a simple revert, we need to do something that ensures that the support for all the different systems plays nicely with each other.
The board this patch fixes is not shipping, the board it breaks is planned to ship from my understanding. Eric, feel free to correct me. We could do a partial revert and remove the _NP fields and both boards would work (Sujith already sent a patch for this "ASoC: amd: acp: Set gpio_spkr_en to None for max speaker amplifer in machine driver") but I think we should still consider a patch to stop hard coding the GPIO as it should be available via a lookup.