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

Add support for MAX31875 #2600

Closed
wants to merge 3 commits into from

Conversation

jemfgeronimo
Copy link

PR Description

  • This adds driver support and device tree bindings for MAX31875 Temperature Sensor.
  • Datasheet: MAX31875
  • Tested on RPI4 with MAX31875EVKIT

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly (if there is the case)

Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

First round of review from me...

adi,comp-int:
description:
If present interrupt mode is used. If not present comparator mode is used
(default).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe adi,interrupt-enable? But this also brings the question? Don't we need interrupts to be set if IRQ mode is enabled? If so, you could use that standard property to know if IRQs are used or not...

Copy link
Author

Choose a reason for hiding this comment

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

I based this on an existing dt property: adi,comp-int

What this controls is the behavior of a STATUS BIT of the device: whether it will be de-asserted via an I2C read or when going below the hysteresis value. This is documented here.

Maybe, I can just modify the description to better reflect this behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I see... interrupt mode is misleading :)

Copy link
Author

Choose a reason for hiding this comment

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

I will change the description into

  If present, the Overtemperature Status bit operates in interrupt mode. If
  not present, it operates in comparator mode (default).

Documentation/devicetree/bindings/hwmon/adi,max31875.yaml Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
Add documentation for devicetree bindings for max31875

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
MAX31875 Low-Power I2C Temperature Sensor

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
Add entry for the MAX31875 driver.

Signed-off-by: John Erasmus Mari Geronimo <[email protected]>
@jemfgeronimo
Copy link
Author

jemfgeronimo commented Oct 11, 2024

V2

  • added adi,extended-format
  • modified adi,comp-int description
  • changed adi,timeout-enable to adi,timeout-disable
  • changed adi,fault-q from u8 to u32
  • renamed resolution of max31875_data to bit_reso
  • added bool extend_fmt to max31875_data
  • renamed shutdown_write to max31875_shutdown_write
  • removed ugly cast from a function call of max31875_update_bits
  • fixed indentions/alignments
  • refactored max31875_convert_raw_to_temp() and max31875_convert_temp_to_raw()
  • used switch cases to handle the type and attr in max31875_is_visible(), max31875_read() and max31875_write()
  • removed i2c_set_clientdata() call

Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

Some more inputs from me

drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Outdated Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
&dev_attr_temp1_resolution.attr,
NULL
};
ATTRIBUTE_GROUPS(max31875);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It depends, if the mismatches in the register addresses are not that much and the differences overall you can always have a chip_info dedicated struct with the differences. Then no need for conditionals. As a reference look at this:

https://elixir.bootlin.com/linux/v6.11.3/source/drivers/iio/imu/adis16475.c#L900

And see how many devices we support in there. Anyways, up to you... but keep in mind the very same question might pop up upstream... Another thing to have in mind are the DT bindings. Even if you end up with a different driver, you should see if the bindings for the other device could be used for this one (adding the new compatible to the old file and taking care of all differences)

drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
/**
* In the extended format, the MSB is increased from 64C to 128C.
*/
s32 temp = (sign_extend32(raw, 15) * (long)MILLI) >> (extend_fmt ? 7 : 8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the cast to long? I think what you need to reason is that if the multiplication can actually lead to 64bits? If so, you need a cast to s64...

drivers/hwmon/max31875.c Show resolved Hide resolved
drivers/hwmon/max31875.c Show resolved Hide resolved
@nunojsa nunojsa mentioned this pull request Oct 28, 2024
6 tasks
@nunojsa nunojsa closed this Nov 4, 2024
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.

2 participants