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

[Edgecore][as4224/as4564] Modify fan driver to support sensors command #222

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brandonchuang
Copy link
Contributor

@brandonchuang brandonchuang commented May 30, 2023

Currently, fan information is not shown with the 'sensors' command.
This patch will migrate the fan folder under 'hwmon',
so that sensors can detect the fan and display its information.

sensors output:
as4224_fan-isa-0000
Adapter: ISA adapter
fan1: 8587 RPM (min = 0 RPM, max = 20500 RPM)
fan2: 8667 RPM (min = 0 RPM, max = 20500 RPM)
fan3: 8667 RPM (min = 0 RPM, max = 20500 RPM)
fan4: 8587 RPM (min = 0 RPM, max = 20500 RPM)
fan5: 8746 RPM (min = 0 RPM, max = 20500 RPM)
fan6: 8746 RPM (min = 0 RPM, max = 20500 RPM)

as4564_fan-isa-0000
Adapter: ISA adapter
fan1: 1470 RPM (min = 0 RPM, max = 10670 RPM)
fan2: 1510 RPM (min = 0 RPM, max = 10670 RPM)

Changes are listed below:

  • Add min/max fan attributes
  • Create sysfs attributes under hwmon folder
  • Modify fani.c/sysi.c accordingly

Signed-off-by: Brandon Chuang [email protected]

Copy link
Contributor

@paulmenzel paulmenzel left a comment

Choose a reason for hiding this comment

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

Thank you for the patch.

Signed-off-by: brandonchuang [email protected]

Please fix your git setup to spell your name: Brandon Chuang.

Lastly, please amend the commit message to describe the current behavior of sensors, and how it behaves after your fix.

@@ -75,15 +75,15 @@ onlp_fani_get_fan_attr_int(int fid, char *fmt, int *value)
{
char attr_name[32] = {0};
sprintf(attr_name, fmt, fid);
return onlp_file_read_int(value, "%s%s", FAN_SYSFS_PATH, attr_name);
return onlp_file_read_int(value, "%s*%s", FAN_SYSFS_PATH, attr_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the asterisk needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is the asterisk needed?

The path is migrated under the 'hwmon' folder.
Without the asterisk, opening the file will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excuse my ignorance, can you please give an example for the old and the new path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excuse my ignorance, can you please give an example for the old and the new path?

Old path:
/sys/devices/platform/as4224_fan/

New path:
/sys/devices/platform/as4224_fan/hwmon/hwmon5/

hwmon"5" could represent any other number, such as hwmon1 or hwmon6.
The index starts from 1, and the number is determined by the order in which the driver is attached to hwmon.

@brandonchuang brandonchuang force-pushed the as4224_as4564_sensors branch 3 times, most recently from c580ca3 to 9f33fed Compare June 1, 2023 01:59
Comment on lines 545 to 546
data->hwmon_dev = hwmon_device_register_with_info(&pdev->dev,
DRVNAME, NULL, NULL, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI passing NULL as chip_info (fourth parameter) is considered wrong in newer kernels (5.19 and later), as of commit torvalds/linux@ddaefa2. Doing so may prevent moving to newer LTS kernels or require rewriting the driver.

The old hwmon_device_register() still exists and works though as of Linux 6.4, you just need to live with the warning about the deprecated API at boot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI passing NULL as chip_info (fourth parameter) is considered wrong in newer kernels (5.19 and later), as of commit torvalds/linux@ddaefa2. Doing so may prevent moving to newer LTS kernels or require rewriting the driver.

The old hwmon_device_register() still exists and works though as of Linux 6.4, you just need to live with the warning about the deprecated API at boot.

Just replace hwmon_device_register_with_info() with hwmon_device_register_with_groups()

Currently, fan information is not shown with the 'sensors' command.
This patch will migrate the fan folder under 'hwmon',
so that sensors can detect the fan and display its information.

sensors output:
as4224_fan-isa-0000
Adapter: ISA adapter
fan1:        8587 RPM  (min =    0 RPM, max = 20500 RPM)
fan2:        8667 RPM  (min =    0 RPM, max = 20500 RPM)
fan3:        8667 RPM  (min =    0 RPM, max = 20500 RPM)
fan4:        8587 RPM  (min =    0 RPM, max = 20500 RPM)
fan5:        8746 RPM  (min =    0 RPM, max = 20500 RPM)
fan6:        8746 RPM  (min =    0 RPM, max = 20500 RPM)

as4564_fan-isa-0000
Adapter: ISA adapter
fan1:        1470 RPM  (min =    0 RPM, max = 10670 RPM)
fan2:        1510 RPM  (min =    0 RPM, max = 10670 RPM)

Changes are listed below:
- Add min/max fan attributes
- Create sysfs attributes under hwmon folder
- Modify fani.c/sysi.c accordingly

Signed-off-by: Brandon Chuang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants