| Message ID | cover.1701733441.git.soyer@irl.hu |
|---|---|
| Headers | show |
| Series | ALSA: hda/tas2563: Add tas253 HDA driver | expand |
>>> + >>> +static void tas2563_fixup_i2c(struct hda_codec *cdc, >>> + const struct hda_fixup *fix, int action) >>> +{ >>> + tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866"); >> >> Any specific reason to use an Intel ACPI identifier? Why not use >> "TIAS2563" ? >> > INT8866 is in the ACPI. > I don't know why Lenovo uses this name. > I think it's more internal than intel. > > Scope (_SB.I2CD) > { > Device (TAS) > { > Name (_HID, "INT8866") // _HID: Hardware ID Ouch, I hope they checked with Intel that this isn't an HID already in use... > Name (_UID, Zero) // _UID: Unique ID > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource > Settings > { > Name (RBUF, ResourceTemplate () > { > I2cSerialBusV2 (0x004C, ControllerInitiated, > 0x00061A80, > AddressingMode7Bit, "\\_SB.I2CD", > 0x00, ResourceConsumer, , Exclusive, > ) > I2cSerialBusV2 (0x004D, ControllerInitiated, > 0x00061A80, > AddressingMode7Bit, "\\_SB.I2CD", > 0x00, ResourceConsumer, , Exclusive, > ) > GpioInt (Edge, ActiveLow, SharedAndWake, PullNone, > 0x0000, > "\\_SB.GPIO", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0020 > } > }) > Return (RBUF) /* \_SB_.I2CD.TAS_._CRS.RBUF */ > } > > Method (_STA, 0, NotSerialized) // _STA: Status > { > Return (0x0F) > } > } > } >>> +static int tas2563_system_resume(struct device *dev) >>> +{ >>> + int ret; >>> + struct tas2563_data *tas2563 = dev_get_drvdata(dev); >>> + >>> + dev_dbg(tas2563->dev, "System Resume\n"); >>> + >>> + ret = pm_runtime_force_resume(dev); >>> + if (ret) >>> + return ret; >>> + >>> + for (int i = 0; i < tas2563->ndev; ++i) >>> + tas2563_tasdev_setup(tas2563, &tas2563->tasdevs[i]); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct dev_pm_ops tas2563_hda_pm_ops = { >>> + SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume) >> >> where's the pm_runtime stuff? >> > > The amp stores its state in software shutdown mode. > The tas2563_hda_playback_hook wakes/shutdowns the amp, not the > pm_runtime. My point was that you have all these pm_runtime_ calls in the code, but nothing that provides pm_runtime suspend-resume functions so not sure what exactly the result is?
>>>> +static const struct dev_pm_ops tas2563_hda_pm_ops = { >>>> + SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume) >>> >>> where's the pm_runtime stuff? >>> >> >> The amp stores its state in software shutdown mode. >> The tas2563_hda_playback_hook wakes/shutdowns the amp, not the >> pm_runtime. > > My point was that you have all these pm_runtime_ calls in the code, but > nothing that provides pm_runtime suspend-resume functions so not sure > what exactly the result is? if the inspiration was the tas2781, then see below it does have a RUNTIME_PM_OPS line as well as runtime_suspend/resume routines. static const struct dev_pm_ops tas2781_hda_pm_ops = { RUNTIME_PM_OPS(tas2781_runtime_suspend, tas2781_runtime_resume, NULL) SYSTEM_SLEEP_PM_OPS(tas2781_system_suspend, tas2781_system_resume) };
On Tue, Dec 05, 2023 at 10:01:08AM -0600, Pierre-Louis Bossart wrote: > >>> +static const struct dev_pm_ops tas2563_hda_pm_ops = { > >>> + SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume) > >> where's the pm_runtime stuff? > > The amp stores its state in software shutdown mode. > > The tas2563_hda_playback_hook wakes/shutdowns the amp, not the > > pm_runtime. > My point was that you have all these pm_runtime_ calls in the code, but > nothing that provides pm_runtime suspend-resume functions so not sure > what exactly the result is? It *could* be ACPI doing something I guess...
On Tue, 2023-12-05 at 10:01 -0600, Pierre-Louis Bossart wrote: > > > > + > > > > +static void tas2563_fixup_i2c(struct hda_codec *cdc, > > > > + const struct hda_fixup *fix, int action) > > > > +{ > > > > + tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866"); > > > > > > Any specific reason to use an Intel ACPI identifier? Why not use > > > "TIAS2563" ? > > > > > INT8866 is in the ACPI. > > I don't know why Lenovo uses this name. > > I think it's more internal than intel. > > > > Scope (_SB.I2CD) > > { > > Device (TAS) > > { > > Name (_HID, "INT8866") // _HID: Hardware ID > > Ouch, I hope they checked with Intel that this isn't an HID already in > use... > It looks the INT prefix is not reserved. (yet) https://uefi.org/ACPI_ID_List?acpi_search=INT > > > > > > + return 0; > > > > +} > > > > + > > > > +static const struct dev_pm_ops tas2563_hda_pm_ops = { > > > > + SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume) > > > > > > where's the pm_runtime stuff? > > > > > > > The amp stores its state in software shutdown mode. > > The tas2563_hda_playback_hook wakes/shutdowns the amp, not the > > pm_runtime. > > My point was that you have all these pm_runtime_ calls in the code, but > nothing that provides pm_runtime suspend-resume functions so not sure > what exactly the result is? > > I think nothing. I haven't experienced anything unusual recently.
>>>>> +static void tas2563_fixup_i2c(struct hda_codec *cdc, >>>>> + const struct hda_fixup *fix, int action) >>>>> +{ >>>>> + tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866"); >>>> >>>> Any specific reason to use an Intel ACPI identifier? Why not use >>>> "TIAS2563" ? >>>> >>> INT8866 is in the ACPI. >>> I don't know why Lenovo uses this name. >>> I think it's more internal than intel. >>> >>> Scope (_SB.I2CD) >>> { >>> Device (TAS) >>> { >>> Name (_HID, "INT8866") // _HID: Hardware ID >> >> Ouch, I hope they checked with Intel that this isn't an HID already in >> use... >> > It looks the INT prefix is not reserved. (yet) > https://uefi.org/ACPI_ID_List?acpi_search=INT It's been de-facto reclaimed by Intel over the years, apparently using INTC or INTL was too hard for some of my colleagues... There are lots of INT devices in the kernel today, here's a small list for sound/soc/codecs only rt274.c: { "INT34C2", 0 }, rt286.c: { "INT343A", 0 }, rt298.c: { "INT343A", 0 }, ssm4567.c: { "INT343B", 0 }, Those INT values were added by Intel teams though, it's really odd to see Lenovo use an INT-based HID. Should really use 104C2563 or something. >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct dev_pm_ops tas2563_hda_pm_ops = { >>>>> + SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume) >>>> >>>> where's the pm_runtime stuff? >>>> >>> >>> The amp stores its state in software shutdown mode. >>> The tas2563_hda_playback_hook wakes/shutdowns the amp, not the >>> pm_runtime. >> >> My point was that you have all these pm_runtime_ calls in the code, but >> nothing that provides pm_runtime suspend-resume functions so not sure >> what exactly the result is? >> >> > I think nothing. I haven't experienced anything unusual recently. you can probably see from the /sys directory what the pm_runtime power state is, most likely the status is 'unknown'.
On 12/5/2023 6:22 PM, Pierre-Louis Bossart wrote: > > >>>>>> +static void tas2563_fixup_i2c(struct hda_codec *cdc, >>>>>> + const struct hda_fixup *fix, int action) >>>>>> +{ >>>>>> + tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866"); >>>>> >>>>> Any specific reason to use an Intel ACPI identifier? Why not use >>>>> "TIAS2563" ? >>>>> Will just note that prefix should probably be TXNW (not TIAS) as discussed recently on list. >>>> INT8866 is in the ACPI. >>>> I don't know why Lenovo uses this name. >>>> I think it's more internal than intel. >>>> >>>> Scope (_SB.I2CD) >>>> { >>>> Device (TAS) >>>> { >>>> Name (_HID, "INT8866") // _HID: Hardware ID >>> >>> Ouch, I hope they checked with Intel that this isn't an HID already in >>> use... >>> >> It looks the INT prefix is not reserved. (yet) >> https://uefi.org/ACPI_ID_List?acpi_search=INT > > It's been de-facto reclaimed by Intel over the years, apparently using > INTC or INTL was too hard for some of my colleagues... > Perhaps it should be reserved then, so it is present on above list? > There are lots of INT devices in the kernel today, here's a small list > for sound/soc/codecs only > > rt274.c: { "INT34C2", 0 }, > rt286.c: { "INT343A", 0 }, > rt298.c: { "INT343A", 0 }, > ssm4567.c: { "INT343B", 0 }, > > Those INT values were added by Intel teams though, it's really odd to > see Lenovo use an INT-based HID. Should really use 104C2563 or something. I will just note that those RT ones are used on quite old RVPs, and yes I would have also preferred if they had used "real" IDs, but it is unlikely that anyone fixes it after all this time ;). Adding Andy to CC, as he commented recently about problematic assignments of ACPI IDs on this list, maybe he can shed some light on the "INT" prefix.
Please disregard this patch. The tas2781-hda driver will handle this. https://lore.kernel.org/all/cover.1701906455.git.soyer@irl.hu/ On Tue, 2023-12-05 at 00:45 +0100, Gergo Koteles wrote: > The ta2563 is a smart amplifier. Similar to tas2562 but with DSP. Some > Lenovo laptops have it to drive the bass speakers. By default, it is in > software shutdown state. > > To make the DSP work it needs a firmware and some calibration data. > The latter can be read from the EFI in Lenovo laptops. > > For the correct configuration it needs additional register data. > It captured after running the Windows driver. > > The firmware can be extracted as TAS2563Firmware.bin from the Windows > driver with innoextract. > https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe > > The driver will search for it as TAS2563-17AA3870.bin with the 14ARB7. > > It uses the default program/configuration, and has no controls for these yet. > > The amplifier works without firmware, but I don't know how safe is it, > that's why the firmware is required. > > Gergo Koteles (2): > ASoc: tas2563: DSP Firmware loading support > ALSA: hda/tas2563: Add tas2563 HDA driver > > {sound/soc/codecs => include/sound}/tas2562.h | 8 + > include/sound/tas25xx-dsp.h | 100 ++++ > sound/pci/hda/Kconfig | 14 + > sound/pci/hda/Makefile | 2 + > sound/pci/hda/patch_realtek.c | 22 +- > sound/pci/hda/tas2563_hda_i2c.c | 508 ++++++++++++++++++ > sound/soc/codecs/Kconfig | 7 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tas2562.c | 2 +- > sound/soc/codecs/tas25xx-dsp.c | 282 ++++++++++ > 10 files changed, 942 insertions(+), 5 deletions(-) > rename {sound/soc/codecs => include/sound}/tas2562.h (90%) > create mode 100644 include/sound/tas25xx-dsp.h > create mode 100644 sound/pci/hda/tas2563_hda_i2c.c > create mode 100644 sound/soc/codecs/tas25xx-dsp.c > > > base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
On Wed, Dec 06, 2023 at 05:07:16PM +0100, Amadeusz Sławiński wrote: > On 12/5/2023 6:22 PM, Pierre-Louis Bossart wrote: ... > > > > > > > + tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866"); > > > > > > > > > > > > Any specific reason to use an Intel ACPI identifier? Why not use > > > > > > "TIAS2563" ? > > Will just note that prefix should probably be TXNW (not TIAS) as discussed > recently on list. ...which should come directly from TI as it's their responsibility to allocate an ACPI ID. ... > > > > > INT8866 is in the ACPI. > > > > > I don't know why Lenovo uses this name. > > > > > I think it's more internal than intel. This is wrong (PNP) ID. ... > > > > > Name (_HID, "INT8866") // _HID: Hardware ID > > > > > > > > Ouch, I hope they checked with Intel that this isn't an HID already in > > > > use... > > > > > > > It looks the INT prefix is not reserved. (yet) > > > https://uefi.org/ACPI_ID_List?acpi_search=INT You are looking into wrong registry, and yeah, Intel used wrong PNP ID for years... > > It's been de-facto reclaimed by Intel over the years, apparently using > > INTC or INTL was too hard for some of my colleagues... > > Perhaps it should be reserved then, so it is present on above list?
The ta2563 is a smart amplifier. Similar to tas2562 but with DSP. Some Lenovo laptops have it to drive the bass speakers. By default, it is in software shutdown state. To make the DSP work it needs a firmware and some calibration data. The latter can be read from the EFI in Lenovo laptops. For the correct configuration it needs additional register data. It captured after running the Windows driver. The firmware can be extracted as TAS2563Firmware.bin from the Windows driver with innoextract. https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe The driver will search for it as TAS2563-17AA3870.bin with the 14ARB7. It uses the default program/configuration, and has no controls for these yet. The amplifier works without firmware, but I don't know how safe is it, that's why the firmware is required. Gergo Koteles (2): ASoc: tas2563: DSP Firmware loading support ALSA: hda/tas2563: Add tas2563 HDA driver {sound/soc/codecs => include/sound}/tas2562.h | 8 + include/sound/tas25xx-dsp.h | 100 ++++ sound/pci/hda/Kconfig | 14 + sound/pci/hda/Makefile | 2 + sound/pci/hda/patch_realtek.c | 22 +- sound/pci/hda/tas2563_hda_i2c.c | 508 ++++++++++++++++++ sound/soc/codecs/Kconfig | 7 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tas2562.c | 2 +- sound/soc/codecs/tas25xx-dsp.c | 282 ++++++++++ 10 files changed, 942 insertions(+), 5 deletions(-) rename {sound/soc/codecs => include/sound}/tas2562.h (90%) create mode 100644 include/sound/tas25xx-dsp.h create mode 100644 sound/pci/hda/tas2563_hda_i2c.c create mode 100644 sound/soc/codecs/tas25xx-dsp.c base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa