Support several firmware versions. Flip or read bits instead of whole bytes for some parameters. Some new parameters.

[teackot]

It's a big rewrite. The main feature: the driver now supports different EC registers configurations (read: different firmware versions) and determines the suitable one during initialization. Configurations are hardcoded (for now?) for my, @BeardOverflow's and @ThePBone's laptops.

I also noticed that some on/off parameters only need one bit switched/read to be changed/read. In different firmwares those bits may have the same index, but other bits in the byte may be different and serve some other purpose. Who knows what other bits do, better not touch them.

I also added some new features from MControlCenter and msi-ec-modern.

Edit: also adds support for GS66 Stealth 11UE (16V4EMS1), see #9

[BeardOverflow]

Thanks for your efforts. I appreciate so much your work.
To continue, see the points in the review. Feel free to open a debate to review them in depth.

[rottenpants466]

@BeardOverflow Sorry for hijaking this merge request (it's related somewhat).

Have you seen Hans de Goede @jwrdegoede post asking people for help in order to add certain MSI features to be built right into the kernel?

dmitry-s93/MControlCenter#78

Again, sorry for hijaking this.

[jwrdegoede]

I just took a quick look at this with my upstream drivers/platform/x86 maintainer hat on.

I like the direction this is headed in!

Once this has landed I would welcome an upstream submission of the resulting msi-ec driver. Although if this is going to be submitted upstream it should probably be divided into smaller bits.

For example start with submitting a driver which does use the new "struct msi_ec_conf" way of working and you could even have an unmodified registers_configuration.h in there including all the "struct msi_ec_conf" initialization from the current msi-ec.c, but strip out most of the code actually exporting functionality to user space, except for lets say the battery-charge thresholds (for the first patch in the series).

And then have follow-up patches building on top of that exporting more functionality to userspace. For easy merging it would be best to start with patches which adds functionality with well defined userspace interfaces, such as e.g. LED class devices, input devices (if relevant), etc.

And then later on add functionality which adds custom sysfs attributes. The patches adding new userspace API in the form of custom sysfs attributes are going to need the most upstream review / discussion.

[teackot]

This is awesome!

I hope this PR will be merged soon, though the repo owner seems to be offline lately. Also I still want to implement some things, e.g. only create attributes supported by the laptop.

Once this has landed I would welcome an upstream submission of the resulting msi-ec driver

Do I understand the process correctly: clone the kernel repo -> add the driver -> build and test the kernel -> create the patches?

such as e.g. LED class devices

About that, they don't seem to work as intended. I can trigger them manually but muting the sound or microphone doesn't trigger them. Neither do they work if I set the trigger to BAT1-charging or disk-read.

[teackot]

A note regarding some parameters I found

I found an issue saying that switching performance mode on MSI Modern 15 changes [0x79] and [0x91] addresses. 0x46 seems to be the lowest value written into them.

My MSI Prestige 14 has both of those addresses set to 0x46 so I assume they represent the same parameters. I tried setting them to different values while monitoring CPU voltage, GPU power states and clock speeds, but I didn't notice any changes.

The same issue says that switching Super Battery mode off/on sets [0xeb] to 0x80 / 0x8f (on Modern 15). I don't remember having Super Battery mode on my P14 on windows, but I still tried toggling different registers that contain 0x80. No luck. So I assume either (most likely) P14 doesn't support this mode or it has a different signature.

I will add Super Battery mode for those laptops that support it.

[teackot]

I did some research:

Right now we have "off" as one of the shift modes with a value of 0x80. Turns out the "off" shift mode does not exist. Look at this graph:

It shows the relationship between CPU voltage and the selected mode. As you can see, each time the "off" mode is selected the voltage stays the same. This graph also shows that "comfort" and "sport" modes exist as they both bump the voltage up from the "eco" level.

Now, all of the mode values start with a C digit: 0xc2, 0xc1, 0xc0 and 0xc4. Writing 0xc0 into a register containing 0x80 sets the 6th bit. So I assumed that unless you have a 1 in the 6th bit - the mode won't be changed.

And yes, it is true! The 6th bit is like a gate: if it is closed (0) - the mode won't be set; if it is open (1) - it will set the mode. You can freely write any value into the register as long as the higher half stays 0x8 - and the mode will stay intact.

When your laptop turns off or goes to sleep, it resets the register to 0x80 and resets the mode to some default value. Judging by the voltage, it's either comfort or sport.

And there is also a problem. This mode parameter (which is btw only a part of the dragon center shift mode switch) seems to be responsible for the voltage but I can't find any persistent differences between comfort, sport and turbo voltage graphs:

(columns are almost in the middle of each interval and represent the average voltage)

[jwrdegoede]

I hope this PR will be merged soon, though the repo owner seems to be offline lately. Also I still want to implement some things, e.g. only create attributes supported by the laptop.

Note if @BeardOverflow is not responsive then this does not need to be merged here for upstream submission. If you are happy with the state of the code in your own fork, then you can submit that upstream without needing to wait for it being merged here.

With that said having it had an extra pair of eyes look at it before upstream review (as part of merging it here) of course is a good idea if possible, but this is not a blocker.

Do I understand the process correctly: clone the kernel repo -> add the driver -> build and test the kernel -> create the patches?

Yes clone the master branch from:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/

Then build + install that without your driver first to get a working base. Once you have it working add a (minimal, e.g. battery thresholds only) version of the driver and integrate that into drivers/platform/x86/Kconfig and drivers/platform/x86/Makefile .

Once you have that working, commit it and then expose some more functionality to userspace, commit that, etc. And then submit a patch series upstream following: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Note please use git send-email to send the patches, using regular mail clients tends to mess up the patches because they make whitespace changes, wrap lines, etc. If you want you can first send the series as a test to just me at hdegoede@redhat.com . Then I can check if everything is correct wrt the kernel patch submission process side of things; and after that you can then re-send the series with the platform-driver-x86@vger.kernel.org mailinglist in the Cc, so that we can do patch-review on the public-list.

such as e.g. LED class devices

About that, they don't seem to work as intended. I can trigger them manually but muting the sound or microphone doesn't trigger them. Neither do they work if I set the trigger to BAT1-charging or disk-read.

For mic / spk mute LEDs to work they also need some integration on the sound system side. I assume you are already setting the default trigger for the LED class device to "audio-micmute" resp. "audio-mute" ?

If manually controlling the LEDs by echoing to their brightness under /sys/class/leds works and the default triggers are set as above and toggling the mic / spk mute does not properly control the LEDs then some work is needed on the audio driver side. In that case please report this to the alsa mailinglist: alsa-devel@alsa-project.org with Jaroslav Kysela perex@perex.cz in the Cc. Jaroslav is the expert on making the audio drivers actually set the "audio-micmute" resp. "audio-mute" triggers.

I believe that things like the BAT and disk driver also need support on the battery-monitoring / disk driver side and I guess that at least the ACPI battery driver is missing this because normally battery LEDs on x86/ACPI laptops are directly controlled through ACPI / the EC and not controlled via the LED class subsystem.

[sainak]

For mic / spk mute LEDs to work they also need some integration on the sound system side. I assume you are already setting the default trigger for the LED class device to "audio-micmute" resp. "audio-mute" ?

If manually controlling the LEDs by echoing to their brightness under /sys/class/leds works and the default triggers are set as above and toggling the mic / spk mute does not properly control the LEDs then some work is needed on the audio driver side. In that case please report this to the alsa mailinglist: alsa-devel@alsa-project.org with Jaroslav Kysela perex@perex.cz in the Cc. Jaroslav is the expert on making the audio drivers actually set the "audio-micmute" resp. "audio-mute" triggers.

Yes the triggers work as expected when I manually register the sound cards to the snd_ctl_led sysfs by doing this:

modprobe snd_ctl_led
echo -n name="Speaker Playback Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
echo -n name="Headphone Playback Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
echo -n name="Capture Switch" > /sys/class/sound/card1/controlC1/led-mic/attach
echo done

[BeardOverflow]

@teackot
I answered you about my previous comments. Please, check it out!

@rottenpants466 @jwrdegoede @sainak Thanks to all for joining to discuss in this issue. Until today, I was not aware of your answers. Thankfully, I was pinned privately by @FaridZelli

Actually, (IMHO) I see a relevant TODO before trying to merge into the kernel. It is fan speed configuration:
For auto, does not need any additional configuration.
For basic, you can configure 1 threshold level for cpu and gpu fan speed and it already is implemented.
For advanced, you can configure 6 threshold levels for cpu and gpu fan speeds, but they are not implemented.

For example, MControlCenter already does it by own way (see the 4nd image). Nowadays, it is not possible with the current implementation of msi-ec. I am waiting to solve the previous comments in the review (points 3, 4 and 5), merging this PR and working again about the updated codebase.

[teackot]

I answered you about my previous comments. Please, check it out!

Thanks! Those are just some small changes and I'll do them right now.

Actually, (IMHO) I see a relevant TODO before trying to merge into the kernel. It is fan speed configuration

We don't need to merge it all at once, and:

For easy merging it would be best to start with patches which adds functionality with well defined userspace interfaces, such as e.g. LED class devices, input devices (if relevant), etc.

we first need too merge power subsystem and LED class devices, as @jwrdegoede stated.

So we have a plenty of time to implement advanced fan profile. We'll have to decide how to expose it to the userspace, but let's leave it for the future discussion after this PR gets merged

[teackot]

Yes the triggers work as expected when I manually register the sound cards to the snd_ctl_led sysfs

Can confirm

[teackot]

All done!

[BeardOverflow]

You have finished all pending code reviews, so I can finally merge your branch. Thanks a lot @teackot

So we have a plenty of time to implement advanced fan profile. We'll have to decide how to expose it to the userspace, but let's leave it for the future discussion after this PR gets merged

Ok, I have enabled "Discussion" tab as a place to continue speaking.