Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Akoya E Models hwdb #14296

Merged
merged 1 commit into from Dec 17, 2019
Merged

Akoya E Models hwdb #14296

merged 1 commit into from Dec 17, 2019

Conversation

cvoinf
Copy link
Contributor

@cvoinf cvoinf commented Dec 9, 2019

In reply to #14272 , #14272 (comment)

@hadess
Copy link
Contributor

hadess commented Dec 11, 2019

Please squash the commits as appropriate, this isn't easily reviewable.

Copy link
Contributor

@hadess hadess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the changes as needed.

I still don't know what the patch is trying to fix either.

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 11, 2019
@cvoinf
Copy link
Contributor Author

cvoinf commented Dec 15, 2019

I'm sorry, I forgot how to squash the changes and don't have the time right now to read me into git. Please, can you do this for me? Thank you. I guess for you it's made in 2 minutes, I will have to read at least an hour and afterwards it's perhaps wrong again.

To explain, what my suggestion is about:
When I bought these two laptops, I first was disappointed that Linux did not rotate the screen automatically. But after a while I found out how to change the hwdb so that my laptops were correctly recognized by systemd.
What I realized was that all Akoya models beginning with Exxxx (up to now) have the same Matrix. So I propose to change the file 60-sensor.hwdb that all other Akoya E-models get the same matrix, since it is more likely that other models beginning with an E have the same matrix than that they don't have this one but the one (out of many possible permutations of that matrix) that is chosen if their exact fingerprint is not found in the list.

Alternatively you could add the lines for my two laptops:

# Medion Akoya E3222
sensor:modalias:acpi:KIOX010A*:dmi:*:svnMEDION:pnMEDION*:*
ACCEL_MOUNT_MATRIX=0, -1, 0; -1, 0, 0; 0, 0, 1

And the other one:

# Medion Akoya E2293
sensor:modalias:acpi:KIOX010A*:dmi:*:svnMEDION:pnE2293MD61144*:*
ACCEL_MOUNT_MATRIX=0, -1, 0; -1, 0, 0; 0, 0, 1

But with my first proposal, perhaps many owners of other E-models not in the list up to now are made happy; at least the probability is higher for that than for the possibility that somebody gets harmed.

@cvoinf cvoinf requested a review from hadess December 15, 2019 22:45
@cvoinf cvoinf closed this Dec 15, 2019
@cvoinf cvoinf reopened this Dec 15, 2019
@keszybz
Copy link
Member

keszybz commented Dec 16, 2019

Please squash the commits as appropriate, this isn't easily reviewable.

There's always the "Files changed" tab that shows a single diff for all commits. And for merging, "squash and merge" DTRT. So for commits like this, that should be just one commit, we don't need to ask contributors to squash. OTOH, when the PR is larger and we want to have multiple commits, then asking the contributor to rebase makes sense.

What I realized was that all Akoya models beginning with Exxxx (up to now) have the same Matrix. So I propose to change the file 60-sensor.hwdb that all other Akoya E-models get the same matrix

That is harder to judge. @hadess what do you think?

@keszybz keszybz added squash-on-merge needs-discussion 🤔 and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 16, 2019
@hadess
Copy link
Contributor

hadess commented Dec 16, 2019

Please squash the commits as appropriate, this isn't easily reviewable.

There's always the "Files changed" tab that shows a single diff for all commits. And for merging, "squash and merge" DTRT. So for commits like this, that should be just one commit, we don't need to ask contributors to squash. OTOH, when the PR is larger and we want to have multiple commits, then asking the contributor to rebase makes sense.

My time to work on this is limited. If you have time to try and untangle this, make sense of the changes, write a new commit message, then please go for it. I think that if the OP doesn't care to spend the time, maybe you can. I can't.

@keszybz
Copy link
Member

keszybz commented Dec 17, 2019

OK, let's merge this then. If it turns out that the assumption that other Medion Akoya models are dissimilar, we will have to revert.

Since up to now all known Akoya E* models have the same Matrix, we assume all
other Akoya E* models work the same.
@keszybz keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed squash-on-merge labels Dec 17, 2019
@poettering poettering merged commit 3d86465 into systemd:master Dec 17, 2019
@cvoinf
Copy link
Contributor Author

cvoinf commented Dec 18, 2019

Thank you, guys!

From keszybz I got an email, saying it would be nice to comment the line

+sensor:modalias:acpi:KIOX010A*:dmi::svnMEDION:pnMEDION:*

I'm sorry not to be so familiar with github that I know where to comment it best (perhaps in the source code, but that would mean we have to restart this procedure again...), so I'll try it here.

One Laptop is named Akoya E2293 MD61144. It's output is:

cat /sys/class/dmi/id/modalias

dmi:bvnAmericanMegatrendsInc.:bvrGeminiLake_YS11G_V1.0.21:bd10/10/2018:svnMEDION:pnE2293MD61144:pvrELAN:rvnMEDION:rnYS11G:rvrDefaultstring:cvnMEDION:ct31:cvrDefaultstring:

The other one is named Akoya E3222 MD62450. It's output is:

cat /sys/class/dmi/id/modalias

dmi:bvnAmericanMegatrendsInc.:bvrGeminiLake_YS13G_V1.0.21:bd10/10/2018:svnMEDION:pnMEDION:pvrNONE:rvnMEDION:rnMEDION:rvrDefaultstring:cvnMEDION:ct10:cvrDefaultstring:

As you can see, the last output has this svnMEDION:pnMEDION -string inside. It has the same Matrix.

Thank's again for helping!

@cvoinf cvoinf deleted the patch-1 branch December 18, 2019 18:47
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Dec 19, 2019
@keszybz
Copy link
Member

keszybz commented Dec 19, 2019

@cvoinf thanks. I added that comment when squashing your PR, so it's all fine now.

@poettering
Copy link
Member

Hmm, seems this wasn't entirely correct, see #14437

@hadess
Copy link
Contributor

hadess commented Jan 9, 2020

Hmm, seems this wasn't entirely correct, see #14437

Narrator: it was correct.

keszybz added a commit to keszybz/systemd that referenced this pull request Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants