Add support for MAX30210 #2544

jemfgeronimo · 2024-07-11T10:05:30Z

PR Description

This adds driver support, device tree bindings, and ABI documentation for added attributes for MAX30210 Temperature Sensor.
Datasheet: MAX30210
Tested on RPI4 with MAX30210EVKIT
Driver was initially done by Daniel Matyas: 8fc25e1
- I've added features such as added events and attributes related to temperature rate-of-change
- From what I've understood from his source code, I've also changed the trigger source of the triggered buffer from CVT/PDB pin to INT pin.

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)

drivers/iio/temperature/max30210.c

nunojsa

Alright, I'm done for a first round of reviewing

nunojsa · 2024-07-25T07:20:32Z

Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

+  adi,comp-mode:
+    description: |
+      If present, comparator mode is used. If not present, interrupt mode is
+      used (default).


Do we need this property? Can't we assume this mode in the absence of an interrupt?

I adapted this from the existing code below:

linux/Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

Line 42 in 5768e80

adi,comp-int:

Comparator mode still uses interrupts. It just uses some register values differently, i.e., low temp alarm is used as high temp hysteresis.
This can also be controlled using debugfs_reg_access; should I just remove it?

Hmm, we can try the DT parameter but I think the description should be improved. More in line with that explanation stating that behavior of the device will change. With the above, it seems that interrupts won't be used at all in comparator mode.

I will use the following description:

If present, the device is in comparator mode. In this mode, the low temperature threshold value acts as high temperature hysteresis. If not present, interrupt mode is used (default).

Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

drivers/iio/temperature/max30210.c

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

jemfgeronimo · 2024-09-12T09:23:25Z

V2

Removed math from macros
Fixed implementations of events that depends on change detect enable (roc-en) and alert mode (comp-int)
Added PWM to implement the multifunction pin CVT/PDB
Moved extra attributes to debugfs
Made adi,int-output-drive-type into string
Fixed indentions
Added register reset in probe()
Changed fwnode_property to device_property
Added powerdown-gpios in dt-bindings
Removed roc-filter from dt-bindings
Renamed dt property adi,comp-mode to adi,comp-int

nunojsa

Some stuff that still needs to be addressed but I would say that this is already in shape to be sent upstream.

Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

MAINTAINERS

nunojsa · 2024-09-23T07:38:21Z

Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml

+  adi,comp-mode:
+    description: |
+      If present, comparator mode is used. If not present, interrupt mode is
+      used (default).


Hmm, we can try the DT parameter but I think the description should be improved. More in line with that explanation stating that behavior of the device will change. With the above, it seems that interrupts won't be used at all in comparator mode.

drivers/iio/temperature/max30210.c

nunojsa · 2024-09-23T08:14:10Z

drivers/iio/temperature/max30210.c

+	if (ret)
+		return ret;
+
+	st->watermark = val;


This needs a lock... Also, should we be allowed to change the watermark while buffering? Would that work out?

If I remember correctly, the hwfifo_set_watermark() runs every after preenable of the triggered buffer. I think
st->watermark does not need a lock in this situation.

drivers/iio/temperature/max30210.c

Add documentation for devicetree bindings for max30210 Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

jemfgeronimo · 2024-10-17T07:29:03Z

V3

removed description of reg property
removed adi,int-output-drive-type property
removed roc_length from debugfs & add adi,roc-len property
improved description of adi,comp-int property
removed Daniel Matyas name in MAINTAINERS file
added __aligned(IIO_DMA_MINALIGN)
added iio_device_claim_direct_mode() in IIO_CHAN_INFO_SAMP_FREQ of max30210_write_raw()
improvement of PWM implementation
added use of __set_bit()
used switch-case instead of if-else in some cases
fixed typo of max30210_debugfs_init()

MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

machschmitt · 2024-10-18T14:44:20Z

drivers/iio/temperature/max30210.c

+static int max30210_read_temp_raw(struct regmap *regmap, unsigned int reg,
+				   int *temp)
+{
+	u8 uval[2] __aligned(IIO_DMA_MINALIGN);


Wouldn't it be better to have the cache aligned buffers declared in struct max30210_state?
Most IIO drivers declare their transfer buffers that need to be aligned in their state structs.

I can do that. Or can I just remove __aligned(IIO_DMA_MINALIGN)? I honestly don't know the use cases of this alignment. I will appreciate an explanation. Thanks.

Not an expert on DMA, but I don't think removing the alignment pragma would be a good thing. I think it is likely that upstream IIO maintainer will ask for explicit buffer alignment to enforce DMA safety. IIRC, the SPI controller in some platforms can talk to the DMA controller and ask SPI transfers to be DMA mapped (rpi can do this IIRC). That would happen seamlessly to the device driver. In those cases, the buffer data can get corrupted if it's not properly aligned on cache lines or miss-handled by the DMA controller. There are other situations in which buffers can get corrupted when data moves along through DMA. Some upstream content about DMA buffer alignment:
https://lore.kernel.org/linux-iio/20240413173409.63d33a0a@jic23-huawei/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
Anyway, I'm not expert on DMA so other reviewers may provide better guided suggestions here.

drivers/iio/temperature/max30210.c

machschmitt · 2024-10-18T16:58:29Z

drivers/iio/temperature/max30210.c

+static void max30210_fifo_read(struct iio_dev *indio_dev)
+{
+	struct max30210_state *st = iio_priv(indio_dev);
+	u8 data[3 * MAX30210_FIFO_SIZE]  __aligned(IIO_DMA_MINALIGN);


Can this buffer reside in the device state struct?

machschmitt · 2024-10-18T17:27:22Z

drivers/iio/temperature/max30210.c

+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read Part ID.\n");
+	if (val != MAX30210_PART_ID)
+		dev_err_probe(dev, -ENODEV, "Part ID mismatch.\n");


Don't know how maintainers are considering warnings from IIO devices but, not long ago, Greg K-H said to avoid kernel warnings https://www.youtube.com/watch?v=Rg_VPMT0XXw.
In sum, everything that can trigger a dev_warn() would be considered a kernel CVE.
If panic_on_warn is enabled, reboot would happen.

jemfgeronimo marked this pull request as ready for review July 11, 2024 12:09

jemfgeronimo requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners July 11, 2024 12:09

jemfgeronimo commented Jul 11, 2024

View reviewed changes

drivers/iio/temperature/max30210.c Outdated Show resolved Hide resolved

nunojsa reviewed Jul 25, 2024

View reviewed changes

jemfgeronimo commented Jul 30, 2024

View reviewed changes

drivers/iio/temperature/max30210.c Outdated Show resolved Hide resolved

jemfgeronimo force-pushed the dev/max30210 branch from 745144f to 9bd27aa Compare September 12, 2024 05:23

iio: Kconfig.adi: imply MAX30210

87685d0

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

jemfgeronimo force-pushed the dev/max30210 branch 4 times, most recently from 82e4345 to 7a24de3 Compare September 12, 2024 08:58

jemfgeronimo requested a review from nunojsa September 12, 2024 09:23

nunojsa reviewed Sep 23, 2024

View reviewed changes

dt-bindings: iio: temperature: add adi,max30210.yaml

5668cc0

Add documentation for devicetree bindings for max30210 Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

jemfgeronimo force-pushed the dev/max30210 branch 5 times, most recently from 41c93cb to 21ffe8b Compare October 17, 2024 06:14

jemfgeronimo force-pushed the dev/max30210 branch from 21ffe8b to 8f1207b Compare October 17, 2024 06:54

iio: temperature: add driver for max30210

a0108de

MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

jemfgeronimo force-pushed the dev/max30210 branch from 8f1207b to a0108de Compare October 17, 2024 07:34

machschmitt reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MAX30210 #2544

Add support for MAX30210 #2544

jemfgeronimo commented Jul 11, 2024 •

edited

Loading

nunojsa left a comment

nunojsa Jul 25, 2024

jemfgeronimo Jul 30, 2024

nunojsa Sep 23, 2024

jemfgeronimo Oct 16, 2024

jemfgeronimo commented Sep 12, 2024 •

edited

Loading

nunojsa left a comment

nunojsa Sep 23, 2024

nunojsa Sep 23, 2024

jemfgeronimo Sep 30, 2024 •

edited

Loading

jemfgeronimo commented Oct 17, 2024 •

edited

Loading

machschmitt Oct 18, 2024

jemfgeronimo Oct 22, 2024

machschmitt Oct 28, 2024

machschmitt Oct 18, 2024

machschmitt Oct 18, 2024

Add support for MAX30210 #2544

Are you sure you want to change the base?

Add support for MAX30210 #2544

Conversation

jemfgeronimo commented Jul 11, 2024 • edited Loading

PR Description

PR Type

PR Checklist

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemfgeronimo commented Sep 12, 2024 • edited Loading

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemfgeronimo Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

jemfgeronimo commented Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemfgeronimo commented Jul 11, 2024 •

edited

Loading

jemfgeronimo commented Sep 12, 2024 •

edited

Loading

jemfgeronimo Sep 30, 2024 •

edited

Loading

jemfgeronimo commented Oct 17, 2024 •

edited

Loading