drivers: gpio: max22190: Add MAX22190 octal input with diagnostics #71141

bogdanovs · 2024-04-05T11:52:26Z

Add max22190 gpio driver with input functionality, since device support only input with output.

Implemented diagnostic funcionality for all 8 channels which include various check to over/under voltage and wire break. Filtering configuration is done from devicetree on per channel bases and is configured on chip start.

In case of some fault condition occure FAULT pin drive LOW which prop to FAULT registers to be read. Data is stored in data structure for furter analizes and ERR message is prited in console.

decsny · 2024-04-22T18:58:26Z

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

default value requires justification in description (same for other properties)

sure, I add clarification for all. Could you take a look if it is enough ?

I think for the mode you should just remove the default value, it needs to be specified in the board DT, it is hardwired signals on the board after all, needs to be described

I specified default value because this was mode set on the devkit it self, but it make sens to have it in DT.

decsny · 2024-04-23T15:58:01Z

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

I think for the mode you should just remove the default value, it needs to be specified in the board DT, it is hardwired signals on the board after all, needs to be described

decsny · 2024-04-23T16:00:16Z

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

I think you should clarify that this default value is not the default value of the hardware

I think it is better all filter-wbes , filter-fbps and filter-delays to have for default values from datasheet.
This mean:

wire break - 0 disable

fbps - 1 bypass

delays - 50

Value which was set is the one which is in use in case WBE is 1. In that case delays is 20000.

okay, but now the description seems wrong, it still says set 1? is it intentional, it seems unclear at best

Maybe my language here is not quiet clear. I want to state that by default every channel is with disabled Wire Break functionality - WBEx bit = 0.
To enable specific channel Wire Break functionality WBEx bit = 1.
Example for channel first 4 channels enabled WB and last 4 disabled WB
[ 1, 1, 1, 1, 0, 0, 0, 0]

If you have some suggestions how to make it clearer please share it.

decsny · 2024-04-23T16:01:33Z

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

word this more like "The default value corresponds to the default value of the hardware"

decsny · 2024-04-23T16:01:42Z

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

similar comment as above

addressed

MaureenHelm

Please add to drivers/build_all/gpio/app.overlay

drivers/gpio/Kconfig.max22190

drivers/gpio/gpio_max22190.c

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

Piziwate · 2024-10-21T13:57:32Z

Thank you for this contribution, which I find interesting. However, I have a few small remarks or suggestions :

It seems to me that the latch pin is configured but never used. Typically, it is used when the IC operates in Daisy Chain mode.
It would be very beneficial to support Daisy Chain mode, as it is frequently used with this type of IC.

Another more general remark regarding the GPIO API: it is unfortunate that it is not possible to report errors on the GPIOs through the API (such as wire break, fault, etc.). The reading and analysis of error registers only provide output to the console. This is not very useful for normal operating conditions.

bogdanovs · 2024-10-22T07:10:06Z

Thanks @Piziwate

It seems to me that the latch pin is configured but never used. Typically, it is used when the IC operates in Daisy Chain mode.

It would be very beneficial to support Daisy Chain mode, as it is frequently used with this type of IC.

Agree it will be useful, I can add support for that in next PR after this one is merged.

Another more general remark regarding the GPIO API: it is unfortunate that it is not possible to report errors on the GPIOs through the API (such as wire break, fault, etc.). The reading and analysis of error registers only provide output to the console. This is not very useful for normal operating conditions.

To be hones there are number of ICs (MAX22190, MAX14906, MAX14916 and other) which have advanced diagnostics and lack of way to report it in GPIO API really limit implementation and functionality. I was thinning to extend GPIO API to support diagnostics callbacks of something similar.

+#ifdef CONFIG_GPIO_DIAGNOSTIC
+       int (*diag_config_get)(const struct device *dev);
+       int (*diag_config_set)(const struct device *dev);
+       int (*diag_check)(const struct device *dev);
+#endif

Piziwate · 2024-10-22T08:45:05Z

Thanks @Piziwate

It seems to me that the latch pin is configured but never used. Typically, it is used when the IC operates in Daisy Chain mode.

It would be very beneficial to support Daisy Chain mode, as it is frequently used with this type of IC.

Agree it will be useful, I can add support for that in next PR after this one is merged.

Another more general remark regarding the GPIO API: it is unfortunate that it is not possible to report errors on the GPIOs through the API (such as wire break, fault, etc.). The reading and analysis of error registers only provide output to the console. This is not very useful for normal operating conditions.

To be hones there are number of ICs (MAX22190, MAX14906, MAX14916 and other) which have advanced diagnostics and lack of way to report it in GPIO API really limit implementation and functionality. I was thinning to extend GPIO API to support diagnostics callbacks of something similar.
+#ifdef CONFIG_GPIO_DIAGNOSTIC
+       int (*diag_config_get)(const struct device *dev);
+       int (*diag_config_set)(const struct device *dev);
+       int (*diag_check)(const struct device *dev);
+#endif

Indeed, that would be great! Ideally, the configuration of the filters and the wirebreak should also be possible via API in order to dynamically change the operating mode.
For your information, the Daisy Chain mode has been implemented for this driver #65982, it might inspire you.

bogdanovs · 2024-10-22T10:30:34Z

Indeed, that would be great! Ideally, the configuration of the filters and the wirebreak should also be possible via API in order to dynamically change the operating mode.

This was just sample snippet, but yes all of those should be possible to configure. Maybe flags approach for overlapping functions and room for extension for custom flags and functions will be good. Need loo back on it think it thought again.

For your information, the Daisy Chain mode has been implemented for this driver #65982, it might inspire you.

Thanks!

drivers/gpio/gpio_max22190.c

kartben · 2024-12-10T01:51:45Z

drivers/gpio/gpio_max22190.c

Please move crc5 function to CRC subsystem (crc.h), we don't want more duplication of this code (see e.g.

zephyr/drivers/gpio/gpio_max149x6.h

Line 30 in 3a69ed6

static uint8_t max149x6_crc(uint8_t *data, bool encode)

or

zephyr/drivers/i3c/i3c_cdns.c

Line 601 in 3a69ed6

static uint8_t i3c_cdns_crc5(uint8_t crc5, uint16_t word)

)
You may then also make CRC checking optional via a Kconfig like many other drivers do

@kartben for MAX22190 and MAX149x6 CRC5 functions are taken from chip manufacturer noOS repo. From my understanding and testing calculation of CRC5 is different for those chips, following different flow.

for MAX22190
https://github.com/analogdevicesinc/no-OS/blob/cc73a3d70d056d143b783bd846489e2d2819ba7c/drivers/digital-io/max22190/max22190.c#L45

for MAX149x6
https://github.com/analogdevicesinc/no-OS/blob/cc73a3d70d056d143b783bd846489e2d2819ba7c/drivers/digital-io/max149x6/max149x6-base.c#L46

For i3c_cdns looks different too, but I am not familiar with that driver.

I can move MAX22190 CRC5 function to CRC subsystem if you think it is better there, even it is specific to MAX22190.

You may then also make CRC checking optional via a Kconfig like many other drivers do

If CRC is enabled depends on SPI Interface Modes - device tree option for this is max22190-mode. There are four modes available in for CRC and Daisychain. Mode is set from two pins which are pulled up or down M0 and M1, so it is hardware specific and belong to DT. From combination of those two pins CRC and Packet size are taken according to table.

Right, let's process with the way it is right now. It is not clear to me if these various CRC5 variants cannot be factorized into a common on, but this shouldn't prevent this PR to land.

kartben · 2024-12-17T16:43:46Z

now has a merge conflict, unfortunately, please rebase @bogdanovs

Add max22190 gpio driver with input functionality, since device support only input without output. Implemented diagnostic functionality for all 8 channels which include various check to over/under voltage and wire break. Filtering configuration is done from devicetree on per channel bases and is configured on chip start. In case some fault condition occure FAULT pin drive LOW which prop to FAULT registers to be read. Data is stored in data structure for furter analizes and ERR message is printed in console. Signed-off-by: Stoyan Bogdanov <[email protected]>

bogdanovs · 2024-12-17T17:18:01Z

now has a merge conflict, unfortunately, please rebase @bogdanovs

@kartben rebased on latest main

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

zephyrbot requested review from MaureenHelm, galak, henrikbrixandersen, microbuilder and mnkp April 5, 2024 11:53

zephyrbot assigned MaureenHelm Apr 5, 2024

carlocaione requested review from carlocaione and gmarull April 5, 2024 11:54

bogdanovs force-pushed the adi-max22190-gpio branch 2 times, most recently from 5ac5f2c to 823f4da Compare April 8, 2024 11:07

bogdanovs force-pushed the adi-max22190-gpio branch from 823f4da to 82aeb3f Compare April 22, 2024 10:09

zephyrbot requested a review from decsny April 22, 2024 10:09

bogdanovs force-pushed the adi-max22190-gpio branch 2 times, most recently from d903a24 to 1aa75d3 Compare April 22, 2024 11:58

decsny previously requested changes Apr 22, 2024

View reviewed changes

bogdanovs force-pushed the adi-max22190-gpio branch from 1aa75d3 to f589043 Compare April 23, 2024 11:32

bogdanovs requested a review from decsny April 23, 2024 11:34

decsny reviewed Apr 23, 2024

View reviewed changes

bogdanovs force-pushed the adi-max22190-gpio branch from f589043 to b154371 Compare April 23, 2024 18:59

bogdanovs requested a review from decsny April 23, 2024 19:00

MaureenHelm previously requested changes Apr 24, 2024

View reviewed changes

bogdanovs force-pushed the adi-max22190-gpio branch from b154371 to b5cbd91 Compare April 25, 2024 11:02

bogdanovs requested a review from MaureenHelm April 25, 2024 12:27

bogdanovs force-pushed the adi-max22190-gpio branch 3 times, most recently from 21d8d38 to 36b0edc Compare May 13, 2024 12:22

bogdanovs force-pushed the adi-max22190-gpio branch 2 times, most recently from f52e6fc to 06ff40a Compare October 21, 2024 08:23

bogdanovs requested a review from MaureenHelm October 21, 2024 12:20

MaureenHelm previously approved these changes Oct 21, 2024

View reviewed changes

bogdanovs dismissed MaureenHelm’s stale review via 1fd68e5 October 31, 2024 09:28

bogdanovs force-pushed the adi-max22190-gpio branch from 06ff40a to 1fd68e5 Compare October 31, 2024 09:28

mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Nov 8, 2024

bogdanovs force-pushed the adi-max22190-gpio branch from 1fd68e5 to fe695a6 Compare December 3, 2024 15:03

bogdanovs requested a review from MaureenHelm December 3, 2024 15:03

MaureenHelm reviewed Dec 6, 2024

View reviewed changes

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

bogdanovs force-pushed the adi-max22190-gpio branch from fe695a6 to 67b5c0b Compare December 9, 2024 09:38

MaureenHelm previously approved these changes Dec 9, 2024

View reviewed changes

kartben reviewed Dec 10, 2024

View reviewed changes

kartben previously approved these changes Dec 17, 2024

View reviewed changes

bogdanovs dismissed stale reviews from kartben and MaureenHelm via 284b0a3 December 17, 2024 17:15

bogdanovs force-pushed the adi-max22190-gpio branch from 67b5c0b to 284b0a3 Compare December 17, 2024 17:15

bogdanovs requested review from MaureenHelm and kartben December 17, 2024 17:19

MaureenHelm approved these changes Dec 17, 2024

View reviewed changes

kartben approved these changes Dec 18, 2024

View reviewed changes

kartben merged commit 92aeb78 into zephyrproject-rtos:main Dec 18, 2024
24 checks passed

drivers: gpio: max22190: Add MAX22190 octal input with diagnostics #71141

drivers: gpio: max22190: Add MAX22190 octal input with diagnostics #71141

Uh oh!

Conversation

bogdanovs commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piziwate commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogdanovs commented Oct 22, 2024

Uh oh!

Piziwate commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogdanovs commented Oct 22, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdanovs Dec 11, 2024 • edited by kartben Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartben commented Dec 17, 2024

Uh oh!

bogdanovs commented Dec 17, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Piziwate commented Oct 21, 2024 •

edited

Loading

Piziwate commented Oct 22, 2024 •

edited

Loading

bogdanovs Dec 11, 2024 •

edited by kartben

Loading