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

projects/S905: Delete cputemp #5

Merged
merged 1 commit into from Oct 17, 2016
Merged

projects/S905: Delete cputemp #5

merged 1 commit into from Oct 17, 2016

Conversation

CGarces
Copy link

@CGarces CGarces commented Oct 16, 2016

File not need, contains the same code than packages/mediacenter/kodi/scripts/cputemp

@CGarces
Copy link
Author

CGarces commented Oct 16, 2016

Current cputemp contains same code than packages/mediacenter/kodi/scripts/cputemp

  # used on some intel systems
  if [ "$TEMP" = "0" -a -f /sys/devices/virtual/thermal/thermal_zone0/temp ]; then
    TEMP=`cat /sys/devices/virtual/thermal/thermal_zone0/temp`
  fi

  # used on RaspberryPi
  if [ "$TEMP" = "0" -a -f /sys/class/thermal/thermal_zone0/temp ]; then
    TEMP=`cat /sys/class/thermal/thermal_zone0/temp`
  fi

In any case not is working on S905X

@kszaq
Copy link
Owner

kszaq commented Oct 17, 2016

The script is not 100% the same as the project-specific one doesn't divide returned temperature value by 1000.

On the other hand, you are correct that this file is not needed as Amlogic 3.14 kernel returns temperature value in millicelsius.

I have 2 requests to this PR: please also delete Kodi patch for temperature scaling and reword commit message to include "projects/S905". Thanks.

If you want to make temperature sensor work, you can enable it in kernel config. It will work for S905X but with S905 the only thing you get is spammed kmsg - that's the reason why I don't enable it for my builds.

File not need, contains the same code than packages/mediacenter/kodi/scripts/cputemp
@CGarces CGarces changed the title Delete cputemp projects/S905: Delete cputemp Oct 17, 2016
@kszaq kszaq merged commit 2dd2f5b into kszaq:libreelec-7.0 Oct 17, 2016
@kszaq
Copy link
Owner

kszaq commented Oct 17, 2016

Thank you!

@CGarces CGarces deleted the patch-1 branch October 17, 2016 20:21
@CGarces
Copy link
Author

CGarces commented Oct 18, 2016

Works fine on S905X after compile again the kernel.
Maybe it's possible filter the cputemp output based on the /proc/cpuinfo (020a on S905X) please confirm this approach because I don't have a S905 device.

@kszaq
Copy link
Owner

kszaq commented Oct 18, 2016

The issue with S905 is not in cputemp - it's in kernel thermal driver.

@CGarces
Copy link
Author

CGarces commented Oct 18, 2016

Let me understand the problem.

You not compile that option in the kernel because get incorrect information for S905, but works fine for S905X

My proposed solution is compile the kernel with the temperature sensor, and create a new cputemp that only return values for the revision 020a that I assume that should match only with S905X

Something like this (I haven tested the code sorry I'll do latter)

#Hack for S905x
VERSION=cat /proc/cpuinfo | grep "Revision" | sort -u | cut -d: -f2
if [ "$VERSION" == "020a" ]; then
    TEMP=`cat /sys/class/thermal/thermal_zone0/temp`
fi

This will get a value at least for S905X, fix the kernel thermal driver is out of my skills. But I need to confirm that the "Revision" approach is the correct way to identify the S905X over the S905

@kszaq
Copy link
Owner

kszaq commented Oct 18, 2016

No. The issue is that if you compile kernel with TEMP_SENSOR enabled, kernel log is spammed with thermal driver messages. It is not a big issue for a user but for me it makes debugging very unpleasant. I may disable thermal driver for S905 in platform_init but I would prefer to have temperature sensor fixed.

Wilhansen pushed a commit to Wilhansen/LibreELEC.tv that referenced this pull request Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants