Add support for MAX14906 and MAX14916 industrial IO with diagnostics #74891

bogdanovs · 2024-06-24T14:52:14Z

Add MAX14906 and MAX14916 drivers with diagnostics

MAX14906 in 4 channel I/O with advanced diagnostic.
In SPI communication diagnostic status transmitted on every
READ/WRITE which includes generic status of chip.
Configuration both on global level and on per channel bases.

Tested with modified basic/blinky and basic/button examples.

MAX14916 Industrial 8 channel output with advanced diagnostics.
Allowing giagnostic configuration both on per channel or global bases
In SPI communication diagnostic status transmitted on every
READ/WRITE which includes generic status of chip.

Tested with modified basic/blinky example.

Both chips have advanced diagnostics which include:

Overload detection
Open wire detection
Short to VDD
Above VDD
Com Err
VDD warning

drivers/gpio/Kconfig.max14906

drivers/gpio/gpio_max149x6.h

dts/bindings/gpio/adi,max14906-gpio.yaml

MaureenHelm · 2024-08-02T23:09:24Z

dts/bindings/gpio/adi,max14906-gpio.yaml

+    type: boolean
+  spi-addr:
+    type: int
+    default: 0


Need to explain in the description why the default value was selected
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#dt-bindings-default-rules

Same for the following properties below

Thanks , explanation is added

This also needs to be done for the properties below

Added default values are from documentation - default value per register

Actually, why is this property needed? It's redundant to the reg property

I think using reg for that purpose will be kind of confusing.
spi-addr is something specific to MAX14906 and MAX14916.

drivers/gpio/Kconfig.max14916

drivers/gpio/gpio_max14916.h

dts/bindings/gpio/adi,max14916-gpio.yaml

MaureenHelm · 2024-08-02T23:13:16Z

dts/bindings/gpio/adi,max14916-gpio.yaml

+    type: boolean
+  spi-addr:
+    type: int
+    default: 0


Need to explain in the description why the default value was selected
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#dt-bindings-default-rules

Same for the properties below

explanation added

This also needs to be done for the properties below

Added default values are from documentation - default value per register

Actually, why is this property needed? It's redundant to the reg property

I think using reg for that purpose will be kind of confusing.
spi-addr is something specific to MAX14906 and MAX14916.

ozersa · 2024-08-22T09:07:12Z

drivers/gpio/gpio_max14906.h

+union max14906_doi_level {
+	uint8_t reg_raw;
+	struct {
+		uint8_t VDDOK_FAULT1:1; /* BIT0 */
+		uint8_t VDDOK_FAULT2:1;
+		uint8_t VDDOK_FAULT3:1;
+		uint8_t VDDOK_FAULT4:1;
+		uint8_t SAFE_DAMAGE_F1:1;
+		uint8_t SAFE_DAMAGE_F2:1;
+		uint8_t SAFE_DAMAGE_F3:1;
+		uint8_t SAFE_DAMAGE_F4:1; /* BIT7 */
+	} reg_bits;


It is obvious that bitfield and union simplify driver implementation.
I wonder does the zephyr encourage using bitfield @MaureenHelm

#4009
#4254

MaureenHelm

It is obvious that bitfield and union simplify driver implementation.
I wonder does the zephyr encourage using bitfield @MaureenHelm

I don't think it should block this PR.

MaureenHelm · 2024-09-09T14:14:05Z

drivers/gpio/gpio_max149x6.h

+ * @param val - Value which is to be written to requested register.
+ * @return 0 in case of success, negative error code otherwise.
+ */
+static int max149x6_reg_transceive(const struct device *dev, uint8_t addr, uint8_t val,


Why is this implemented in the header file?

because it is shared from MAX14906 and MAX14916

MaureenHelm · 2024-09-09T14:14:10Z

drivers/gpio/gpio_max149x6.h

+ * @param encode - action to be performed - true(encode), false(decode)
+ * @return the resulted CRC5
+ */
+static uint8_t max149x6_crc(uint8_t *data, bool encode)


Why is this implemented in the header file?

because it is shared from MAX14906 and MAX14916

How about to to rename it as .c instead .h to it be more readable.

I am not sure if will be more readable. I prefer to keep it with .h to avoid confusion if max149x6 is separate driver.

It is ok, thanks.

MaureenHelm · 2024-09-09T14:17:10Z

dts/bindings/gpio/adi,max14906-gpio.yaml

+    type: boolean
+  spi-addr:
+    type: int
+    default: 0


Actually, why is this property needed? It's redundant to the reg property

MaureenHelm · 2024-09-09T14:22:15Z

dts/bindings/gpio/adi,max14906-gpio.yaml

+      - 0 mean enable
+      - 1 mean disable


Is this correct? I think it may be inverted

Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.

MaureenHelm · 2024-09-09T14:22:43Z

dts/bindings/gpio/adi,max14906-gpio.yaml

+      - 0 mean enable
+      - 1 mean disable


Is this correct? I think it may be inverted

Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.

MaureenHelm · 2024-09-09T14:23:01Z

dts/bindings/gpio/adi,max14906-gpio.yaml

+      - 0 mean enable
+      - 1 mean disable


Is this correct? I think it may be inverted

Yes it is inverted. Unfortunately multiplicative by copy/paste. Fixed on all locations.

drivers/gpio/gpio_max14916.c

MaureenHelm · 2024-09-09T14:26:20Z

drivers/gpio/gpio_max14916.c

+
+	if (!gpio_pin_get_dt(&config->fault_gpio)) {
+		LOG_ERR("FLT flag is rised");
+		ret = -1;


Use an errno

MaureenHelm · 2024-09-09T14:26:31Z

drivers/gpio/gpio_max14916.c

+		if (data->glob.interrupt.reg_bits.COM_ERR) {
+			LOG_ERR("MAX14916 Communication Error");
+		}
+		ret = -2;


Use an errno

MaureenHelm · 2024-09-09T14:29:10Z

dts/bindings/gpio/adi,max14916-gpio.yaml

+    type: boolean
+  spi-addr:
+    type: int
+    default: 0


Actually, why is this property needed? It's redundant to the reg property

MaureenHelm · 2024-10-01T18:28:58Z

@bogdanovs please rebase

@ttmut @ozersa can you take a look?

drivers/gpio/gpio_max14906.c

drivers/gpio/gpio_max14906.h

drivers/gpio/gpio_max14916.c

drivers/gpio/gpio_max14916.h

ozersa · 2024-10-02T11:59:50Z

Please run clang format check to remove below warnings.

bogdanovs · 2024-10-02T20:29:11Z

@MaureenHelm branch is rebased on latest main
@ozersa comments are addressed

ozersa · 2024-10-03T08:46:07Z

drivers/gpio/gpio_max14906.h

+struct max149x6_config {
+	struct spi_dt_spec spi;
+	struct gpio_dt_spec fault_gpio;
+	struct gpio_dt_spec ready_gpio;
+	struct gpio_dt_spec sync_gpio;
+	struct gpio_dt_spec en_gpio;
+	bool crc_en;
+	union max14906_config1 config1;
+	union max14906_config2 config2;
+	union max14906_config_curr_lim curr_lim;
+	union max14906_config_di config_do;
+	union max14906_config_do config_di;
+	enum max149x6_spi_addr spi_addr;
+	uint8_t pkt_size;
+};
+
+#define max14906_config max149x6_config


Is there a specific reason not using typedef?

Mainly because of @MaureenHelm comment here #71141 (comment)
I personally prefer them but since they are not very welcomed in Zephyr that is fine by me.

Thanks to point it.

ozersa · 2024-10-03T08:49:08Z

drivers/gpio/gpio_max14906.c

+#define max14906_reg_read(dev, addr) max14906_reg_trans_spi_diag(dev, addr, 0, MAX149x6_READ)
+#define max14906_reg_write(dev, addr, val) max14906_reg_trans_spi_diag(dev, addr, val, \


It is fine for me but might be not match with naming convention for macro
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions

Just read the article , but I am not sure what you are referring to. @ozersa can you elaborate please?

Seems macros defined with upper case (MIN, MAX, ARRAY_SIZE...) that make it more readable, when looking the code it clearly mention either it is macro or not when reading source code.

Ok yes, that make sens. I thought you are talking about how macros names are worded.
Will replace them with upper case.

bogdanovs · 2024-10-03T20:57:31Z

drivers/gpio/gpio_max149x6.h

+#define MAX149x6_READ  0
+#define MAX149x6_WRITE 1
+
+#define MAX149X6_GET_BIT(val, i) (0x1 & ( (val) >> (i)))


@ozersa here is GET_BIT define

MAX14906 in 4 channel I/O with advanced diagnostic. In SPI communication diagnostic status transmitted on every READ/WRITE which includes generic status of chip. Configuration both on global level and on per channel bases. Diagnostics includes : * Thermal overload * current limit * open wire detection * short to VDD * Above VDD * Safe DEmagnitization fault * VDD warning * VDD low * SPI/CRC Error * WDog Error * Loss GND Add app.overlay for MAX14906 driver. Tested with adopted basic/button and basic/blinky sample. Signed-off-by: Stoyan Bogdanov <[email protected]>

MAX14906 industrial 4 channel Input/Ouput GPIO expander with diagnostics. Per channel diagnostics for open wire, over current. Global diagnostic for power supply, communication and various fault conditions. Signed-off-by: Stoyan Bogdanov <[email protected]>

Industrial 8 channel output with advanced diagnostics. Allowing giagnostic configuration both on per channel or global bases In SPI communication diagnostic status transmitted on every READ/WRITE which includes generic status of chip. Diagnostics includes : * Oveload * Open Wire * Over current * Short to VDD * Thermal Shutdown * VDD Warn * Watch Dog Error * Communication Error * VDD under voltage Add app.overlay for MAX14916 driver. Tested with adopted basic/blinky example. Signed-off-by: Stoyan Bogdanov <[email protected]>

Industrial 8 channel input GPIO expander with diagnostics Per channel diagnostics from dtb Signed-off-by: Stoyan Bogdanov <[email protected]>

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: ADI Analog Devices, Inc. area: GPIO labels Jun 24, 2024

zephyrbot requested review from decsny, galak, henrikbrixandersen, MaureenHelm, microbuilder and mnkp June 24, 2024 14:52

zephyrbot assigned MaureenHelm Jun 24, 2024

bogdanovs force-pushed the adi-max149x6-gpio branch 2 times, most recently from 6059494 to 78c5fe0 Compare June 24, 2024 16:19

MaureenHelm requested changes Aug 2, 2024

View reviewed changes

bogdanovs force-pushed the adi-max149x6-gpio branch 2 times, most recently from ef4c38b to 4d07d53 Compare August 5, 2024 14:36

bogdanovs requested a review from MaureenHelm August 5, 2024 17:24

bogdanovs force-pushed the adi-max149x6-gpio branch from 4d07d53 to 7fa2e0e Compare August 8, 2024 12:49

ozersa reviewed Aug 22, 2024

View reviewed changes

MaureenHelm requested changes Sep 9, 2024

View reviewed changes

bogdanovs force-pushed the adi-max149x6-gpio branch from 7fa2e0e to dcbd148 Compare September 9, 2024 22:06

zephyrbot requested a review from ttmut September 9, 2024 22:06

bogdanovs requested a review from MaureenHelm September 9, 2024 22:09

MaureenHelm added this to the v4.0.0 milestone Sep 11, 2024

MaureenHelm previously approved these changes Sep 23, 2024

View reviewed changes

bogdanovs requested a review from ozersa September 24, 2024 09:19

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14906.c Outdated Show resolved Hide resolved

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14906.c Outdated Show resolved Hide resolved

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14906.h Show resolved Hide resolved

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14916.c Outdated Show resolved Hide resolved

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14916.c Outdated Show resolved Hide resolved

ozersa reviewed Oct 2, 2024

View reviewed changes

drivers/gpio/gpio_max14916.h Show resolved Hide resolved

bogdanovs dismissed MaureenHelm’s stale review via 11d7f63 October 2, 2024 20:23

bogdanovs force-pushed the adi-max149x6-gpio branch from dcbd148 to 11d7f63 Compare October 2, 2024 20:23

ozersa reviewed Oct 3, 2024

View reviewed changes

bogdanovs force-pushed the adi-max149x6-gpio branch 2 times, most recently from e41f5fe to 83e3d37 Compare October 3, 2024 20:55

bogdanovs commented Oct 3, 2024

View reviewed changes

bogdanovs force-pushed the adi-max149x6-gpio branch from 83e3d37 to ae203ac Compare October 3, 2024 21:34

bogdanovs added 4 commits October 4, 2024 00:52

dts: bindings: gpio: Add dtb bindings for MAX14916

ac70f34

Industrial 8 channel input GPIO expander with diagnostics Per channel diagnostics from dtb Signed-off-by: Stoyan Bogdanov <[email protected]>

bogdanovs force-pushed the adi-max149x6-gpio branch from ae203ac to ac70f34 Compare October 3, 2024 21:53

bogdanovs requested review from ozersa and MaureenHelm October 3, 2024 23:07

		#define max14906_reg_read(dev, addr) max14906_reg_trans_spi_diag(dev, addr, 0, MAX149x6_READ)
		#define max14906_reg_write(dev, addr, val) max14906_reg_trans_spi_diag(dev, addr, val, \

Add support for MAX14906 and MAX14916 industrial IO with diagnostics #74891

Are you sure you want to change the base?

Add support for MAX14906 and MAX14916 industrial IO with diagnostics #74891

Conversation

bogdanovs commented Jun 24, 2024

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

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

bogdanovs Aug 8, 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

MaureenHelm 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

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

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

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

MaureenHelm commented Oct 1, 2024

ozersa commented Oct 2, 2024

bogdanovs commented Oct 2, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanovs Aug 8, 2024 •

edited

Loading